Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1388353003:
[MD settings] adding DefaultBrowser settings page (Closed)
|
Created:
5 years, 2 months ago by dschuyler Modified:
5 years, 2 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] adding DefaultBrowser settings page
This CL adds the html/js code for the Material Design version
of the DefaultBrowser settings.
BUG=536295
Committed: https://crrev.com/33833436d4eb733736a9840df47ce44b2cc305fd
Cr-Commit-Position: refs/heads/master@{#353306}
Patch Set 1 #
Total comments: 26
Patch Set 2 : review nits #Patch Set 3 : changed to using i18n_behavior #Patch Set 4 : changing upstream branch #Patch Set 5 : moved upstream branch to master #
Total comments: 6
Patch Set 6 : review changes #Patch Set 7 : review changes #
Total comments: 1
Messages
Total messages: 22 (6 generated)
dschuyler@chromium.org changed reviewers: + stevenjb@chromium.org
lgtm https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/default_browser_page/default_browser_page.js (right): https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:27: * @type {Object|undefined} nit: @type not needed since it's just an Object https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:36: * @type {Object|undefined} nit: @type not needed https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:59: * @type {string} nit: @type not needed https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:67: * @type {boolean} nit: @type not needed https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:76: * @type {boolean} nit: @type not needed
https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/default_browser_page/default_browser_page.js (right): https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:27: * @type {Object|undefined} On 2015/10/08 18:15:39, stevenjb wrote: > nit: @type not needed since it's just an Object Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:36: * @type {Object|undefined} On 2015/10/08 18:15:39, stevenjb wrote: > nit: @type not needed Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:59: * @type {string} On 2015/10/08 18:15:39, stevenjb wrote: > nit: @type not needed Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:67: * @type {boolean} On 2015/10/08 18:15:39, stevenjb wrote: > nit: @type not needed Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:76: * @type {boolean} On 2015/10/08 18:15:39, stevenjb wrote: > nit: @type not needed Done.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1388353003/#ps20001 (title: "review nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1388353003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388353003/20001
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/default_browser_page/default_browser_page.js (right): https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:47: i18n_: { would you mind extracting this into a behavior? i.e. var LoadTimeDataI18n = { /** * @param {string} id The ID of the string to translate. * @param {...var_args} Placeholders required by the string ($1-9). * @return {string} A translated, substituted string. */ i18n: function(id, var_args) { return loadTimeData.getStringF.apply(loadTimeData, arguments); }, }; behaviors: [ LoadTimeDataI18n, ], https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:80: value: true, do you know that showButton_ = true is likely to happen a lot? if not, probably remove the default value: https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:96: chrome.send('SettingsDefaultBrowser.requestDefaultBrowserState'); shouldn't this all be done from ready? https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:108: * @param {boolean} isDefault Whether Chrome is currently the user's default browser. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:109: * @param {boolean} canBeDefault Whether Chrome can be the default browser on this system. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:119: } no curlies https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:119: } nit: ternary
The CQ bit was unchecked by dbeam@chromium.org
https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/default_browser_page/default_browser_page.js (right): https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:47: i18n_: { On 2015/10/08 18:42:42, Dan Beam wrote: > would you mind extracting this into a behavior? > > i.e. > > var LoadTimeDataI18n = { > /** > * @param {string} id The ID of the string to translate. > * @param {...var_args} Placeholders required by the string ($1-9). > * @return {string} A translated, substituted string. > */ > i18n: function(id, var_args) { > return loadTimeData.getStringF.apply(loadTimeData, arguments); > }, > }; > > behaviors: [ > LoadTimeDataI18n, > ], I'm actually working on that right now if you want to wait a few hours... I'm also composing an email so we can define a set of preferred patterns for l10n.
PTAL, this has been updated to use the i18n behavior function. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/default_browser_page/default_browser_page.js (right): https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:47: i18n_: { On 2015/10/08 18:42:42, Dan Beam wrote: > would you mind extracting this into a behavior? > > i.e. > > var LoadTimeDataI18n = { > /** > * @param {string} id The ID of the string to translate. > * @param {...var_args} Placeholders required by the string ($1-9). > * @return {string} A translated, substituted string. > */ > i18n: function(id, var_args) { > return loadTimeData.getStringF.apply(loadTimeData, arguments); > }, > }; > > behaviors: [ > LoadTimeDataI18n, > ], Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:47: i18n_: { On 2015/10/08 19:50:36, stevenjb wrote: > On 2015/10/08 18:42:42, Dan Beam wrote: > > would you mind extracting this into a behavior? > > > > i.e. > > > > var LoadTimeDataI18n = { > > /** > > * @param {string} id The ID of the string to translate. > > * @param {...var_args} Placeholders required by the string ($1-9). > > * @return {string} A translated, substituted string. > > */ > > i18n: function(id, var_args) { > > return loadTimeData.getStringF.apply(loadTimeData, arguments); > > }, > > }; > > > > behaviors: [ > > LoadTimeDataI18n, > > ], > > I'm actually working on that right now if you want to wait a few hours... I'm > also composing an email so we can define a set of preferred patterns for l10n. Acknowledged. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:80: value: true, On 2015/10/08 18:42:42, Dan Beam wrote: > do you know that showButton_ = true is likely to happen a lot? if not, probably > remove the default value: Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:96: chrome.send('SettingsDefaultBrowser.requestDefaultBrowserState'); On 2015/10/08 18:42:42, Dan Beam wrote: > shouldn't this all be done from ready? Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:108: * @param {boolean} isDefault On 2015/10/08 18:42:42, Dan Beam wrote: > Whether Chrome is currently the user's default browser. Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:109: * @param {boolean} canBeDefault On 2015/10/08 18:42:42, Dan Beam wrote: > Whether Chrome can be the default browser on this system. Done. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:119: } On 2015/10/08 18:42:42, Dan Beam wrote: > no curlies Acknowledged. https://codereview.chromium.org/1388353003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:119: } On 2015/10/08 18:42:42, Dan Beam wrote: > nit: ternary Done.
lgtm
https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/default_browser_page/default_browser_page.html (right): https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/default_browser_page/default_browser_page.html:16: <iron-icon icon="error" i18n-values="title:unableToSetDefaultBrowser" wait, wait does i18n-values work here but i18n-content="defaultBrowserMakeDefault" wouldn't? https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/default_browser_page/default_browser_page.js (right): https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:16: * </iron-animated-pages> why is this 3\s indented? https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:31: }, where is prefs used?
https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/default_browser_page/default_browser_page.html (right): https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/default_browser_page/default_browser_page.html:16: <iron-icon icon="error" i18n-values="title:unableToSetDefaultBrowser" On 2015/10/09 01:41:51, Dan Beam wrote: > wait, wait does i18n-values work here but > i18n-content="defaultBrowserMakeDefault" wouldn't? This string isn't present in the pak file yet. So it failing would not be immediately noticed. https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/default_browser_page/default_browser_page.js (right): https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:16: * </iron-animated-pages> On 2015/10/09 01:41:52, Dan Beam wrote: > why is this 3\s indented? Other examples in settings use that indentation. I'll change it to two. https://codereview.chromium.org/1388353003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/default_browser_page/default_browser_page.js:31: }, On 2015/10/09 01:41:52, Dan Beam wrote: > where is prefs used? It's not used. Done.
re lgtm :) https://codereview.chromium.org/1388353003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/default_browser_page/default_browser_page.html (right): https://codereview.chromium.org/1388353003/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/default_browser_page/default_browser_page.html:8: href="chrome://md-settings/settings_page/settings_page.css"> nit: you can probably just remove chrome://md-settings/ from these
lgtm
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1388353003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388353003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/33833436d4eb733736a9840df47ce44b2cc305fd Cr-Commit-Position: refs/heads/master@{#353306} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
