Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1987813004:
MD Settings: About page, implementing update status. (Closed)
|
Created:
4 years, 7 months ago by dpapad Modified:
4 years, 7 months ago Reviewers:
tommycli 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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: About page, implementing update status.
Specifically updating the status icon and status message according
to incoming events from the browser. The various buttons
(restart/relaunch/check-for-updates) will be handled in follow up CL.
BUG=603625, 546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/19cd3d2e33edd3bacdc7acff4f758f3fc3e2a589
Cr-Commit-Position: refs/heads/master@{#395118}
Patch Set 1 : Adding test. #Patch Set 2 : Rebasing. #
Total comments: 22
Patch Set 3 : Addressing comments. #
Total comments: 4
Patch Set 4 : Addressing comments. #Patch Set 5 : Update test, after changing "=" to "$=" #
Messages
Total messages: 29 (13 generated)
Description was changed from ========== MD Settings: About page, implementing update status. BUG=603625,546841 ========== to ========== MD Settings: About page, implementing update status. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: About page, implementing update status. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, implementing update status. Specifically updating the status icon and status message according to incoming events from the browser. The various buttons (restart/relaunch/check-for-updates) buttons will be handled in follow up CL. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
dpapad@chromium.org changed reviewers: + tommycli@chromium.org
The states the UI can be are depicted in the following screenshots. Chromium/Chrome screenshots: http://imgur.com/a/A8ums ChromiumOS/ChromeOS screenshots: http://imgur.com/a/YF8ur
Description was changed from ========== MD Settings: About page, implementing update status. Specifically updating the status icon and status message according to incoming events from the browser. The various buttons (restart/relaunch/check-for-updates) buttons will be handled in follow up CL. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, implementing update status. Specifically updating the status icon and status message according to incoming events from the browser. The various buttons (restart/relaunch/check-for-updates) buttons will be handled in follow up CL. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: About page, implementing update status. Specifically updating the status icon and status message according to incoming events from the browser. The various buttons (restart/relaunch/check-for-updates) buttons will be handled in follow up CL. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, implementing update status. Specifically updating the status icon and status message according to incoming events from the browser. The various buttons (restart/relaunch/check-for-updates) will be handled in follow up CL. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dpapad: hopefully these comments help https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { Actually I would recommend: iron-icon[icon="settings:check-circle"] Although I know this seems a bit more fragile, it also means we don't need these extra classes. Then you could just get rid of getIconCssClass_ https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:43: iron-icon.error-icon { same here https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:62: src="[[getIconSrc_(currentUpdateStatusEvent_)]]"> Are some of these really a custom image that can't be consolidated within settings-icons? It's not a huge problem, but a bit annoying. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:93: shouldShowUpdateStatus_: function() { optional suggestion: maybe isUpdateStatusShown? (or isUpdateStatusHidden) https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:101: getUpdateStatusMessage_: function() { Arg... yeah this is complicated and i can't think of a better way to do it. My only suggestion: Maybe get rid of the messageId and channelDisplayName variables, and just directly do: return this.i18n('aboutUpdateCheckStarted') directly from there? It just gets rid of some state... And you could just return this.currentUpdateStatusEvent_.message if it falls through all the way to the end. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:142: case UpdateStatus.FAILED: return 'error-icon'; formatting nit: maybe put these returns on the separate line? So the FAILED is separated from the UPDATED and NEARLY_UPDATED? https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:146: default: return ''; nit: same here https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:156: case UpdateStatus.DISABLED_BY_ADMIN: return 'cr:domain'; formatting nit: same here
https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { On 2016/05/19 at 20:44:48, tommycli wrote: > Actually I would recommend: > iron-icon[icon="settings:check-circle"] > > Although I know this seems a bit more fragile, it also means we don't need these extra classes. > > Then you could just get rid of getIconCssClass_ I like your suggestion, but unfortunately it did not work. The reason is that "icon" is not an attribute, so CSS can't see it. Adding "reflectToAttribute: true" at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme... makes it work, but requires a Polymer change. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:43: iron-icon.error-icon { On 2016/05/19 at 20:44:48, tommycli wrote: > same here See previous response. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:62: src="[[getIconSrc_(currentUpdateStatusEvent_)]]"> On 2016/05/19 at 20:44:48, tommycli wrote: > Are some of these really a custom image that can't be consolidated within settings-icons? It's not a huge problem, but a bit annoying. src can only be null or chrome://resources/images/throbber_small.svg. Perhaps throbber_small.svg can be added as settings icon. If we end up using this throbber in more places might be worth figuring this out, but since this is currently used only here, I think is OK to defer. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:93: shouldShowUpdateStatus_: function() { On 2016/05/19 at 20:44:48, tommycli wrote: > optional suggestion: maybe isUpdateStatusShown? (or isUpdateStatusHidden) I find the suggested naming a bit backward compared to the logic. This method is called from the HTML to determine whether to show/hide a node. Naming it isFooShown/isFooHidden makes it sound like this method queries the UI to figure out if a node is currently shown/hidden. I believe current naming is more appropriate since it makes it clear that the UI queries the "model" to update itself accordingly. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:101: getUpdateStatusMessage_: function() { On 2016/05/19 at 20:44:48, tommycli wrote: > Arg... yeah this is complicated and i can't think of a better way to do it. > > My only suggestion: Maybe get rid of the messageId and channelDisplayName variables, and just directly do: return this.i18n('aboutUpdateCheckStarted') directly from there? It just gets rid of some state... > > And you could just return this.currentUpdateStatusEvent_.message if it falls through all the way to the end. Done. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:142: case UpdateStatus.FAILED: return 'error-icon'; On 2016/05/19 at 20:44:48, tommycli wrote: > formatting nit: maybe put these returns on the separate line? So the FAILED is separated from the UPDATED and NEARLY_UPDATED? Done. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:146: default: return ''; On 2016/05/19 at 20:44:48, tommycli wrote: > nit: same here Done. https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:156: case UpdateStatus.DISABLED_BY_ADMIN: return 'cr:domain'; On 2016/05/19 at 20:44:48, tommycli wrote: > formatting nit: same here Done.
https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { On 2016/05/19 21:31:04, dpapad wrote: > On 2016/05/19 at 20:44:48, tommycli wrote: > > Actually I would recommend: > > iron-icon[icon="settings:check-circle"] > > > > Although I know this seems a bit more fragile, it also means we don't need > these extra classes. > > > > Then you could just get rid of getIconCssClass_ > > I like your suggestion, but unfortunately it did not work. The reason is that > "icon" is not an attribute, so CSS can't see it. Adding "reflectToAttribute: > true" at > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme... > makes it work, but requires a Polymer change. there's already a pull for that, btw
https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { On 2016/05/19 22:23:21, Dan Beam wrote: > On 2016/05/19 21:31:04, dpapad wrote: > > On 2016/05/19 at 20:44:48, tommycli wrote: > > > Actually I would recommend: > > > iron-icon[icon="settings:check-circle"] > > > > > > Although I know this seems a bit more fragile, it also means we don't need > > these extra classes. > > > > > > Then you could just get rid of getIconCssClass_ > > > > I like your suggestion, but unfortunately it did not work. The reason is that > > "icon" is not an attribute, so CSS can't see it. Adding "reflectToAttribute: > > true" at > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme... > > makes it work, but requires a Polymer change. > > there's already a pull for that, btw https://github.com/PolymerElements/iron-icon/pull/69
https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { Thanks, just found it, https://github.com/PolymerElements/iron-icon/pull/69. I'll make a note to update this code once/if this lands, but I am reluctant adding a TODO, since we don't know if the PR will be LGTMed.
https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { On 2016/05/19 at 22:26:56, dpapad wrote: > Thanks, just found it, https://github.com/PolymerElements/iron-icon/pull/69. I'll make a note to update this code once/if this lands, but I am reluctant adding a TODO, since we don't know if the PR will be LGTMed. Hi, I think this should work. I did something identical in the people_page though I did use the icon$= binding. Maybe that would do the trick? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { On 2016/05/20 00:24:36, tommycli wrote: > On 2016/05/19 at 22:26:56, dpapad wrote: > > Thanks, just found it, https://github.com/PolymerElements/iron-icon/pull/69. > I'll make a note to update this code once/if this lands, but I am reluctant > adding a TODO, since we don't know if the PR will be LGTMed. > > Hi, I think this should work. I did something identical in the people_page > though I did use the icon$= binding. Maybe that would do the trick? > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... yes, $= uses setAttribute('thing', value), whereas = effectively does .thing = value
two more optional suggestions https://codereview.chromium.org/1987813004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/1987813004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.js:105: case UpdateStatus.NEARLY_UPDATED: optional nit: can we just do something like: <if expr="chromeos"> if (this.channelsDiffer_) return this.i18n('aboutUpgradeSuccessChannelSwitch'); </if> return this.i18n('aboutUpgradeRelaunch'); https://codereview.chromium.org/1987813004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.js:114: case UpdateStatus.UPDATING: optional nit: and same in this case, i think we can avoid using any variables.
https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/1987813004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:39: iron-icon.check-icon { Done, $= worked. https://codereview.chromium.org/1987813004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/1987813004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.js:105: case UpdateStatus.NEARLY_UPDATED: On 2016/05/20 at 00:30:45, tommycli wrote: > optional nit: can we just do something like: > > <if expr="chromeos"> > if (this.channelsDiffer_) > return this.i18n('aboutUpgradeSuccessChannelSwitch'); > </if> > return this.i18n('aboutUpgradeRelaunch'); Done. https://codereview.chromium.org/1987813004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/about_page/about_page.js:114: case UpdateStatus.UPDATING: On 2016/05/20 at 00:30:45, tommycli wrote: > optional nit: and same in this case, i think we can avoid using any variables. Done.
LGTM thanks!
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/1987813004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987813004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/1987813004/#ps140001 (title: "Update test, after changing "=" to "$="")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987813004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987813004/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: About page, implementing update status. Specifically updating the status icon and status message according to incoming events from the browser. The various buttons (restart/relaunch/check-for-updates) will be handled in follow up CL. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, implementing update status. Specifically updating the status icon and status message according to incoming events from the browser. The various buttons (restart/relaunch/check-for-updates) will be handled in follow up CL. BUG=603625,546841 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/19cd3d2e33edd3bacdc7acff4f758f3fc3e2a589 Cr-Commit-Position: refs/heads/master@{#395118} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/19cd3d2e33edd3bacdc7acff4f758f3fc3e2a589 Cr-Commit-Position: refs/heads/master@{#395118} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
