Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2020463002:
Mojo JS: Attempt to simplify bindTo* related code. (Closed)
|
Created:
4 years, 7 months ago by dpapad Modified:
4 years, 6 months ago Reviewers:
yzshen1 CC:
Aaron Boodman, abarth-chromium, arv+watch_chromium.org, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojo JS: Attempt to simplify bindTo* related code.
Introducing a new bindStubDerivedImpl() method, which hides unnecessary
details from client code (no need to manually create a message pipe,
no need to deal with an intermediate "stub" object).
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2ee1c2e2053a4c186b94ecc70ca7026060b381ff
Cr-Commit-Position: refs/heads/master@{#396600}
Patch Set 1 #
Total comments: 5
Patch Set 2 : More refinements. #
Total comments: 4
Patch Set 3 : Addressing comemnts. #Patch Set 4 : Rename #
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Mojo JS: Attempt to simplify bindTo* related code, WIP. BUG= ========== to ========== Mojo JS: Attempt to simplify bindTo* related code, WIP. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
yzshen@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... chrome/browser/resources/plugins.js:305: connection.bindHandleToObj(pipe.handle0, pageProxy); Does it make sense to name it bindHandleToImpl()? Impl indicates that this is the implementation side of an interface. Obj seems a little vague. PluginsPageProxy should probably be named PluginsPageImpl. "Proxy" usually used for the caller side of an interface. https://codereview.chromium.org/2020463002/diff/1/mojo/public/js/connection.js File mojo/public/js/connection.js (right): https://codereview.chromium.org/2020463002/diff/1/mojo/public/js/connection.j... mojo/public/js/connection.js:151: function bindHandleToObj(handle, obj) { Please add comments.
Description was changed from ========== Mojo JS: Attempt to simplify bindTo* related code, WIP. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Mojo JS: Attempt to simplify bindTo* related code. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Thanks for all the suggestions on the discussion thread. I think these improvements simplify quite a bit the client code. Further work to make the JS bindings look more similar to C++ requires more in-depth understanding of the JS bindings code, so I think it is better suited for later in the Mojo+WebUI project. PTAL https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... chrome/browser/resources/plugins.js:305: connection.bindHandleToObj(pipe.handle0, pageProxy); On 2016/05/27 at 16:24:45, yzshen1 wrote: > Does it make sense to name it bindHandleToImpl()? > Impl indicates that this is the implementation side of an interface. Obj seems a little vague. > > PluginsPageProxy should probably be named PluginsPageImpl. "Proxy" usually used for the caller side of an interface. I found a way to simplify even more by completely hiding the pipe creation, and simply exposing (returning) the end of the pipe that needs to be sent to C++. This makes the client code simpler 1) No need to create message pipe. 2) New method only receives one parameter, the implementation object. 3) Client code no longer needs to depend directly on the Mojo "core" module, it was only needed to call core.createMessagePipe. Regarding the naming "pageProxy" I prefer to keep it as-is for two reasons. 1) Even though the "pageProxy" object is not residing on the caller side, it still acts as a proxy since it is an intermediate to execute methods on the page (see how PluginsPageProxy#onPluginsUpdated calls returnPluginsData). In other words, I am considering "returnPluginsData" (and all other global methods in plugins.js) as the real implementation, and |pageProxy| as an intermediate object. 2) From the test's point of view, which simulates incoming calls from the browser, pageProxy matches exactly your expectation (caller side of an interface), see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w....
https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... chrome/browser/resources/plugins.js:305: connection.bindHandleToObj(pipe.handle0, pageProxy); On 2016/05/27 17:55:37, dpapad wrote: > On 2016/05/27 at 16:24:45, yzshen1 wrote: > > Does it make sense to name it bindHandleToImpl()? > > Impl indicates that this is the implementation side of an interface. Obj seems > a little vague. > > > > PluginsPageProxy should probably be named PluginsPageImpl. "Proxy" usually > used for the caller side of an interface. > > I found a way to simplify even more by completely hiding the pipe creation, and > simply exposing (returning) the end of the pipe that needs to be sent to C++. > This makes the client code simpler > 1) No need to create message pipe. > 2) New method only receives one parameter, the implementation object. > 3) Client code no longer needs to depend directly on the Mojo "core" module, it > was only needed to call core.createMessagePipe. > > Regarding the naming "pageProxy" I prefer to keep it as-is for two reasons. > 1) Even though the "pageProxy" object is not residing on the caller side, it > still acts as a proxy since it is an intermediate to execute methods on the page > (see how PluginsPageProxy#onPluginsUpdated calls returnPluginsData). In other > words, I am considering "returnPluginsData" (and all other global methods in > plugins.js) as the real implementation, and |pageProxy| as an intermediate > object. > > 2) From the test's point of view, which simulates incoming calls from the > browser, pageProxy matches exactly your expectation (caller side of an > interface), see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w.... Okay. I am fine with any name that seems appropriate to the domain experts like you. :) Just saying that it feel counter-intuitive to me since "Proxy" has a specific meaning in the JS bindings. https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... File mojo/public/js/connection.js (right): https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... mojo/public/js/connection.js:158: function createHandleForImpl(obj) { I like this method! I feel that createMessagePipeAndBindImpl() may be more clear. Because createHandleForImpl() sounds like the handle returned is for the impl, but actually it is for the opposite side. WDYT?
https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... File chrome/browser/resources/plugins.js (right): https://codereview.chromium.org/2020463002/diff/1/chrome/browser/resources/pl... chrome/browser/resources/plugins.js:305: connection.bindHandleToObj(pipe.handle0, pageProxy); > Okay. I am fine with any name that seems appropriate to the domain experts like you. :) Just saying that it feel counter-intuitive to me since "Proxy" has a specific meaning in the JS bindings. Ended up renaming to PluginsPageImpl per your suggestion, to be more consistent with the usage of the "term" proxy withing JS bindings terminology (bindings.js). https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... File mojo/public/js/connection.js (right): https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... mojo/public/js/connection.js:158: function createHandleForImpl(obj) { On 2016/05/27 at 18:32:46, yzshen1 wrote: > I like this method! > > I feel that createMessagePipeAndBindImpl() may be more clear. Because createHandleForImpl() sounds like the handle returned is for the impl, but actually it is for the opposite side. > > WDYT? Done. As a side note, there is a similar (but not identical) bindImpl() method a few lines above. I think in follow up cleanups perhaps we can migrate all callers (if any) to use the newer method, and rename the newer method to simply bindImpl(). In any case I would prefer to leave such cleanups for later, such that I can simplify the chrome://plugins page now, which will be used as a reference example for future migrations.
LGTM https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... File mojo/public/js/connection.js (right): https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... mojo/public/js/connection.js:158: function createHandleForImpl(obj) { On 2016/05/27 20:36:56, dpapad wrote: > On 2016/05/27 at 18:32:46, yzshen1 wrote: > > I like this method! > > > > I feel that createMessagePipeAndBindImpl() may be more clear. Because > createHandleForImpl() sounds like the handle returned is for the impl, but > actually it is for the opposite side. > > > > WDYT? > > Done. > As a side note, there is a similar (but not identical) bindImpl() method a few > lines above. I think in follow up cleanups perhaps we can migrate all callers > (if any) to use the newer method, and rename the newer method to simply > bindImpl(). In any case I would prefer to leave such cleanups for later, such > that I can simplify the chrome://plugins page now, which will be used as a > reference example for future migrations. Ah, I didn't notice that one! Then in order to make things more consistent, maybe this one could be named bindStubDerivedImpl()? Sorry. :)
https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... File mojo/public/js/connection.js (right): https://codereview.chromium.org/2020463002/diff/20001/mojo/public/js/connecti... mojo/public/js/connection.js:158: function createHandleForImpl(obj) { > Ah, I didn't notice that one! Then in order to make things more consistent, maybe this one could be named bindStubDerivedImpl()? > > Sorry. :) No problem. Done. After looking further into the existing bindImpl(), it seems quite odd from an API perspective. It essentially returns two values, the newly created "stub" object, and handle1 from the newly created pipe. The "stub" object is returned via a callback that is passed as a param to bindImpl(), whereas the handle is returned via a normal "return" statement. It could simply return an object as follows. return {handle: pipe.handle1, stub: stub}; Either way, when I find time for some more cleanups, I'll find all call sites of all bind*() public methods to possibly come up with a reduced and simpler set of bind*() methods.
Description was changed from ========== Mojo JS: Attempt to simplify bindTo* related code. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Mojo JS: Attempt to simplify bindTo* related code. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Mojo JS: Attempt to simplify bindTo* related code. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Mojo JS: Attempt to simplify bindTo* related code. Introducing a new bindStubDerivedImpl() method, which hides unnecessary details from client code (no need to manually create a message pipe, no need to deal with an intermediate "stub" object). CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2020463002/#ps60001 (title: "Rename")
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2020463002/#ps60001 (title: "Rename")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020463002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2020463002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Mojo JS: Attempt to simplify bindTo* related code. Introducing a new bindStubDerivedImpl() method, which hides unnecessary details from client code (no need to manually create a message pipe, no need to deal with an intermediate "stub" object). CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Mojo JS: Attempt to simplify bindTo* related code. Introducing a new bindStubDerivedImpl() method, which hides unnecessary details from client code (no need to manually create a message pipe, no need to deal with an intermediate "stub" object). CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2ee1c2e2053a4c186b94ecc70ca7026060b381ff Cr-Commit-Position: refs/heads/master@{#396600} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2ee1c2e2053a4c186b94ecc70ca7026060b381ff Cr-Commit-Position: refs/heads/master@{#396600} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
