MD Settings: chrome.send() usage patterns discussion

[Demetrios Papadopoulos]

I have noticed about 3 different patterns in our MD Settings code with regards

to chrome.send(). I think it would be useful if we review them and come up with

one pattern that we all feel comfortable with.

1. No wrapping: Call chrome.send() directly from a Polymer UI element. Expose callbacks for the C++ to call by calling cr.define() within the attached() lifecycle method. Example reset_profile_dialog.js.
2. Wrapping variation1: Wrap all chrome.send() calls within a separate foo_private_api.js (which only has static methods. no constructor). Expose JS methods for C++ to call in the same way as 1 above. Example change_picture_private_api.js and change_picture.js.
   
3. Wrapping variation2: Wrap all chrome.send() calls within a separate foo_private_api.js (which only has static methods. no constructor). Expose JS methods for C++ to call within foo_private_api.js (as static methods). Pass delegate functions ony by one from Polymer UI component to foo_private_api.js and make the static methods delegate to them. Example passwords_private_api.js.

All of the patterns above have pros and cons, and there is no clear winner so far. How about we talk about it next week to settle down to a single approach (or a new one)?

I'll start by adding my suggestion to the mix.

Wrapping variation3: Make foo_private_api.js a real class (with a constructor), and pass the Polymer element itself as a delegate to it, see pseudocode below.

// my_custom_element.js
Polymer({
 apiWrapper_: new FooPrivateApi(this),
 ...
 doSomething_: function() { this.apiWrapper_.chromeDoSomething() },
 myDelegateMethod1: function() {...},
 myDelegateMethod2: function() {...},
});

// foo_private_api.js
var FooPrivateApi(delegate) {
  this.delegate_ = delegate; // callback delegate
};
FooPrivateApi.prototype.chromeDoSomething = function() {
  chrome.send('doSomething');
  // C++ will call myDelegateMethod1, then myDelegateMethod2.
};
// Called from C++
FooPrivateApi.prototype.myDelegateMethod1 = function(result) {
  this.myDelegateMethod1(result);
};
// Called from C++
FooPrivateApi.prototype.myDelegateMethod2 = function(result) {
  this.myDelegateMethod2(result);
};

Thank you,

Demetrios

[Demetrios Papadopoulos]

Small correction is pseudocode inlined below.

Oops, the last two methods should be as follows,

// Called from C++
FooPrivateApi.prototype.myDelegateMethod1 = function(result) {

  this.delegate_.myDelegateMethod1(result);

};
// Called from C++
FooPrivateApi.prototype.myDelegateMethod2 = function(result) {

  this.delegate_.myDelegateMethod2(result);

}; 

Thank you,

Demetrios

[Hector Carmona]

I like your idea about combining the private API with a polymer class. What if we expanded on that and expose the values we care about as settings without the callbacks:

Polymer({
  properties: {
    data: {
      type: Array,
      value: function() { return []; },
      notify: true
    },
  },

  loadData: function() {
    chrome.send('data1');
  },

  // Called from C++
  dataCallback(data) {
    this.data = data;
  },
});

This way the chrome.send and callbacks are encapsulated in one object. This can be replaced entirely for testing. Any consumer of this data can listen to the change of value for data in their own polymer object if they need to do something when the data changes.

[Tommy Li]

Hi all, I have some questions:

How is FooPrivateApi.prototype.myDelegateMethod1 exposed to the global namespace to be callable from C++? I ask because C++ can only call static methods right?

Is there a way we can avoid the forwarding methods? I ask because after adding the Closure annotation, the forwarding adds a lot of lines of code. The change_picture implementation avoids the boilerplate by the element directly setting a global... which is admittedly quite hacky heh.

[Hector Carmona]

My code is missing some of the implementation details in order to keep it short, but you're correct, we would need a global function exposed for the c++.

[Demetrios Papadopoulos]

@Tommy: Good question, I did not address that in my snippet. I am thinking something that would look as follows:

I was thinking something that would look like the example at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/resources/downloads/manager.js&l=233-253. Basically wrap every member method with a static method (that refers to the singleton instance), such that C++ can use it. In that case the Polymer element would not call the constructor directly, but it would just refer to the singleton instance.

On Tuesday, January 19, 2016 at 11:39:24 AM UTC-8, Tommy Li wrote:

[Steven Bennetts]

+finnur@ who is working on a CL that includes a chrome.send wrapper.

I agree that it would be great to use a consistent pattern where we wrap chrome.send.

