Shindig
  1. Shindig
  2. SHINDIG-1772

gadgets.io.RequestParameters.SIGN_OWNER and gadgets.io.RequestParameters.SIGN_VIEWER not implemented in Shindig

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.0
    • Fix Version/s: 2.5.0-beta2
    • Component/s: None
    • Labels:
      None

      Description

      The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-14 13:16:37, Ryan Baxter wrote:

        > Erik every looks good, please attach the patch to the JIRA and I will commit this.

        Committed revision 1339144. Please close the review.

        • Ryan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7840
        -----------------------------------------------------------

        On 2012-05-14 12:39:40, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-14 12:39:40)

        Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-14 13:16:37, Ryan Baxter wrote: > Erik every looks good, please attach the patch to the JIRA and I will commit this. Committed revision 1339144. Please close the review. Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7840 ----------------------------------------------------------- On 2012-05-14 12:39:40, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-14 12:39:40) Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        Ryan Baxter added a comment -

        Committed revision 1339144

        Show
        Ryan Baxter added a comment - Committed revision 1339144
        Hide
        Erik BI added a comment -

        Ryan. I met server error when I tried to attached the patch file. here it is.

        Show
        Erik BI added a comment - Ryan. I met server error when I tried to attached the patch file. here it is.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7840
        -----------------------------------------------------------

        Ship it!

        Erik every looks good, please attach the patch to the JIRA and I will commit this.

        • Ryan

        On 2012-05-14 12:39:40, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-14 12:39:40)

        Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7840 ----------------------------------------------------------- Ship it! Erik every looks good, please attach the patch to the JIRA and I will commit this. Ryan On 2012-05-14 12:39:40, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-14 12:39:40) Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/
        -----------------------------------------------------------

        (Updated 2012-05-14 12:39:40.305248)

        Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.

        Summary
        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.
        https://issues.apache.org/jira/browse/shindig-1772

        Diffs (updated)


        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432
        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432
        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing
        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-14 12:39:40.305248) Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs (updated) http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-10 12:38:56, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js, line 449

        > <https://reviews.apache.org/r/5085/diff/2/?file=108271#file108271line449>

        >

        > I prefer we use the new constant here

        Erik Bi wrote:

        the reason I still see the parameter name instead the constant is because I want to to keep constant with other code in the makeRequest function, as you can see, in this function, it checks many parameters, but all use the string value directly instead of the constant. your opinion?

        Ryan Baxter wrote:

        Just because the rest of the code does it doesn't mean its the best thing to do. At least by using the constants you are "testing" them and making sure their values are what you expect.

        Make sense. I will change it to using constant.

        • Erik

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7765
        -----------------------------------------------------------

        On 2012-05-14 12:39:40, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-14 12:39:40)

        Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-10 12:38:56, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js , line 449 > < https://reviews.apache.org/r/5085/diff/2/?file=108271#file108271line449 > > > I prefer we use the new constant here Erik Bi wrote: the reason I still see the parameter name instead the constant is because I want to to keep constant with other code in the makeRequest function, as you can see, in this function, it checks many parameters, but all use the string value directly instead of the constant. your opinion? Ryan Baxter wrote: Just because the rest of the code does it doesn't mean its the best thing to do. At least by using the constants you are "testing" them and making sure their values are what you expect. Make sense. I will change it to using constant. Erik ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7765 ----------------------------------------------------------- On 2012-05-14 12:39:40, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-14 12:39:40) Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-10 12:38:56, Ryan Baxter wrote:

        > I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too.

        Stanton Sievers wrote:

        +1

        Erik Bi wrote:

        Yes, it's easy to support both, but I thought that the chance somebody was using it should be very small, and if somebody did, he must have been looking through the code and found the parameter name, because it doesn't exist in the spec, and that is wrong...

        Do you think maybe it's better I ask around the shindig group see if someone used the old value before? if there is, we will add the support for old value but comment it deprecated, if not, just drop them.

        I cant argue with anything you are saying, but I figured since it would take very little effort to add backward compatibility it wouldn't be a big deal. However the chance of people running into this is probably small so I don't see that strongly about it.

        Please add the Shindig group as a reviewer.

        On 2012-05-10 12:38:56, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js, line 449

        > <https://reviews.apache.org/r/5085/diff/2/?file=108271#file108271line449>

        >

        > I prefer we use the new constant here

        Erik Bi wrote:

        the reason I still see the parameter name instead the constant is because I want to to keep constant with other code in the makeRequest function, as you can see, in this function, it checks many parameters, but all use the string value directly instead of the constant. your opinion?

        Just because the rest of the code does it doesn't mean its the best thing to do. At least by using the constants you are "testing" them and making sure their values are what you expect.

        • Ryan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7765
        -----------------------------------------------------------

        On 2012-05-10 03:44:35, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-10 03:44:35)

        Review request for Ryan Baxter, Dan Dumont and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-10 12:38:56, Ryan Baxter wrote: > I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too. Stanton Sievers wrote: +1 Erik Bi wrote: Yes, it's easy to support both, but I thought that the chance somebody was using it should be very small, and if somebody did, he must have been looking through the code and found the parameter name, because it doesn't exist in the spec, and that is wrong... Do you think maybe it's better I ask around the shindig group see if someone used the old value before? if there is, we will add the support for old value but comment it deprecated, if not, just drop them. I cant argue with anything you are saying, but I figured since it would take very little effort to add backward compatibility it wouldn't be a big deal. However the chance of people running into this is probably small so I don't see that strongly about it. Please add the Shindig group as a reviewer. On 2012-05-10 12:38:56, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js , line 449 > < https://reviews.apache.org/r/5085/diff/2/?file=108271#file108271line449 > > > I prefer we use the new constant here Erik Bi wrote: the reason I still see the parameter name instead the constant is because I want to to keep constant with other code in the makeRequest function, as you can see, in this function, it checks many parameters, but all use the string value directly instead of the constant. your opinion? Just because the rest of the code does it doesn't mean its the best thing to do. At least by using the constants you are "testing" them and making sure their values are what you expect. Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7765 ----------------------------------------------------------- On 2012-05-10 03:44:35, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-10 03:44:35) Review request for Ryan Baxter, Dan Dumont and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-10 12:38:56, Ryan Baxter wrote:

        > I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too.

        Stanton Sievers wrote:

        +1

        Yes, it's easy to support both, but I thought that the chance somebody was using it should be very small, and if somebody did, he must have been looking through the code and found the parameter name, because it doesn't exist in the spec, and that is wrong...
        Do you think maybe it's better I ask around the shindig group see if someone used the old value before? if there is, we will add the support for old value but comment it deprecated, if not, just drop them.

        On 2012-05-10 12:38:56, Ryan Baxter wrote:

        > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js, line 449

        > <https://reviews.apache.org/r/5085/diff/2/?file=108271#file108271line449>

        >

        > I prefer we use the new constant here

        the reason I still see the parameter name instead the constant is because I want to to keep constant with other code in the makeRequest function, as you can see, in this function, it checks many parameters, but all use the string value directly instead of the constant. your opinion?

        • Erik

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7765
        -----------------------------------------------------------

        On 2012-05-10 03:44:35, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-10 03:44:35)

        Review request for Ryan Baxter, Dan Dumont and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-10 12:38:56, Ryan Baxter wrote: > I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too. Stanton Sievers wrote: +1 Yes, it's easy to support both, but I thought that the chance somebody was using it should be very small, and if somebody did, he must have been looking through the code and found the parameter name, because it doesn't exist in the spec, and that is wrong... Do you think maybe it's better I ask around the shindig group see if someone used the old value before? if there is, we will add the support for old value but comment it deprecated, if not, just drop them. On 2012-05-10 12:38:56, Ryan Baxter wrote: > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js , line 449 > < https://reviews.apache.org/r/5085/diff/2/?file=108271#file108271line449 > > > I prefer we use the new constant here the reason I still see the parameter name instead the constant is because I want to to keep constant with other code in the makeRequest function, as you can see, in this function, it checks many parameters, but all use the string value directly instead of the constant. your opinion? Erik ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7765 ----------------------------------------------------------- On 2012-05-10 03:44:35, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-10 03:44:35) Review request for Ryan Baxter, Dan Dumont and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        Ryan Baxter added a comment -

        Rich I think I am a little confused on what you are saying here, when you say client are you talking about the gadget or the container?

        With the current patch, if my gadget used the old shindig defined constants then my gadget's functionality could break. For example setting the old parameters to false will cause Shindig not to add any extra URL parameters when making the request for a request token. If the provider you are doing the dance with is very strict and there are extra parameters on the request it may reject the request all together. When I go to use my gadget in a 2.5 container all of a sudden it will stop working. It won't take much to support both so we can avoid situations like this.

        Show
        Ryan Baxter added a comment - Rich I think I am a little confused on what you are saying here, when you say client are you talking about the gadget or the container? With the current patch, if my gadget used the old shindig defined constants then my gadget's functionality could break. For example setting the old parameters to false will cause Shindig not to add any extra URL parameters when making the request for a request token. If the provider you are doing the dance with is very strict and there are extra parameters on the request it may reject the request all together. When I go to use my gadget in a 2.5 container all of a sudden it will stop working. It won't take much to support both so we can avoid situations like this.
        Hide
        Rich Thompson added a comment -

        If the server supports both the spec defined constant and the old Shindig defined constant then the client should support both for backwards compatibility reasons. If the client was just using the wrong value for these use cases and the server ignored those values, then the old values were broken and should just be dropped

        Show
        Rich Thompson added a comment - If the server supports both the spec defined constant and the old Shindig defined constant then the client should support both for backwards compatibility reasons. If the client was just using the wrong value for these use cases and the server ignored those values, then the old values were broken and should just be dropped
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-05-10 12:38:56, Ryan Baxter wrote:

        > I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too.

        +1

        • Stanton

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7765
        -----------------------------------------------------------

        On 2012-05-10 03:44:35, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-10 03:44:35)

        Review request for Ryan Baxter, Dan Dumont and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-05-10 12:38:56, Ryan Baxter wrote: > I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too. +1 Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7765 ----------------------------------------------------------- On 2012-05-10 03:44:35, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-10 03:44:35) Review request for Ryan Baxter, Dan Dumont and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7765
        -----------------------------------------------------------

        I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too.

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js
        <https://reviews.apache.org/r/5085/#comment17072>

        I prefer we use the new constant here

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js
        <https://reviews.apache.org/r/5085/#comment17073>

        I prefer we use the new constant here

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js
        <https://reviews.apache.org/r/5085/#comment17074>

        I prefer we use the new constant here

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js
        <https://reviews.apache.org/r/5085/#comment17075>

        I prefer we use the new constant here

        • Ryan

        On 2012-05-10 03:44:35, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-10 03:44:35)

        Review request for Ryan Baxter, Dan Dumont and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7765 ----------------------------------------------------------- I am wondering if we want to try to not break backwards compatibility with this change. If there are gadgets out there that are passing VIEWER_SIGNED or OWNER_SIGNED as params they will stop working. While I think the chance of this is small, the code to support it is trivial. Comments in the code indicating why we look for both parameters would help too. http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js < https://reviews.apache.org/r/5085/#comment17072 > I prefer we use the new constant here http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js < https://reviews.apache.org/r/5085/#comment17073 > I prefer we use the new constant here http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js < https://reviews.apache.org/r/5085/#comment17074 > I prefer we use the new constant here http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js < https://reviews.apache.org/r/5085/#comment17075 > I prefer we use the new constant here Ryan On 2012-05-10 03:44:35, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-10 03:44:35) Review request for Ryan Baxter, Dan Dumont and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/#review7763
        -----------------------------------------------------------

        Ship it!

        LGTM

        • Stanton

        On 2012-05-10 03:44:35, Erik Bi wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/5085/

        -----------------------------------------------------------

        (Updated 2012-05-10 03:44:35)

        Review request for Ryan Baxter, Dan Dumont and Stanton Sievers.

        Summary

        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.

        https://issues.apache.org/jira/browse/shindig-1772

        Diffs

        -----

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432

        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing

        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/#review7763 ----------------------------------------------------------- Ship it! LGTM Stanton On 2012-05-10 03:44:35, Erik Bi wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- (Updated 2012-05-10 03:44:35) Review request for Ryan Baxter, Dan Dumont and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/5085/
        -----------------------------------------------------------

        Review request for Ryan Baxter, Dan Dumont and Stanton Sievers.

        Summary
        -------

        The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants.

        This addresses bug shindig-1772.
        https://issues.apache.org/jira/browse/shindig-1772

        Diffs


        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432
        http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432
        http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432

        Diff: https://reviews.apache.org/r/5085/diff

        Testing
        -------

        Update iotest.js

        Thanks,

        Erik

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5085/ ----------------------------------------------------------- Review request for Ryan Baxter, Dan Dumont and Stanton Sievers. Summary ------- The OpenSocial spec says that you should be able to pass 'SIGN_OWNER' and 'SIGN_VIEWER' as parameters to makeRequest. Shindig does not define these and in fact looks for the wrong parameters. In io.js Shindig looks for 'OWNER_SIGNED' and 'VIEWER_SIGNED'. In addition 'SIGN_OWNER' and 'SIGN_VIEWER' should be made constants. This addresses bug shindig-1772. https://issues.apache.org/jira/browse/shindig-1772 Diffs http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/proxied-form-post/post.js 1327432 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js 1327432 Diff: https://reviews.apache.org/r/5085/diff Testing ------- Update iotest.js Thanks, Erik

          People

          • Assignee:
            Unassigned
            Reporter:
            Ryan Baxter
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development