Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2500513003:
Make setting's protocol handler use cr-action-menu instead of paper-item. (Closed)
|
Created:
4 years, 1 month ago by scottchen Modified:
4 years, 1 month ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake setting's protocol handler use cr-action-menu instead of paper-item.
BUG=603976
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/dcb5fc9af2549a795edf7e67d596e261364a948b
Cr-Commit-Position: refs/heads/master@{#432048}
Patch Set 1 #Patch Set 2 : add tests to protocol handlers #
Total comments: 30
Patch Set 3 : touch-up on protocol-handler code and tests per CR comments #
Total comments: 4
Patch Set 4 : protocol handler function to use private models instead of passed in #Patch Set 5 : fix return type of boolean function #Messages
Total messages: 30 (19 generated)
Description was changed from ========== Make setting's protocol handler use cr-action-menu instead of paper-item. BUG=603976 ========== to ========== Make setting's protocol handler use cr-action-menu instead of paper-item. BUG=603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
scottchen@chromium.org changed reviewers: + dpapad@chromium.org, dschuyler@chromium.org
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.html (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.html:62: hidden$="[[isDefault_(actionMenuModel_.index, I believe that hiding a menu option does not also make it non-focusable by default which would break keyboard navigation within the menu (up/down arrows). Can you verify if that's the case? Either way, I think we should also be disabling hidden options, instead of just hiding them. https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.html:63: actionMenuModel_.protocol.default_handler)]]" Can we simplify the isDefault_ data binding as follows? // HTML hidden$="isDefault(actionMenuModel_)" // JS isDefault_: function() { return this.actionMenuModel_.index == actionMenuModel_.protocol.default_handler; } https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.js (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.js:156: this.browserProxy.removeProtocolHandler(protocol, url); Nit: local vars protocol and url, don't seem to add much value. Can we simply inline |item.protocol| and |item.url| here? (Same for onDefaultTap_ above). https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/protocol_handlers_tests.js (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:17: * @type {ProtocolEntry} Shouldn't this be !Array<!ProtocolEntry> ? https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:79: return browserProxy.whenCalled('initializeProtocolHandlerList'). Would that also work? return browserProxy.whenCalled('initializeProtocolHandlerList').then(Polymer.dom.flush); https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:117: * @param {!string} button id of the button to test. "!" is implied for string,number,boolean, since those types are non-nullable by default for the compiler. You can remove. Also let's rename |button| to |buttonId|. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:121: function testButtonFlow(button, handler) { Given that that this file is testing "protocol handlers", and the term |handler| here is unrelated to that, I find it slightly confusing. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:127: .querySelectorAll('paper-icon-button'); Nit: Dot operator on previous line. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:138: return browserProxy.whenCalled(handler) Semicolon missing. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:139: }).then(function(args){ Space missing after "(args)" https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:141: * handler is expected to be called with arguments "/**" is used when adding JSDoc comments (@param, @return, @type etc). If you are simply adding an explanatory comment, "//" is more appropriate. Also per styleguide, comments do not have to be full sentences, but if they are, then they should use proper punctuation. // Handler is expected to be called with arguments as [protocol, url]. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:76: /** @private {!Array<ProtocolEntry>} */ If those arrays do not hold null values, annotation should be !ProtocolEntry and !UsbDeviceEntry few lines above. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:260: return Promise.resolve(); The prod version of initializeProtocolHandlerList does not seem to return a Promise.
https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.html (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.html:62: hidden$="[[isDefault_(actionMenuModel_.index, On 2016/11/14 18:26:20, dpapad wrote: > I believe that hiding a menu option does not also make it non-focusable by > default which would break keyboard navigation within the menu (up/down arrows). > Can you verify if that's the case? Either way, I think we should also be > disabling hidden options, instead of just hiding them. stuff with a computed style of display: none; (which [hidden] does if specific enough) wont be included in the focus/accessibility tree https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.html:63: actionMenuModel_.protocol.default_handler)]]" On 2016/11/14 18:26:20, dpapad wrote: > Can we simplify the isDefault_ data binding as follows? > > // HTML > hidden$="isDefault(actionMenuModel_)" i think dpapad@ meant hidden$="[[isDefault(actionMenuModel_)]]" > > // JS > isDefault_: function() { > return this.actionMenuModel_.index == > actionMenuModel_.protocol.default_handler; > }
https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.html (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.html:62: hidden$="[[isDefault_(actionMenuModel_.index, > stuff with a computed style of display: none; (which [hidden] does if specific enough) wont be included in the focus/accessibility tree Looked it up after I added this comment. For the cr-action-menu case I think it works because of this check https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_action....
https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.html (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.html:62: hidden$="[[isDefault_(actionMenuModel_.index, On 2016/11/14 18:26:20, dpapad wrote: > I believe that hiding a menu option does not also make it non-focusable by > default which would break keyboard navigation within the menu (up/down arrows). > Can you verify if that's the case? Either way, I think we should also be > disabling hidden options, instead of just hiding them. The keyboard navigation seems to be working as expected for me. https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.html:63: actionMenuModel_.protocol.default_handler)]]" On 2016/11/14 18:26:20, dpapad wrote: > Can we simplify the isDefault_ data binding as follows? > > // HTML > hidden$="isDefault(actionMenuModel_)" > > // JS > isDefault_: function() { > return this.actionMenuModel_.index == > actionMenuModel_.protocol.default_handler; > } Done. I created a new isModelDefault_ function because isDefault_ already exists for rendering of the "Default" text in the dom-repeat above. Tried reusing the same function in both places, but there wasn't a way to access "this model" inside a dom-repeat render loop. https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.js (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.js:156: this.browserProxy.removeProtocolHandler(protocol, url); On 2016/11/14 18:26:20, dpapad wrote: > Nit: local vars protocol and url, don't seem to add much value. Can we simply > inline |item.protocol| and |item.url| here? (Same for onDefaultTap_ above). Done. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/protocol_handlers_tests.js (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:17: * @type {ProtocolEntry} On 2016/11/14 18:26:20, dpapad wrote: > Shouldn't this be !Array<!ProtocolEntry> ? It should. Any idea why the closure_compiler didn't complain about this? https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:79: return browserProxy.whenCalled('initializeProtocolHandlerList'). On 2016/11/14 18:26:20, dpapad wrote: > Would that also work? > > return > browserProxy.whenCalled('initializeProtocolHandlerList').then(Polymer.dom.flush); this worked: browserProxy.whenCalled('initializeProtocolHandlerList').then(Polymer.dom.flush.bind(Polymer.dom)); https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:117: * @param {!string} button id of the button to test. On 2016/11/14 18:26:21, dpapad wrote: > "!" is implied for string,number,boolean, since those types are non-nullable by > default for the compiler. You can remove. > > Also let's rename |button| to |buttonId|. Done. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:121: function testButtonFlow(button, handler) { On 2016/11/14 18:26:21, dpapad wrote: > Given that that this file is testing "protocol handlers", and the term |handler| > here is unrelated to that, I find it slightly confusing. Acknowledged. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:127: .querySelectorAll('paper-icon-button'); On 2016/11/14 18:26:20, dpapad wrote: > Nit: Dot operator on previous line. Done. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:138: return browserProxy.whenCalled(handler) On 2016/11/14 18:26:20, dpapad wrote: > Semicolon missing. Done. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:139: }).then(function(args){ On 2016/11/14 18:26:21, dpapad wrote: > Space missing after "(args)" Done. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:141: * handler is expected to be called with arguments On 2016/11/14 18:26:21, dpapad wrote: > "/**" is used when adding JSDoc comments (@param, @return, @type etc). If you > are simply adding an explanatory comment, "//" is more appropriate. Also per > styleguide, comments do not have to be full sentences, but if they are, then > they should use proper punctuation. > > // Handler is expected to be called with arguments as [protocol, url]. Done. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:76: /** @private {!Array<ProtocolEntry>} */ On 2016/11/14 18:26:21, dpapad wrote: > If those arrays do not hold null values, annotation should be !ProtocolEntry and > !UsbDeviceEntry few lines above. Done. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js:260: return Promise.resolve(); On 2016/11/14 18:26:21, dpapad wrote: > The prod version of initializeProtocolHandlerList does not seem to return a > Promise. Done.
LGTM, with nit. https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/protocol_handlers_tests.js (right): https://codereview.chromium.org/2500513003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/protocol_handlers_tests.js:17: * @type {ProtocolEntry} On 2016/11/14 at 21:38:36, scottchen wrote: > On 2016/11/14 18:26:20, dpapad wrote: > > Shouldn't this be !Array<!ProtocolEntry> ? > > It should. Any idea why the closure_compiler didn't complain about this? Because none of our tests are compiled. https://codereview.chromium.org/2500513003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.js (right): https://codereview.chromium.org/2500513003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.js:160: isModelDefault_: function(model) { Do you need to declare the parameter here? Why not isModelDefault_: function() { return this.actionMenuModel_.index == this.actionMenuModel_.protocol.default_handler; }, There are plenty examples in the codebase, where in HTML we pass "deps" to a binding, but in JS we simply grab the "dep" from |this| instead of using the provided parameter. https://codereview.chromium.org/2500513003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.js:161: return model && (model.index == model.protocol.default_handler); Indentation off by 1.
https://codereview.chromium.org/2500513003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.js (right): https://codereview.chromium.org/2500513003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.js:160: isModelDefault_: function(model) { On 2016/11/14 21:54:09, dpapad wrote: > Do you need to declare the parameter here? Why not > > isModelDefault_: function() { > return this.actionMenuModel_.index == > this.actionMenuModel_.protocol.default_handler; > }, > > There are plenty examples in the codebase, where in HTML we pass "deps" to a > binding, but in JS we simply grab the "dep" from |this| instead of using the > provided parameter. Ah, this is the remnant of my attempt to merge the two isDefault methods. Fixed now.
https://codereview.chromium.org/2500513003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/protocol_handlers.js (right): https://codereview.chromium.org/2500513003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/protocol_handlers.js:161: return model && (model.index == model.protocol.default_handler); On 2016/11/14 21:54:09, dpapad wrote: > Indentation off by 1. Done.
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by scottchen@chromium.org
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/2500513003/#ps80001 (title: "fix return type of boolean function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make setting's protocol handler use cr-action-menu instead of paper-item. BUG=603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make setting's protocol handler use cr-action-menu instead of paper-item. BUG=603976 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/dcb5fc9af2549a795edf7e67d596e261364a948b Cr-Commit-Position: refs/heads/master@{#432048} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dcb5fc9af2549a795edf7e67d596e261364a948b Cr-Commit-Position: refs/heads/master@{#432048} |