I would very much like any wrapper to support callbacks so that we can emulate the behavior that a proper API would provide, e.g something similar to the CL listed for variation 2.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.
To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/71ee3508-1f23-41d9-9d97-eedd1503bd20%40chromium.org.

[Demetrios Papadopoulos]

Regarding having the wrapper to support callbacks +1. It seems to me from discussions with folks that we generally seem to agree that keeping the calls and callbacks close to each other makes code more readable. How about the following API convention?

1. If a single call to C++ is generating a single response, instead of passing a callback from the Polymer element to the wrapper, simply return a Promise from the wrapper to the Polymer element (the
   wrapper would be responsible for resolving that Promise when C++ calls back).
   
2. If a single call to C++ generates multiple responses, then pass the callback(s) from the Polymer element to the wrapper. An alternative would be that the wrapper returns an event target where events are fired as necessary, but I think this might be too much infrastructure, given that chrome.send is on its way out.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

[Demetrios Papadopoulos]

The JS-end of a Mojo call is using a Promise if a response is expected, or no return value if no response is expected. You can see an example at the WIP https://codereview.chromium.org/1576183003/diff/80001/chrome/browser/resources/plugins.js, (search for "getPluginsData()").

Yes, it would make it easier to switch later if the chrome.send was abstracted in a similar fashion. My motivation though to unify the chrome.send usage was not to make it look Mojo-like for later migration, but only to make it more consistent and readable (it just happens that the Promise based wrappers are also used by Mojo, because they are more readable).

On Tue, Jan 19, 2016 at 2:08 PM, Dave Schuyler <dsch...@google.com> wrote:

Demetrios, what is the JS-end of the Mojo system likely to look like?  Is there value it trying to make a wrapper that resembles using mojo (would it make it easier to switch later without being too cumbersome now)?

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/71ee3508-1f23-41d9-9d97-eedd1503bd20%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/d98896b2-96d4-4223-87bf-348ee255ce8f%40chromium.org.

[Hector Carmona]

How does Mojo handle multiple responses?

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/71ee3508-1f23-41d9-9d97-eedd1503bd20%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

[Demetrios Papadopoulos]

Multiple responses or C++ calling JS at will (not as a result of a previous JS->C++ call), are handled by exposing the JS to be called as a "Mojo service". The C++ handler has a "pageProxy" object that it can call at will, example at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc&l=166.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/71ee3508-1f23-41d9-9d97-eedd1503bd20%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/d98896b2-96d4-4223-87bf-348ee255ce8f%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/7c79e086-ec7f-4f42-9ddf-c92e494d5a86%40chromium.org.

[Tommy Li]

Although I like Promises, I wonder if they are too incompatible with chrome.send to be used?

Assuming you called:

  var promise1 = FooPrivateApi.doSomething();

  var promise2 = FooPrivateApi.doSomething();

FooPrivateApi would now have to store both promises. When the C++ calls JS, are the promises fulfilled FIFO or FILO?

Also, at least in the Change Picture and Sync APIs, a lot of the C++=>JS calls are called not responses, but are caused by observers on the C++ side...

I think Promises are good for call-and-response patterns, but at least in my usage, that's the exception rather than the rule.

--

How about the below? It's very similar to Demetrio's initial proposal.

// my_custom_element.js

Polymer({

 doSomething_: function() { this.apiWrapper_.chromeDoSomething() },

 myNativeMethod1: function(foo) {...},

 myNativeMethod2: function() {...},

 attached: function() {

   FooPrivateApi.nativeDelegate = this;

 },

 detached: function() {

   FooPrivateApi.nativeDelegate = null;

 },

});

// foo_private_api.js

FooPrivateApi.chromeDoSomething = function() {

  chrome.send('doSomething');

  // C++ will call myNativeMethod1, then myNativeMethod2.

};

/**

 * Defines Foo C++ => JS interface.

 * @public {{

 *   myNativeMethod1: function(string),

 *   myNativeMethod2: function(string, boolean),

 * }}

 */

FooPrivateApi.nativeDelegate = null;

// foo_handler.cc

CallJS("settings.FooPrivateApi.nativeDelegate.myNativeMethod1", args);

Tommy

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/71ee3508-1f23-41d9-9d97-eedd1503bd20%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/d98896b2-96d4-4223-87bf-348ee255ce8f%40chromium.org.

[Michael Giuffrida]

On Tue, Jan 19, 2016 at 3:01 PM Tommy Li <tomm...@chromium.org> wrote:

