Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2011703002:
MD Settings: About page, hooking up channel switcher dialog. (Closed)
|
Created:
4 years, 7 months ago by dpapad Modified:
4 years, 6 months ago Reviewers:
dschuyler CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@about_regulatory_info Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: About page, hooking up channel switcher dialog.
Also disabling "Change channel" button according to whether
the user is allowed to change release channels.
BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/4988151e7932cc1e368250d91cd474f4947ff4bc
Cr-Commit-Position: refs/heads/master@{#396916}
Patch Set 1 : Nit. #
Total comments: 17
Patch Set 2 : Address comments. #Patch Set 3 : Resolving conflicts in about_page.html #Patch Set 4 : Indentation nits, after merging. #
Messages
Total messages: 31 (22 generated)
Description was changed from ========== MD Settings: About page, channel switcher dialog, WIP. BUG=546841 ========== to ========== MD Settings: About page, channel switcher dialog, WIP. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #9 (id:160001) has been deleted
Patchset #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== MD Settings: About page, channel switcher dialog, WIP. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, hooking up channel switcher dialog. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: About page, hooking up channel switcher dialog. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, hooking up channel switcher dialog. Also disabling "Change channel" button according to whether the user should be allowed to change release channels. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: About page, hooking up channel switcher dialog. Also disabling "Change channel" button according to whether the user should be allowed to change release channels. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, hooking up channel switcher dialog. Also disabling "Change channel" button according to whether the user is allowed to change release channels. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #3 (id:220001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #1 (id:180001) has been deleted
dpapad@chromium.org changed reviewers: + dschuyler@chromium.org
https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.html:78: [[getUpdateStatusMessage_(currentUpdateStatusEvent_,targetChannel_)]] Please wrap line (and/or add a space after the comma). https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.html:87: hidden="[[!shouldShowRelaunch_(currentUpdateStatusEvent_,targetChannel_)]]" Please wrap line (and/or add a space after the comma). https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.html:94: hidden="[[!shouldShowRelaunchAndPowerwash_(currentUpdateStatusEvent_,targetChannel_)]]" Please wrap line and add a space after the comma. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.js:59: }.bind(this)); Just a suggestion: I've sometimes had trouble with a listener added before attached(). If a change to this value is fine prior to being attached, then this is fine in ready(). If you're not sure or find a problem, this may be better to have in attached(). https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.html:5: <link rel="import" href="/icons.html"> This will include the settings/icons.html and the cr:domain below comes from /src/ui/webui/resources/cr_elements/icons.html i.e. I think this import should change to <link rel="import" href="chrome://resources/cr_elements/icons.html"> https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.html:9: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html"> Sort above paper-button. (move this up a line). https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.html:38: $i18n{aboutChangeChannel}</paper-button> Please indent the $18n two more spaces -or- move </paper-button> to the next line. (I vote for moving the closing tag to its own line). https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/detailed_build_info.js (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.js:61: this.async(function() { Please add a comment above this.async to explain why we need to wait. Maybe: // Async to wait for dialog to appear in the dom.
https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.html:78: [[getUpdateStatusMessage_(currentUpdateStatusEvent_,targetChannel_)]] On 2016/05/27 at 22:16:21, dschuyler wrote: > Please wrap line > (and/or add a space after the comma). Done. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.html:87: hidden="[[!shouldShowRelaunch_(currentUpdateStatusEvent_,targetChannel_)]]" On 2016/05/27 at 22:16:21, dschuyler wrote: > Please wrap line > (and/or add a space after the comma). Done. Regarding breaking line: Do we have a rule about 80 columns in HTML? I did not find any mention at https://google.github.io/styleguide/htmlcssguide.xml. Personally I find single-line Polymer binding declaration a bit more readable, especially when the binding is within quotes, mostly because I am not used to having opening and closing quotes in different lines in HTML. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.html:94: hidden="[[!shouldShowRelaunchAndPowerwash_(currentUpdateStatusEvent_,targetChannel_)]]" On 2016/05/27 at 22:16:21, dschuyler wrote: > Please wrap line and add a space after the comma. Done. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.js:59: }.bind(this)); On 2016/05/27 at 22:16:21, dschuyler wrote: > Just a suggestion: I've sometimes had trouble > with a listener added before attached(). > If a change to this value is fine prior to > being attached, then this is fine in ready(). > If you're not sure or find a problem, > this may be better to have in attached(). Moved it to attached. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.html:5: <link rel="import" href="/icons.html"> On 2016/05/27 at 22:16:21, dschuyler wrote: > This will include the settings/icons.html > and the cr:domain below comes from > /src/ui/webui/resources/cr_elements/icons.html > > i.e. I think this import should change to > <link rel="import" href="chrome://resources/cr_elements/icons.html"> Done. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.html:9: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html"> On 2016/05/27 at 22:16:21, dschuyler wrote: > Sort above paper-button. > (move this up a line). Done. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.html:38: $i18n{aboutChangeChannel}</paper-button> On 2016/05/27 at 22:16:21, dschuyler wrote: > Please indent the $18n two more spaces -or- > move </paper-button> to the next line. > (I vote for moving the closing tag to its > own line). Done. https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/detailed_build_info.js (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/detailed_build_info.js:61: this.async(function() { On 2016/05/27 at 22:16:21, dschuyler wrote: > Please add a comment above this.async to > explain why we need to wait. Maybe: > // Async to wait for dialog to appear in the dom. Done.
lgtm https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2011703002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.html:87: hidden="[[!shouldShowRelaunch_(currentUpdateStatusEvent_,targetChannel_)]]" On 2016/05/27 23:32:17, dpapad wrote: > On 2016/05/27 at 22:16:21, dschuyler wrote: > > Please wrap line > > (and/or add a space after the comma). > > Done. > > Regarding breaking line: Do we have a rule about 80 columns in HTML? I did not > find any mention at https://google.github.io/styleguide/htmlcssguide.xml. It's at https://www.chromium.org/developers/web-development-style-guide We make an exception for <import>. Import lines can go on past 80 cols as needed.
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011703002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011703002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2011703002/#ps300001 (title: "Indentation nits, after merging.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011703002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011703002/300001
Message was sent while issue was closed.
Committed patchset #4 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: About page, hooking up channel switcher dialog. Also disabling "Change channel" button according to whether the user is allowed to change release channels. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, hooking up channel switcher dialog. Also disabling "Change channel" button according to whether the user is allowed to change release channels. BUG=546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/4988151e7932cc1e368250d91cd474f4947ff4bc Cr-Commit-Position: refs/heads/master@{#396916} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4988151e7932cc1e368250d91cd474f4947ff4bc Cr-Commit-Position: refs/heads/master@{#396916} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
