You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Matthew Hatem <mh...@gmail.com> on 2011/08/24 01:06:15 UTC
Review Request: Selection feature implementation does not match spec
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1615/
-----------------------------------------------------------
Review request for shindig and Ryan Baxter.
Summary
-------
https://issues.apache.org/jira/browse/SHINDIG-1591
This addresses bug SHINDIG-1591.
https://issues.apache.org/jira/browse/SHINDIG-1591
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436
Diff: https://reviews.apache.org/r/1615/diff
Testing
-------
Passes all unit tests.
Thanks,
Matthew
Re: Review Request: Selection feature implementation does not match spec
Posted by Matthew Hatem <mh...@gmail.com>.
> On 2011-08-24 00:22:17, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96
> > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>
> >
> > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?
This allows the container to add and remove listeners, has nothing to do with gadget level API.
> On 2011-08-24 00:22:17, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js, line 84
> > <https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84>
> >
> > Shouldn't there be a test for removeListener as well?
Yes I will add one.
- Matthew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1615/#review1604
-----------------------------------------------------------
On 2011-08-23 23:06:15, Matthew Hatem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1615/
> -----------------------------------------------------------
>
> (Updated 2011-08-23 23:06:15)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Summary
> -------
>
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> This addresses bug SHINDIG-1591.
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436
>
> Diff: https://reviews.apache.org/r/1615/diff
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Matthew
>
>
Re: Review Request: Selection feature implementation does not match spec
Posted by Matthew Hatem <mh...@gmail.com>.
> On 2011-08-24 00:22:17, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96
> > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>
> >
> > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?
>
> Matthew Hatem wrote:
> This allows the container to add and remove listeners, has nothing to do with gadget level API.
>
> Ryan Baxter wrote:
> So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why?
We've talked about this before. Each gadget adds only one listener (lazily) to the list in the container which proxies events to (potentially many) listeners registered with a local list at the gadget. This cuts down on the number of RPCs that are necessary to notify everyone.
You might argue that I need a lifecycle listener to cleanup the proxy listeners. I don't want to hold up this patch for that though, I'll open a separate issue for that.
> On 2011-08-24 00:22:17, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js, line 84
> > <https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84>
> >
> > Shouldn't there be a test for removeListener as well?
>
> Matthew Hatem wrote:
> Yes I will add one.
I take that back. It's currently not possible to test this without exposing things that should be private.
Is there plan to upgrade/improve the unit testing in Shinding?
- Matthew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1615/#review1604
-----------------------------------------------------------
On 2011-08-23 23:06:15, Matthew Hatem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1615/
> -----------------------------------------------------------
>
> (Updated 2011-08-23 23:06:15)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Summary
> -------
>
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> This addresses bug SHINDIG-1591.
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436
>
> Diff: https://reviews.apache.org/r/1615/diff
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Matthew
>
>
Re: Review Request: Selection feature implementation does not match spec
Posted by Ryan Baxter <rb...@gmail.com>.
> On 2011-08-24 00:22:17, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js, line 84
> > <https://reviews.apache.org/r/1615/diff/1/?file=34111#file34111line84>
> >
> > Shouldn't there be a test for removeListener as well?
>
> Matthew Hatem wrote:
> Yes I will add one.
>
> Matthew Hatem wrote:
> I take that back. It's currently not possible to test this without exposing things that should be private.
>
> Is there plan to upgrade/improve the unit testing in Shinding?
Ah ok.
Not that I know of, but we are open to suggestions if you have ideas :)
> On 2011-08-24 00:22:17, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96
> > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>
> >
> > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?
>
> Matthew Hatem wrote:
> This allows the container to add and remove listeners, has nothing to do with gadget level API.
>
> Ryan Baxter wrote:
> So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why?
>
> Matthew Hatem wrote:
> We've talked about this before. Each gadget adds only one listener (lazily) to the list in the container which proxies events to (potentially many) listeners registered with a local list at the gadget. This cuts down on the number of RPCs that are necessary to notify everyone.
>
> You might argue that I need a lifecycle listener to cleanup the proxy listeners. I don't want to hold up this patch for that though, I'll open a separate issue for that.
Ah yes now I remember, thanks for the refresher.
Yes it would be nice to clean this up.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1615/#review1604
-----------------------------------------------------------
On 2011-08-23 23:06:15, Matthew Hatem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1615/
> -----------------------------------------------------------
>
> (Updated 2011-08-23 23:06:15)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Summary
> -------
>
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> This addresses bug SHINDIG-1591.
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436
>
> Diff: https://reviews.apache.org/r/1615/diff
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Matthew
>
>
Re: Review Request: Selection feature implementation does not match spec
Posted by Ryan Baxter <rb...@gmail.com>.
> On 2011-08-24 00:22:17, Ryan Baxter wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js, line 96
> > <https://reviews.apache.org/r/1615/diff/1/?file=34109#file34109line96>
> >
> > How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?
>
> Matthew Hatem wrote:
> This allows the container to add and remove listeners, has nothing to do with gadget level API.
So when the gadget adds a listener we add it to a list in the container, but when a gadget removes a listener we never remove it from the list in the container....why?
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1615/#review1604
-----------------------------------------------------------
On 2011-08-23 23:06:15, Matthew Hatem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1615/
> -----------------------------------------------------------
>
> (Updated 2011-08-23 23:06:15)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Summary
> -------
>
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> This addresses bug SHINDIG-1591.
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436
>
> Diff: https://reviews.apache.org/r/1615/diff
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Matthew
>
>
Re: Review Request: Selection feature implementation does not match spec
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1615/#review1604
-----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js
<https://reviews.apache.org/r/1615/#comment3626>
How does this ever get called? Shouldn't this be called when the gadget calles removeListener? If so it should be called from your router function...right?
http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js
<https://reviews.apache.org/r/1615/#comment3627>
Shouldn't there be a test for removeListener as well?
- Ryan
On 2011-08-23 23:06:15, Matthew Hatem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1615/
> -----------------------------------------------------------
>
> (Updated 2011-08-23 23:06:15)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Summary
> -------
>
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> This addresses bug SHINDIG-1591.
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436
>
> Diff: https://reviews.apache.org/r/1615/diff
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Matthew
>
>
Re: Review Request: Selection feature implementation does not match spec
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1615/#review1618
-----------------------------------------------------------
Ship it!
Committed revision 1161102.
Please close the JIRA with this revision. Thanks Matt.
- Ryan
On 2011-08-23 23:06:15, Matthew Hatem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1615/
> -----------------------------------------------------------
>
> (Updated 2011-08-23 23:06:15)
>
>
> Review request for shindig and Ryan Baxter.
>
>
> Summary
> -------
>
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> This addresses bug SHINDIG-1591.
> https://issues.apache.org/jira/browse/SHINDIG-1591
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-actions-runner.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/conservcontainer/sample-selection-listener.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/feature.xml 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/selection_container.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/selection/taming.js 1160436
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/selection/selection_test.js 1160436
>
> Diff: https://reviews.apache.org/r/1615/diff
>
>
> Testing
> -------
>
> Passes all unit tests.
>
>
> Thanks,
>
> Matthew
>
>