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

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

This CL adds an initializer call for the Protocol Handlers handler so
that updates for the enabled/disabled state will be sent without getting
the whole list of protocol handlers.

BUG=676527
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3f92fd3d49f00789495bff44712a5dcb7fa1ff85
Cr-Commit-Position: refs/heads/master@{#440543}

[dschuyler, 2016-12-22 01:54:34]

Description was changed from

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

This CL adds an initializer call for the Protocol Handlers handler so
that updates for the enabled/disabled state will be sent without getting
the whole list of protocol handlers.

BUG=676527
==========

to

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

This CL adds an initializer call for the Protocol Handlers handler so
that updates for the enabled/disabled state will be sent without getting
the whole list of protocol handlers.

BUG=676527
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2016-12-22 01:54:36]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 01:54:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:213:
* 'setIgnoredProtocolHandler'.
I like the quotes because it helps me see them as messages used with .send() or
a webui listener.

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:224:
* because it doesn't send the list.
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.

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/protocol_handlers_handler.cc (right):

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/protocol_handlers_handler.cc:140:
SendHandlersEnabledValue();
I'd want to call AllowJavascript(); and UpdateHandlerList(); in this function
even if it were separate from HandleInitializeUpdates. The super-set part of
this call is the SendHandlersEnabledValue();.

[dschuyler, 2016-12-22 02:00:26]

dschuyler@chromium.org changed reviewers:
+ dpapad@chromium.org

[commit-bot: I haz the power, 2016-12-22 03:39:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 03:39:06]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:212:
* to 'setHandlersEnabled', 'setProtocolHandlers', and
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++.

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:213:
* 'setIgnoredProtocolHandler'.
On 2016/12/22 at 02:00:05, dschuyler wrote:
> I like the quotes because it helps me see them as messages used with .send()
or a webui listener.

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`).

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:224:
* because it doesn't send the list.
On 2016/12/22 at 02:00:05, dschuyler wrote:
> 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.

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]

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:212:
* to 'setHandlersEnabled', 'setProtocolHandlers', and
On 2016/12/22 19:39:02, dpapad wrote:
> 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++.

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

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:213:
* 'setIgnoredProtocolHandler'.
On 2016/12/22 19:39:02, dpapad wrote:
> On 2016/12/22 at 02:00:05, dschuyler wrote:
> > I like the quotes because it helps me see them as messages used with .send()
> or a webui listener.
> 
> 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`).

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

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:224:
* because it doesn't send the list.
On 2016/12/22 19:39:02, dpapad wrote:
> On 2016/12/22 at 02:00:05, dschuyler wrote:
> > 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.
> 
> 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?

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).

> 
> chrome.send('startObserving', [true /* sendInitialState */]);
> chrome.send('startObserving', [false /* sendInitialState */]);
> 
> and similarly merge the BrowserProxy wrappers to a single
> startObservingHandlers() method?

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]

LGTM, with nit.

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:213:
* 'setIgnoredProtocolHandler'.
On 2016/12/22 at 19:58:15, dschuyler wrote:
> On 2016/12/22 19:39:02, dpapad wrote:
> > On 2016/12/22 at 02:00:05, dschuyler wrote:
> > > I like the quotes because it helps me see them as messages used with
.send()
> > or a webui listener.
> > 
> > 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`).
> 
> Yeah, I've seen (and use) that for code. What about for strings?

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).

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:224:
* because it doesn't send the list.
> 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.

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:45:33]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 21:45:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-22 21:54:40]

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:213:
* 'setIgnoredProtocolHandler'.
On 2016/12/22 20:08:35, dpapad wrote:
> On 2016/12/22 at 19:58:15, dschuyler wrote:
> > On 2016/12/22 19:39:02, dpapad wrote:
> > > On 2016/12/22 at 02:00:05, dschuyler wrote:
> > > > I like the quotes because it helps me see them as messages used with
> .send()
> > > or a webui listener.
> > > 
> > > 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`).
> > 
> > Yeah, I've seen (and use) that for code. What about for strings?
> 
> 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).

Done.

https://codereview.chromium.org/2598893002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:224:
* because it doesn't send the list.
On 2016/12/22 20:08:35, dpapad wrote:
> > 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.
> 
> 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.

Sounds like a good thing to get into the style guide.

> 
> // 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

Let's talk about this more outside of this CL.

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

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.

[dpapad, 2016-12-22 21:59:16]

> 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.

Thanks. I think the latest comments and names are much more clear than before.

[commit-bot: I haz the power, 2016-12-22 22:57:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 22:57:48]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-22 23:32:08]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-12-22 23:32:09]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2598893002/#ps20001
(title: "review changes")

[commit-bot: I haz the power, 2016-12-22 23:32:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 23:39:04]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482449528920180,
"parent_rev": "339c88ac5366ab4c3ed94df28006e053f1fd8dce", "commit_rev":
"5cfb4fd181ab77f4adaaf225ffb5b101df060ade"}

[commit-bot: I haz the power, 2016-12-22 23:39:30]

Description was changed from

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

This CL adds an initializer call for the Protocol Handlers handler so
that updates for the enabled/disabled state will be sent without getting
the whole list of protocol handlers.

BUG=676527
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

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

This CL adds an initializer call for the Protocol Handlers handler so
that updates for the enabled/disabled state will be sent without getting
the whole list of protocol handlers.

BUG=676527
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2598893002
==========

[commit-bot: I haz the power, 2016-12-22 23:39:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-22 23:42:13]

Description was changed from

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

This CL adds an initializer call for the Protocol Handlers handler so
that updates for the enabled/disabled state will be sent without getting
the whole list of protocol handlers.

BUG=676527
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2598893002
==========

to

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

This CL adds an initializer call for the Protocol Handlers handler so
that updates for the enabled/disabled state will be sent without getting
the whole list of protocol handlers.

BUG=676527
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3f92fd3d49f00789495bff44712a5dcb7fa1ff85
Cr-Commit-Position: refs/heads/master@{#440543}
==========

[commit-bot: I haz the power, 2016-12-22 23:42:15]

Patchset 2 (id:??) landed as
https://crrev.com/3f92fd3d49f00789495bff44712a5dcb7fa1ff85
Cr-Commit-Position: refs/heads/master@{#440543}