Site Settings Desktop: Implement protocol handler section.

Site Settings Desktop: Implement protocol handler section.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4fd43ac1df0f83c523da33c0e56bbd76d8b704d9
Cr-Commit-Position: refs/heads/master@{#404912}

[Finnur, 2016-07-08 13:09:27]

Description was changed from

==========
Site Settings Desktop: Implement protocol handler section.

BUG=614277, 543635
==========

to

==========
Site Settings Desktop: Implement protocol handler section.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-07-08 13:36:09]

Patchset #1 (id:1) has been deleted

[Finnur, 2016-07-08 13:40:54]

finnur@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-07-08 20:56:01]

caveat: i didn't review the copied code from HandlerOptionsHandler

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/protocol_handlers.html
(right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:1: <link
rel="import" href="chrome://resources/html/polymer.html">
cr/icons.html

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:35:
<template is="dom-repeat" items="[[protocol.handlers]]">
indent

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:54:
hidden$="[[isDefault_(index, protocol.default_handler)]]">
4-space indent

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:65:
</template>
</div> missing?

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/protocol_handlers.js
(right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.js:58:
this.MenuActions = MenuActions;
this should be in properties to use inside <template>

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.js:85: *
@param {boolean} The state to set.
these three are missing param names: @param {boolean} enabled The state to set.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.js:86: */
looks like these three should be @private: setHandlersEnabled_, etc

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:71:
AddSettingsPageUIHandler(new HandlerOptionsHandler());
OptionsHandler is a bit of a holdover from, well, Options. probably best to use
something precise and plural: ProtocolHandlersHandler

i think that's less confusing, after the necessary double-take :-)

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_handler_options_handler.cc
(right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_handler_options_handler.cc:112:
AllowJavascript();
there's some circular logic with having AllowJavascript here...
UpdateHandlerList can be called by Observe, whose observer is added in
OnJavascriptAllowed which is called after AllowJavascript.

It *works*, but I think the convention to call AllowJavascript in message
callback handlers (functions taking "ListValue* args") is more obvious.

The way you have it, it *seems* like you're always calling AllowJavascript *in
order to allow javascript* before you call JavaScript, which could be wrong.
What I see elsewhere is to call AllowJavascript in javascript message handlers
to *signal that JavaScript has been allowed*.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_handler_options_handler.cc:148:
GetProtocolHandlerRegistry()->enabled()));
indent off

[Finnur, 2016-07-10 00:19:03]

Patchset #3 (id:60001) has been deleted

[Finnur, 2016-07-10 00:40:17]

PTAL

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/protocol_handlers.html
(right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:1: <link
rel="import" href="chrome://resources/html/polymer.html">
I'm assuming you mean cr_elements/icons.html. ?

Done.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:35:
<template is="dom-repeat" items="[[protocol.handlers]]">
On 2016/07/08 20:56:00, michaelpg wrote:
> indent

Done.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:54:
hidden$="[[isDefault_(index, protocol.default_handler)]]">
On 2016/07/08 20:56:00, michaelpg wrote:
> 4-space indent

Done.

Hmm... looks like Rietveld freaked out because indentation changed in two
places... :)

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:65:
</template>
Good catch. Done.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/protocol_handlers.js
(right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.js:58:
this.MenuActions = MenuActions;
Done... I think. :)

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.js:85: *
@param {boolean} The state to set.
Good catch. Done.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/protocol_handlers.js:86: */
On 2016/07/08 20:56:00, michaelpg wrote:
> looks like these three should be @private: setHandlersEnabled_, etc

Done.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_ui.cc:71:
AddSettingsPageUIHandler(new HandlerOptionsHandler());
Yeah. That's a good suggestion.

Done.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_handler_options_handler.cc
(right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_handler_options_handler.cc:112:
AllowJavascript();
Done.

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_handler_options_handler.cc:148:
GetProtocolHandlerRegistry()->enabled()));
Wow. I'm impressed. Did you really catch this by eyeballing it? :)

Done.

[michaelpg, 2016-07-10 17:54:43]

lgtm

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_handler_options_handler.cc
(right):

https://codereview.chromium.org/2131953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_handler_options_handler.cc:148:
GetProtocolHandlerRegistry()->enabled()));
On 2016/07/10 00:40:17, Finnur (OOO summer vacation) wrote:
> Wow. I'm impressed. Did you really catch this by eyeballing it? :)
> 
> Done.

No, I only looked closely because it looked different from the function calls at
129 and 132.

https://codereview.chromium.org/2131953002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/2131953002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.html:15: <link
rel="import" href="/site_settings/protocol_handlers.html">
alphabetize

https://codereview.chromium.org/2131953002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/protocol_handlers.html
(right):

https://codereview.chromium.org/2131953002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/protocol_handlers.html:47:
<paper-menu-button>
import paper-item, paper-menu, paper-menu-button

https://codereview.chromium.org/2131953002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/protocol_handlers.js
(right):

https://codereview.chromium.org/2131953002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/protocol_handlers.js:53:
menuActions_: Object,
let's do: {
  type: Object,
  value: MenuActions,
  readOnly: true,
}

[Finnur, 2016-07-12 10:14:32]

All addressed. Thanks for the review. Checking in.

[Finnur, 2016-07-12 10:14:37]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-07-12 10:14:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2131953002/#ps140001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-07-12 10:15:05]

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-07-12 11:17:33]

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

[commit-bot: I haz the power, 2016-07-12 11:17:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Finnur, 2016-07-12 14:00:02]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-07-12 14:00:40]

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-07-12 16:03:46]

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

[commit-bot: I haz the power, 2016-07-12 16:03:47]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Finnur, 2016-07-12 17:27:55]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-07-12 17:28:54]

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-07-12 19:36:29]

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

[commit-bot: I haz the power, 2016-07-12 19:36:30]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Finnur, 2016-07-12 21:12:42]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-07-12 21:16:26]

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

[Finnur, 2016-07-12 22:12:32]

The CQ bit was unchecked by finnur@chromium.org

[Finnur, 2016-07-12 22:42:01]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-07-12 22:44:13]

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-07-13 02:27:43]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-07-13 02:29:54]

Description was changed from

==========
Site Settings Desktop: Implement protocol handler section.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Site Settings Desktop: Implement protocol handler section.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4fd43ac1df0f83c523da33c0e56bbd76d8b704d9
Cr-Commit-Position: refs/heads/master@{#404912}
==========

[commit-bot: I haz the power, 2016-07-13 02:29:56]

Patchset 6 (id:??) landed as
https://crrev.com/4fd43ac1df0f83c523da33c0e56bbd76d8b704d9
Cr-Commit-Position: refs/heads/master@{#404912}