[MD settings] lighter weight initializer for protocol handler enabled messages

chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js

Index: chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
diff --git a/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js b/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
index b510502f3d3b8d3c31bb95c674bb3e6a3cd2c649..b4f46190841aaed3718b2945340f993b8f59ff02 100644
--- a/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
+++ b/chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
@@ -209,11 +209,23 @@ cr.define('settings', function() {
 
     /**
      * Initializes the protocol handler list. List is returned through JS calls
-     * to setHandlersEnabled, setProtocolHandlers & setIgnoredProtocolHandlers.
+     * to 'setHandlersEnabled', 'setProtocolHandlers', and

[dpapad, 2016/12/22 19:39:02]

Nit (optional): Looking at the C++ handler, setProtocolHandlers and
setIgnoredProtocolHandler are always called back to back. Perhaps those could be
merged into a single callback? I think it would make the code more
understandable, because having separate messages makes me think as a reader that
they can be called independently from C++.

[dschuyler, 2016/12/22 19:58:15]

Seems reasonable - let's look at that further in a different CL.

+     * 'setIgnoredProtocolHandler'.

[dschuyler, 2016/12/22 02:00:05]

I like the quotes because it helps me see them as messages used with .send() or
a webui listener.

[dpapad, 2016/12/22 19:39:02]

Nit: I think is more common in Chromium comments to use "|foo|" or "|foo()|" to
indicate that part of a comment refers to code (the equivalent of Markdown's
`foo`).

[dschuyler, 2016/12/22 19:58:15]

Yeah, I've seen (and use) that for code. What about for strings?

[dpapad, 2016/12/22 20:08:35]

The current comment phrasing makes it sound that "setHandlersEnabled" (etc) are
JS function calls. I guess they are WebUI event names instead, and we use
addWebUIListener() to listen. So let's rephrase the comment to make it clear
that those are WebUI event names (and using quotes for eventNames seems fine to
me).

[dschuyler, 2016/12/22 21:54:39]

Done.

      */
     initializeProtocolHandlerList: function() {},
 
     /**
+     * Initializes the protocol handler so that updates are sent. The
+     * 'setHandlersEnabled' update will be sent immediately and updates may
+     * follow.
+     *
+     * There's no need to call both initializeProtocolHandlerList
+     * and initializeProtocolHandlerUpdates. The *Updates version is lighter
+     * because it doesn't send the list.

[dschuyler, 2016/12/22 02:00:05]

I could easily re-arrange this so that they are independent and both would be
called in some cases. I see pluses and minuses with making them independent or
making one a super-set of the other. 

[dpapad, 2016/12/22 19:39:02]

IIUC initializeProtocolHandlerList and initializeProtocolHandlerUpdates are both
causing the C++ to start observing handler changes, except that one sends an
initial update immediately, and the other one does not. Would it make sense to
merge them and add a parameter?

chrome.send('startObserving', [true /* sendInitialState */]);
chrome.send('startObserving', [false /* sendInitialState */]);

and similarly merge the BrowserProxy wrappers to a single
startObservingHandlers() method?

[dschuyler, 2016/12/22 19:58:15]

It's not that one sends the initial state and one doesn't. There are different
things that can be observed (each with its own initial state). That could be a
series of booleans I suppose (thought that doesn't sound good to me right off).
That looks like it's an alternative way to do it. I'm not excited about using a
bool - it seems more cumbersome to read.

[dpapad, 2016/12/22 20:08:35]

Ack.

If there are two bools needed, then my understanding is incorrect, and yes I
agree that passing bools is less readable. I still think that the
"initializeFoo" naming is a bit vague (does something get initialized in JS, in
C++, in both?).

I think we've talked about this before (in similar context), where I prefer the
following pattern.

// Gets the current state of foo, within a promise callback. No events fire yet.
getFoo(): Promise<Foo>

// Observes foo from now on, by triggering some 'fooChanged' WebUI event.
observeFoo(): void

I am not suggesting to do this here, just mentioning it to explain why the
current API is a bit vague (to me).

[dschuyler, 2016/12/22 21:54:39]

Sounds like a good thing to get into the style guide.
Let's talk about this more outside of this CL.
Using 'observe' instead of 'initialize' makes sense. I've made that change.

I also changed the call that listens to all state changes to be the shorter name
observeProtocolHandlers (this listens to all state changes). So now, just the
specialized call has a longer name of observeProtocolHandlersEnabledState -
which will get just the state of enabled/disabled.

+     */
+    initializeProtocolHandlerUpdates: function() {},
+
+    /**
      * Enables or disables the ability for sites to ask to become the default
      * protocol handlers.
      * @param {boolean} enabled Whether sites can ask to become default.
@@ -358,6 +370,10 @@ cr.define('settings', function() {
       chrome.send('initializeProtocolHandlerList');
     },
 
+    initializeProtocolHandlerUpdates: function() {
+      chrome.send('initializeProtocolHandlerUpdates');
+    },
+
     /** @override */
     setProtocolHandlerDefault: function(enabled) {
       chrome.send('setHandlersEnabled', [enabled]);