Although I like Promises, I wonder if they are too incompatible with chrome.send to be used?

Assuming you called:

  var promise1 = FooPrivateApi.doSomething();

  var promise2 = FooPrivateApi.doSomething();

FooPrivateApi would now have to store both promises. When the C++ calls JS, are the promises fulfilled FIFO or FILO?

Neither, we would associate each promise with an auto-incrementing ID in the JS, and pass that ID to the C++. When the C++ calls the JS, it will include that ID in its arguments, and the JS would resolve and then forget about that promise.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CAMWG5%3D73M%3Do-RstANuJsA8NF3Z%3DVz8EpDL6Npc2FTR1Z4re3tQ%40mail.gmail.com.

[Demetrios Papadopoulos]

Agree with Tommy that Promises are good for call-and-response patterns only (and not random calls resulting from C++ observers). For the case of 2 (or more) in-flight promises for the same chrome.send() call, yes it adds complexity since we need to keep track of both promises (more precisely keep track of the promise resolver function) and fire them in some determined order.

In the usage patters mentioned so far, there are two competing themes with regards to C++ to JS communication.

1. Pass the Polymer object as a delegate (in Tommy's latest suggestion that is 'nativeDelegate').
   
2. Pass individual methods from the Polymer to the wrapper, where the wrapper stores a reference to it.

I think choosing between 1 or 2 would move the discussion forward, no matter how all the remaining details will end up. I put together this CL that converts reset_page/ folder to use a wrapper (without addressing the corner case described above though). This approach is basically a polyfil of what Mojo does, summarizing here.

Create two classes, a BrowserProxy and a PageProxy. JS uses BrowserProxy (chrome.send wrapper) to make calls. C++ uses PageProxy to make calls to JS. This approach (with regards to C++ to JS) seems to fall between 1 and 2.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/71ee3508-1f23-41d9-9d97-eedd1503bd20%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

[Tommy Li]

Micheal: Oh that's clever.

[Tommy Li]

Looking at your CL, Demetrios, I think I like the "before" of reset_profile_dialog.js the best.

There's no encapsulation, and that's the only downside, but the pluses are:

• Idiot proof. It's really obvious what's going on. No boilerplate.
• Callbacks and chrome.send all in one file.
• Don't have to go to a separate reset_private_api.js file.
  

Maybe this whole foo_private_api.js encapsulation thing was a bad idea. Maybe the "stupid" and obvious way is the best way.

Tommy

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/71ee3508-1f23-41d9-9d97-eedd1503bd20%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/d98896b2-96d4-4223-87bf-348ee255ce8f%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/6889c9b7-1436-4332-a4f3-23c7f7688ed7%40chromium.org.

[Dan Beam]

(from the right account this time)

Some notes to make our conversation more productive:

I'd highly recommend using a diff/code review tool (codereview.chromium.org, github.com) to show code:

• it's meant to deal with code
  
• you can continually update it
  
• people can comment more easily inline
  
• you can more easily directly test/apply the patch to Chrome
  

My [only] requirements for our temporary chrome.send() abstraction:

• easy to test well (i.e. isolated, small number of injection points)
• simple enough to be worth migrating while we wait for Mojo

I've only been asking for this abstraction to make testing WebUI easier or possible (and hence, more prevalent).  This is not meant to be an interim solution that implements part of Mojo.

Update: I've made a ginormous flow chart of the JS <-> C++ communication to explain some situations and will be sharing it for all of you to scrutinize to bits soon.

-- Dan

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CAMWG5%3D6c9d0KfHCPZ6aM-hnirzyYm1qLHdhU%2BX_GmKxL8AuCfw%40mail.gmail.com.

[Rachel Blum]

On Tue, Jan 19, 2016 at 6:18 PM, Dan Beam <db...@chromium.org> wrote:

I've only been asking for this abstraction to make testing WebUI easier or possible (and hence, more prevalent). 

In that case, it'd be worth pointing out how individual proposals actually bring us closer to that goal, and how they differ in light of that goal. 

 Also: how high is the price we'll pay for each solution? (IOW: If they're all roughly equal in cost, a mojo polyfill makes sense. If they aren't, "cheap" is a major factor, given this is an interim step)

 

 

[Hector Carmona]

I wanted to post a proof of concept for my proposal: http://crrev.com/1609923003

Advantages of this approach: 

• chrome.send and callbacks are in the same file
• no element needs to know about or register for the callbacks
• updates its own polymer settings, so we get callbacks in any element that wants to listen and no callbacks on elements that just want data to be bound w/o effort
• supports updating from c++ without calling the chrome.send for those places we need to refresh the user's data
  
• testing is easy because the api and objects that care about the data are completely decoupled
• when it's time to replace chrome.send with something else, all we have to do is replace the type of instance from the HTML.
  

[Tommy Li]

Hi Hector,

I think I'm on board with your proposal. Your general pattern seems to be: 

<foo-element> embeds <foo-private-api-invisible-element> links itself to FooPrivateApi singleton right?

If that's the case, that seems the most legit of the proposals so far.

Tommy

[Finnur Thorarinsson]

I like Hector's proposal very much and the advantages that it brings (listed above). But I think we need to do something to support multiple elements using the API singleton. 

Right now I have an element implemented as per Hector's proposal. It works really well, but only because currently there is a *single* consumer of the API. 

I see this breaking down in at least two ways: 

Someone in the future adds another consumer to my API, perhaps unintentionally without realizing it by simply re-using my element. This would cause the |instance| variable to be overwritten in the singleton. But there's a bigger issue, because in some cases we will need to support multiple instances, for example:

In the near future I need to create a content_settings utility API that replaces my current usage of the |prefs| object (because we're supposed to go through the HostContentSettingsMap and not prefs, as per previous email to this group). I foresee those functions (being utility functions) will need to be re-usable between different Polymer elements so a single instance model won't cut it there.

I'll add some comments on Hector's proposal. 

[Michael Giuffrida]

Let's clarify terminology:

* MODEL: settings.PasswordsPrivateApi is a singleton API that manipulates data.

* CONTROLLER: <passwords-private-api> is a Polymer element that provides data (passwordList) to the view and updates the model upon events from the view.

* VIEW: passwords-page presents the data visually.

As a Polymer element, the controller lets us connect views to models via data binding. It also lets us explicitly query and manipulate the model in other "controllers" behind the views (passwords_and_forms_page.js).

To provide the model to *multiple* views, we can just use one controller and data bind it to multiple elements:

<passwords-private-api password-list="{{savedPasswords}}"></passwords-private-api> <some-passwords-element password-list="{{savedPasswords}}"></some-passwords-element> <another-passwords-element password-list="{{savedPasswords}}"></another-passwords-element> <single-password-view password="{{savedPasswords.0}}"></single-password-view>

Those elements could then bind "{{savedPasswords}}" to their children, and so on, all with only one <passwords-private-api> controller..

On Wed, Jan 27, 2016 at 4:44 AM Finnur Thorarinsson <fin...@chromium.org> wrote:

I like Hector's proposal very much and the advantages that it brings (listed above). But I think we need to do something to support multiple elements using the API singleton. 

Right now I have an element implemented as per Hector's proposal. It works really well, but only because currently there is a *single* consumer of the API. 

I see this breaking down in at least two ways: 

Someone in the future adds another consumer to my API, perhaps unintentionally without realizing it by simply re-using my element. This would cause the |instance| variable to be overwritten in the singleton.

Easily preventable by enforcing that the controller, <passwords-private-api>, be created at most once.

But *IF* it becomes unwieldy to only use one <passwords-private-api> (e.g., we would have to put it too high up the DOM tree, or bind "passwordsList" through a bunch of unrelated elements), THEN we can consider alternative controllers, like <settings-languages> which can be created multiple times but always refers to the same "settings-languages-singleton" so data binding works properly. This requires lots of trickery so I would advise against it unless really necessary -- and if we need this pattern in multiple APIs we should abstract it out into a behavior.

 

But there's a bigger issue, because in some cases we will need to support multiple instances, for example:

In the near future I need to create a content_settings utility API that replaces my current usage of the |prefs| object (because we're supposed to go through the HostContentSettingsMap and not prefs, as per previous email to this group). I foresee those functions (being utility functions) will need to be re-usable between different Polymer elements so a single instance model won't cut it there.

I'll comment on your comment in Hector's proposal, but: if different Polymer elements have *different* data and want to use the same API for utility functions, that's fine and that's not hard.

If the elements *share* the data (i.e. all the data comes from one source, be it in the C++ or the JS), then any update to one element needs to be reflected in all the other elements, so you don't really want to duplicate the model and you need to do something more intelligent. If that's what you foresee, let's work with a concrete example when it comes up.

 

I'll add some comments on Hector's proposal. 

On Wed, Jan 20, 2016 at 6:05 PM Tommy Li <tomm...@chromium.org> wrote:

Hi Hector,

I think I'm on board with your proposal. Your general pattern seems to be: 

<foo-element> embeds <foo-private-api-invisible-element> links itself to FooPrivateApi singleton right?

If that's the case, that seems the most legit of the proposals so far.

Tommy

On Tue, Jan 19, 2016 at 6:56 PM, Hector Carmona <hcar...@chromium.org> wrote:

I wanted to post a proof of concept for my proposal: http://crrev.com/1609923003

Advantages of this approach: 

• chrome.send and callbacks are in the same file
• no element needs to know about or register for the callbacks
• updates its own polymer settings, so we get callbacks in any element that wants to listen and no callbacks on elements that just want data to be bound w/o effort
• supports updating from c++ without calling the chrome.send for those places we need to refresh the user's data
  
• testing is easy because the api and objects that care about the data are completely decoupled
• when it's time to replace chrome.send with something else, all we have to do is replace the type of instance from the HTML.
  

On Tuesday, January 19, 2016 at 6:36:47 PM UTC-8, Rachel Blum wrote:

On Tue, Jan 19, 2016 at 6:18 PM, Dan Beam <db...@chromium.org> wrote:

I've only been asking for this abstraction to make testing WebUI easier or possible (and hence, more prevalent). 

In that case, it'd be worth pointing out how individual proposals actually bring us closer to that goal, and how they differ in light of that goal. 

 Also: how high is the price we'll pay for each solution? (IOW: If they're all roughly equal in cost, a mojo polyfill makes sense. If they aren't, "cheap" is a major factor, given this is an interim step)

 

 

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-setti...@chromium.org.
To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CAB2L5-bvRGnM_VbFm9Wk7Z6_W68pw%2ByGMg0yTdVzWDSH5aLesQ%40mail.gmail.com.

[Steven Bennetts]

Actual extensions APIs effectively exist as singletons per page. I would be strongly in favor of using a singleton model for any API "controller"  modules that we create in JS.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CACi5S_3bTB5BQoQ_h3BvWmSbu7vGDj8rqN1UQswWEe%2Bj33aZ2A%40mail.gmail.com.

[Dan Beam]

Hey y'all,

Here's what I propose to handle C++ <-> JS communication more robustly:

​

(Sorry, I skipped the UML lesson at school.  Download this file and import into draw.io if you want to poke at this.)

Here's an example of JS -> C++ in code: https://gist.github.com/danbeam/e2b2ae865a47af6c80e3

The salient bits of my proposal:

1. route any calls to global services through an @interface
• this should be sufficiently generic for our needs (any @return type or @params allowed)
  
• easily swappable for more isolated, reliable testing
2. allow responses to requests by returning Promises
• discourages circular references
• The Web Platform Endorsed Way(TM) to handle async responses
• extension APIs (that only take callbacks) can be easily polyfilled
3. handle global response mapping from C++ <-> JS transparently with cr.sendWithPromise()
• this uses 1 global method and a response map
• alleviates exposing tons of globals for each type of reponse
• bakes per-request responses into the platform
• macros could handle request ID routing sneakily on the C++ if desired

What do y'all think?

-- Dan

[Rachel Blum]

FWIW, in favor:

* Saves us from having to ship promises to C++

* type safety right around the corner.

* allows us to switch in mojo in a fairly straightforward way.

* API is a pure controller, the underlying model is still separated out into cr.Send/extension API

Two questions:

* TODO around aborting in-flight requests. Isn't promise.abort still open for debate? (https://github.com/promises-aplus/cancellation-spec/issues/14)

* How do we handle update notifications from the C++ side? (I.e. 'onFrobberChanged')

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CANpe7K1B2yp-bL_aJP9GYSGe0yc5N-S0qgmR8%3Db9trqUsqgfQw%40mail.gmail.com.

[Dan Beam]

On Thu, Jan 28, 2016 at 2:21 PM, Rachel Blum <gr...@chromium.org> wrote:

FWIW, in favor:

* Saves us from having to ship promises to C++

* type safety right around the corner.

* allows us to switch in mojo in a fairly straightforward way.

* API is a pure controller, the underlying model is still separated out into cr.Send/extension API

Two questions:

* TODO around aborting in-flight requests. Isn't promise.abort still open for debate? (https://github.com/promises-aplus/cancellation-spec/issues/14)

We set a configurable timeout in JS and allow C++ to reject the held Promise faster.  @interface returned Promises can detect these rejections with catch() as per their needs.

* How do we handle update notifications from the C++ side? (I.e. 'onFrobberChanged')

That's the "Response to a specific request -> no" branch from C++.  If a purely C++ -> JS notification happens (that's not request-specific), a global JS name must be exposed to handle this.  That global will need find it's own way to route the data (i.e. getting a reference from the DOM, finding a specific element to modify).

[Rachel Blum]

I was thinking about observers, specifically. I.e. "onUpdated". I suppose a global + route_id will do. I'd love addObserver/registerCallback, but not sure it's worth the effort.

[Demetrios Papadopoulos]

FWIW, there is this related helper function in cr.js . It is being used in one place from md-settings (guessing from the early days?), see example. Maybe we can revive that helper method and avoid the global callbacks all together? Not sure how much work is required on the c++ side though to make use of that helper.

[Rachel Blum]

Seems like using it is fairly trivial as well, see your example's C++ counterpart. Dan, mind if your proposal included this?

[Dan Beam]

On Thu, Jan 28, 2016 at 5:00 PM, Rachel Blum <gr...@chromium.org> wrote:

Seems like using it is fairly trivial as well, see your example's C++ counterpart. Dan, mind if your proposal included this?

On Thu, Jan 28, 2016 at 4:55 PM, Demetrios Papadopoulos <dpa...@chromium.org> wrote:

FWIW, there is this related helper function in cr.js . It is being used in one place from md-settings (guessing from the early days?), see example. Maybe we can revive that helper method and avoid the global callbacks all together? Not sure how much work is required on the c++ side though to make use of that helper.

I originally thought of this, but cr.addWebUIListener() seems more generic; SGTM :).

I added web_ui()->NotifyObservers() to my chart as the future C++ equivalent (when we write it). (new file)

FWIW: disallowing C++ to execute [basically arbitrary] JS while also cutting down on the number of global names exposed in JS (and making the source of updates clearer) sounds like a net win.

-- Dan

[Demetrios Papadopoulos]

Regarding cr.addWebUIListener, I am noticing that there is no equivalent removeWebUIListener, which I think is needed. For example when a sub-section that had registered a listener is closed, the listener should be removed.

If so, I suggest modifying addWebUIListener to return a value (numeric ID maybe?), that can be used to later remove the listener. Unregistering listeners by passing the listener function itself is very error prone (I've seen this happening a few times). Better explained with an example

// Example1: Registered listener is NOT removed.

addListener('some-event', myFunction.bind(this));

...

removeListener('some-event', myFunction.bind(this)); 

The two calls above use a different function reference to each call, so removeEventListener() never finds the listener to be removed.

// Exmaple2: Registered listener removed, but not intuitive.

var myListener = myFunction.bind(this);

addListener('some-event', myListener);

...

removeListener('some-event', myListener);

I think the more robust approach is the following

// Example 3: Return an ID from addEventListener, used to later remove the listener.

var listenerId = addEventListener('some-event', function() {...});

...

removeListener('some-event', listenerId);

Thank you,

Demetrios

[Dan Beam]

On Fri, Jan 29, 2016 at 9:26 AM, Demetrios Papadopoulos <dpa...@chromium.org> wrote:

Regarding cr.addWebUIListener, I am noticing that there is no equivalent removeWebUIListener, which I think is needed. For example when a sub-section that had registered a listener is closed, the listener should be removed.

Agreed; we can add it when the first consumer needs to unlisten.

 

If so, I suggest modifying addWebUIListener to return a value (numeric ID maybe?), that can be used to later remove the listener. Unregistering listeners by passing the listener function itself is very error prone (I've seen this happening a few times). Better explained with an example

// Example1: Registered listener is NOT removed.

addListener('some-event', myFunction.bind(this));

...

removeListener('some-event', myFunction.bind(this)); 

The two calls above use a different function reference to each call, so removeEventListener() never finds the listener to be removed.

We could assert() it's found, but that makes the dynamic addWebUIListener() case more arduous (have to track whether your class is actually listening).

 

// Exmaple2: Registered listener removed, but not intuitive.

var myListener = myFunction.bind(this);

addListener('some-event', myListener);

...

removeListener('some-event', myListener);

I think the more robust approach is the following

// Example 3: Return an ID from addEventListener, used to later remove the listener.

var listenerId = addEventListener('some-event', function() {...});

...

removeListener('some-event', listenerId);

Both the bound paradigm (that WebUI and Polymer already use a bit) and ^ this seem fine to me.

-- Dan

[Michael Giuffrida]

One sad face for cr.addWebUIListener is we lose the type safety we get from using extension events. Compare:

    chrome.apiPrivate.onFooChanged.addListener(bar);

with:

    cr.addWebUIListener('foo-chagned', bar);

Closure and JavaScript have no problem with the latter, so we lose some automated bug "coverage" here.

Is there any way to codify these strings so they can't be misspelled in JavaScript/C++, or left behind in JavaScript when the event is deleted from the C++ handler?

On Thu, Jan 28, 2016 at 4:55 PM Demetrios Papadopoulos <dpa...@chromium.org> wrote:

FWIW, there is this related helper function in cr.js . It is being used in one place from md-settings (guessing from the early days?), see example. Maybe we can revive that helper method and avoid the global callbacks all together? Not sure how much work is required on the c++ side though to make use of that helper.

On Thu, Jan 28, 2016 at 4:50 PM, Rachel Blum <gr...@chromium.org> wrote:

I was thinking about observers, specifically. I.e. "onUpdated". I suppose a global + route_id will do. I'd love addObserver/registerCallback, but not sure it's worth the effort.

On Thu, Jan 28, 2016 at 4:46 PM, Dan Beam <db...@chromium.org> wrote:

On Thu, Jan 28, 2016 at 2:21 PM, Rachel Blum <gr...@chromium.org> wrote:

FWIW, in favor:

* Saves us from having to ship promises to C++

* type safety right around the corner.

* allows us to switch in mojo in a fairly straightforward way.

* API is a pure controller, the underlying model is still separated out into cr.Send/extension API

Two questions:

* TODO around aborting in-flight requests. Isn't promise.abort still open for debate? (https://github.com/promises-aplus/cancellation-spec/issues/14)

We set a configurable timeout in JS and allow C++ to reject the held Promise faster.  @interface returned Promises can detect these rejections with catch() as per their needs.

* How do we handle update notifications from the C++ side? (I.e. 'onFrobberChanged')

That's the "Response to a specific request -> no" branch from C++.  If a purely C++ -> JS notification happens (that's not request-specific), a global JS name must be exposed to handle this.  That global will need find it's own way to route the data (i.e. getting a reference from the DOM, finding a specific element to modify).

-- Dan

 

On Wed, Jan 27, 2016 at 7:50 PM, Dan Beam <db...@chromium.org> wrote:

Hey y'all,

Here's what I propose to handle C++ <-> JS communication more robustly:

​

[Rachel Blum]

On Fri, Feb 19, 2016 at 8:25 PM, Michael Giuffrida <mich...@chromium.org> wrote:

Is there any way to codify these strings so they can't be misspelled in JavaScript/C++, or left behind in JavaScript when the event is deleted from the C++ handler?

First thing that comes to mind is an enum, which can be kept in sync via mojo. (More or less). 

[Demetrios Papadopoulos]

There is no intention to migrate any existing private APIs to cr.addWebUIListener. The latter is meant to be used where chrome.send was already being used.

 

Regarding type-safety, see this example where the callback method is annotated with the expected type, such that the compiler can do its job. This  is no different than adding a normal listener for the 'cilck' event, and manually type annotating the callback with "@ param {!MouseEvent}".

Regarding the event names, we could put them in an JS enum and add a comment ("These strings should match the C++ constants at file foo.h"), but repeating some strings across C++ and JS is not much different than our previous approach of calling JS functions by name from C++, and manually ensuring that the names match, except that we traded function names for event names.

(The benefit of this was though, that only one global method webUIListenerCallback is exposed from JS now).

Regarding typos prevention/detection, I have added this TODO for the case where a C++ triggered event ends up firing no listeners because no listener was registered for this particular event, which would help catch typos during development.

On Fri, Feb 19, 2016 at 8:25 PM, Michael Giuffrida <mich...@chromium.org> wrote:

-- Dan

 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CAB2L5-bvRGnM_VbFm9Wk7Z6_W68pw%2ByGMg0yTdVzWDSH5aLesQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CACi5S_3bTB5BQoQ_h3BvWmSbu7vGDj8rqN1UQswWEe%2Bj33aZ2A%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

[Rachel Blum]

On Mon, Feb 22, 2016 at 10:45 AM, Demetrios Papadopoulos <dpa...@chromium.org> wrote:

but repeating some strings across C++ and JS is not much different than our previous approach of calling JS functions by name from C++

Can't we generate the enums from the IDL?

[Demetrios Papadopoulos]

There are no IDLs corresponding to a C++ handler, so not sure which IDLs are you referring to. Only private APIs have associated IDLs AFAIK.

[Rachel Blum]

Sorry - _once we move to Mojo_. That's the missing part :) Right now, we might need to manually duplicate. (Although I wonder if we can abuse the IDL generator for this even now)

In general, if we need to maintain a common set of names across JS & C++, it should be generated from a common file if possible.

[Demetrios Papadopoulos]

Ah got it now.

Once we move to Mojo, there is no need for any string replication. In the Mojo world, there are two abstractions, a BrowserProxy and a PageProxy. C++ calls the methods on the PageProxy instance to trigger JS code, JS calls methods on a BrowserProxy instance to trigger C++ methods, so I don't see why we would need to replicate any strings.

[Michael Giuffrida]

I like the use of cr.sendWithPromise and thought this matter was resolved. But from this CL, it looks like the strategy we are using now has an extra component: 

1. when your object/elements need to use cr.addWebUIListener, just call cr.addWebUIListener directly, as discussed

2. when your object/element needs to use chrome.send or cr.sendWithPromise, create a "proxy" for that element and wrap the calls in that proxy

With the rationale being:

1. cr.addWebUIListener works fine in tests

2. cr.sendWithPromise and chrome.send don't work well in tests

Why the distinction? If we are already comfortable having our tests "manually" trigger WebUI events, why should we write a standalone proxy every time we need to call chrome.send/cr.sendWithPromise?

Or, if we feel the need to wrap cr.sendWithPromise, why do the same arguments not apply to cr.addWebUIListener?

From dpapad's description:

> 1) The browser proxy intentionally has no state, it simply wraps every single

> chrome.send() call with a type annotated version. It has no reference back to

> any Polymer element that is using the proxy.

The browser proxy wouldn't need a state for add/removeWebUIListenerEither, because (just like cr.sendwithPromise) this state is handled in cr.js itself.

> 2) The proxy does not handle any events, and does not expose and global method

> that is called from C++.

See response to 1).

> 3) Individual Polymer elements are responsible for calling cr.addWebUIListener

> (and removeWebUIListener if needed).

Yes, but they're also responsible for calling browserProxy.doFoo() when they could instead be responsible for calling chrome.send('doFoo').

 

Thank you,

Demetrios

-- Dan

 

​

[Demetrios Papadopoulos]

There was a lot (!! I mean a LOT :>) of discussion at https://codereview.chromium.org/1666623006 which is actually the CL that first uses that pattern (already landed). The CL you are referring to is just continuing that work.

Some answers inline. Also maybe it will be more efficient if we (me, you, Dan and anyone else who is interested) talk about this during our weekly sync (or during another eng-only meeting).

On Mon, Feb 22, 2016 at 12:44 PM, Michael Giuffrida <mich...@chromium.org> wrote:

I like the use of cr.sendWithPromise and thought this matter was resolved. But from this CL, it looks like the strategy we are using now has an extra component: 

1. when your object/elements need to use cr.addWebUIListener, just call cr.addWebUIListener directly, as discussed 

2. when your object/element needs to use chrome.send or cr.sendWithPromise, create a "proxy" for that element and wrap the calls in that proxy

Are you suggesting to simply wrap the cr.addWebUIListener('foo', myCallback) call as follows?

SomeBrowserProxy.prototype.addFooListener = function(myCallback) {

  return cr.addWebUIListener('foo', myCallback);

}); 

I think this is reasonable and can be done, although I don't see a big upside one way or the other.

With the rationale being:

1. cr.addWebUIListener works fine in tests

2. cr.sendWithPromise and chrome.send don't work well in tests

Why the distinction? If we are already comfortable having our tests "manually" trigger WebUI events, why should we write a standalone proxy every time we need to call chrome.send/cr.sendWithPromise?

 

See this example of how a "test" browser proxy is implemented and compare this with the alternative of overriding chrome.send with test_api.js#registerMessageCallback.

 

 

Thank you,

Demetrios

-- Dan

 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CAB2L5-bvRGnM_VbFm9Wk7Z6_W68pw%2ByGMg0yTdVzWDSH5aLesQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-settings/CACi5S_3bTB5BQoQ_h3BvWmSbu7vGDj8rqN1UQswWEe%2Bj33aZ2A%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "Chromium Settings" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-settings+unsubscribe@chromium.org.

To post to this group, send email to chromium...@chromium.org.