Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2280513002:
MD History: promote menu button to show clear browsing data in narrow mode (Closed)
|
Created:
4 years, 3 months ago by Dan Beam Modified:
4 years, 3 months ago CC:
chromium-reviews, asanka, dbeam+watch-polymer_chromium.org, dbeam+watch-elements_chromium.org, michaelpg+watch-polymer_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-downloads_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD History: promote menu button to show clear browsing data in narrow mode
Show this tooltip the first time a user goes into narrow mode with the new
history page.
R=tsergeant@chromium.org
BUG=598017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766
Committed: https://crrev.com/7db5f539aa9c828a4b1db716b14028c73e462608
Cr-Original-Commit-Position: refs/heads/master@{#419996}
Cr-Commit-Position: refs/heads/master@{#420095}
Patch Set 1 : asdf #
Total comments: 14
Patch Set 2 : !showingSearch_ #
Total comments: 4
Patch Set 3 : tsergeant@ review #Patch Set 4 : merge #
Total comments: 4
Patch Set 5 : go full vanilla #
Total comments: 2
Patch Set 6 : cr-menu-promo-shown #Patch Set 7 : merge #
Total comments: 7
Patch Set 8 : damn it #Depends on Patchset: Messages
Total messages: 69 (43 generated)
Description was changed from ========== MD History: promote menu button to show clear browsing data in narrow mode R=tsergeant@chromium.org BUG=598017 ========== to ========== MD History: promote menu button to show clear browsing data in narrow mode R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
https://codereview.chromium.org/2280513002/diff/60001/third_party/polymer/v1_... File third_party/polymer/v1_0/components-chromium/paper-tooltip/paper-tooltip-extracted.js (right): https://codereview.chromium.org/2280513002/diff/60001/third_party/polymer/v1_... third_party/polymer/v1_0/components-chromium/paper-tooltip/paper-tooltip-extracted.js:141: this.listen(this, 'mouseenter', 'hide'); this already landed in Polymer, I just don't really feel like rolling right before a branch https://github.com/PolymerElements/paper-tooltip/issues/85
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
screenshot on bug: https://bugs.chromium.org/p/chromium/issues/detail?id=598017#c7
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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Do you mind adding a UMA histogram/actions for promo shown and promo dismissed? I think it would be a good idea to keep an eye on the effectiveness of this tooltip, especially since it requires a user action to be dismissed. https://codereview.chromium.org/2280513002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/2280513002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.js:103: md_history.BrowserService.getInstance().menuPromoShown(); Looks like you'll need to add browser_service as a dependency for the closure compile https://codereview.chromium.org/2280513002/diff/60001/components/history_stri... File components/history_strings.grdp (right): https://codereview.chromium.org/2280513002/diff/60001/components/history_stri... components/history_strings.grdp:182: <message name="IDS_HISTORY_MENU_PROMO" desc="Text body of a promotional tooltip that shows up the first time a user run the Material Design History UI in narrow mode to help find 'Clear Browsing Data' and the side navigation."> Typo nit: "first time a user runs" https://codereview.chromium.org/2280513002/diff/60001/third_party/polymer/v1_... File third_party/polymer/v1_0/components-chromium/paper-tooltip/paper-tooltip-extracted.js (right): https://codereview.chromium.org/2280513002/diff/60001/third_party/polymer/v1_... third_party/polymer/v1_0/components-chromium/paper-tooltip/paper-tooltip-extracted.js:141: this.listen(this, 'mouseenter', 'hide'); On 2016/08/25 01:24:43, Dan Beam wrote: > this already landed in Polymer, I just don't really feel like rolling right > before a branch > https://github.com/PolymerElements/paper-tooltip/issues/85 Acknowledged. https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:105: <iron-icon icon="cr:clear" on-tap="onMenuPromoCloseTap_"> Should this be a paper-icon-button[-light]? Or just a native <button> with an <iron-icon> inside it? Something to indicate to screen readers (and everyone else, with `cursor: pointer`) that this is clickable. https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:50: computed: 'computeShowingMenuPromo_(showMenuPromo, narrow_)', I think you want to tie this to showMenu_ rather than narrow_. The two breakpoints are different in MD History. https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:77: this.fire('cr-menu-promo-shown'); This is potentially confusing, should this be cr-menu-promo-closed rather than -shown?
Description was changed from ========== MD History: promote menu button to show clear browsing data in narrow mode R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dbeam@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...
Patchset #2 (id:80001) has been deleted
On 2016/08/25 04:50:59, tsergeant wrote: > Do you mind adding a UMA histogram/actions for promo shown and promo dismissed? > I think it would be a good idea to keep an eye on the effectiveness of this > tooltip, especially since it requires a user action to be dismissed. So the things that we'd glean from this are: - what % of users saw this - what % of users clicked X right? That might be useful, but I'm not sure how we'd use it to change this UI. Maybe we'd axe the code if nobody ever saw it? What would we use as a baseline metric? Just showing the page? I'd rather do this separately as it's mildly controversial to me. WDYT? https://codereview.chromium.org/2280513002/diff/60001/chrome/browser/resource... File chrome/browser/resources/md_history/history_toolbar.js (right): https://codereview.chromium.org/2280513002/diff/60001/chrome/browser/resource... chrome/browser/resources/md_history/history_toolbar.js:103: md_history.BrowserService.getInstance().menuPromoShown(); On 2016/08/25 04:51:00, tsergeant wrote: > Looks like you'll need to add browser_service as a dependency for the closure > compile Done. https://codereview.chromium.org/2280513002/diff/60001/components/history_stri... File components/history_strings.grdp (right): https://codereview.chromium.org/2280513002/diff/60001/components/history_stri... components/history_strings.grdp:182: <message name="IDS_HISTORY_MENU_PROMO" desc="Text body of a promotional tooltip that shows up the first time a user run the Material Design History UI in narrow mode to help find 'Clear Browsing Data' and the side navigation."> On 2016/08/25 04:51:00, tsergeant wrote: > Typo nit: "first time a user runs" Done. https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:105: <iron-icon icon="cr:clear" on-tap="onMenuPromoCloseTap_"> On 2016/08/25 04:51:00, tsergeant wrote: > Should this be a paper-icon-button[-light]? Or just a native <button> with an > <iron-icon> inside it? Something to indicate to screen readers (and everyone > else, with `cursor: pointer`) that this is clickable. I asked Alan both of those questions: he wanted default cursor (not hand), and we agreed that having a ripple didn't make sense for vanishing UI. I'm happy to push harder on either if you'd like. Also note: we can set up the right aria-role and tabindex if you'd like separately from whether it's a <paper-*> thing. I'm really just trying not to add weight to the page. I was even considering not using <paper-tooltip> (and just a div with similar styles), and maybe even a unicode X like in downloads. I read that the toolbar currently accounts for like 7% of load, but feel these were a) micro optimizations, b) not adding much, though I think I am adding the first use of paper-tooltip but discarding much of the benefit with [manual-mode]. https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:50: computed: 'computeShowingMenuPromo_(showMenuPromo, narrow_)', On 2016/08/25 04:51:00, tsergeant wrote: > I think you want to tie this to showMenu_ rather than narrow_. The two > breakpoints are different in MD History. well, it's already in a <template is="dom-if" if="[[showMenu]]">, so that simplifies the code a bit https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:77: this.fire('cr-menu-promo-shown'); On 2016/08/25 04:51:00, tsergeant wrote: > This is potentially confusing, should this be cr-menu-promo-closed rather than > -shown? This didn't actually close it; it just told the parent what qualifies as "shown" and parent closes. But I changed the meaning of this to be more in line what you expect, I think.
and yes, I know that the current implementation does not notify open pages (I.e. open 2 pages, move one to narrow, switch other yo narrow and you'll get a second promo). I just don't think it's worth the code to observe the pref and notify open pages. it's an edge case and I don't care.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
As discussed separately, I'm happy to drop the metric for now. As for notifying other open pages, I also don't think it's important enough to show. https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:105: <iron-icon icon="cr:clear" on-tap="onMenuPromoCloseTap_"> On 2016/09/09 03:12:08, Dan Beam wrote: > On 2016/08/25 04:51:00, tsergeant wrote: > > Should this be a paper-icon-button[-light]? Or just a native <button> with an > > <iron-icon> inside it? Something to indicate to screen readers (and everyone > > else, with `cursor: pointer`) that this is clickable. > > I asked Alan both of those questions: he wanted default cursor (not hand), and > we agreed that having a ripple didn't make sense for vanishing UI. I'm happy to > push harder on either if you'd like. > > Also note: we can set up the right aria-role and tabindex if you'd like > separately from whether it's a <paper-*> thing. > > I'm really just trying not to add weight to the page. I was even considering > not using <paper-tooltip> (and just a div with similar styles), and maybe even a > unicode X like in downloads. > > I read that the toolbar currently accounts for like 7% of load, but feel these > were a) micro optimizations, b) not adding much, though I think I am adding the > first use of paper-tooltip but discarding much of the benefit with > [manual-mode]. Happy to go with this for the lightweight button. My main concern is for screenreader users. I think it's fine for the promo to appear not be in the document tab order. I don't think we need to draw people's attention to it like that. However, if a screenreader user does navigate into the promo tooltip, they should be able to understand what the icon is. That probably means role="button" and an aria-label, which is a bummer since we need to plumb through an extra string. If you'd like to test it out on canary with a screenreader first before you make those changes, I'd be okay with that. (And thanks for keeping performance in mind 😃) https://codereview.chromium.org/2280513002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2280513002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:248: registry->RegisterBooleanPref(prefs::kMdHistoryMenuPromoShown, false); Naive question from someone not very familiar with prefs: Will this sync? If not, does it take much effort to make this sync? https://codereview.chromium.org/2280513002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:108: title="[[titleIfNotShowMenuPromo_(menuLabel, showMenuPromo)]]"> Can you add `aria-label$="[[menuLabel]]"` so that the button is still meaningful to screenreaders while the promo is showing?
Patchset #3 (id:120001) has been deleted
hoping to screenreader test this soon https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:105: <iron-icon icon="cr:clear" on-tap="onMenuPromoCloseTap_"> On 2016/09/09 06:09:39, tsergeant wrote: > On 2016/09/09 03:12:08, Dan Beam wrote: > > On 2016/08/25 04:51:00, tsergeant wrote: > > > Should this be a paper-icon-button[-light]? Or just a native <button> with > an > > > <iron-icon> inside it? Something to indicate to screen readers (and everyone > > > else, with `cursor: pointer`) that this is clickable. > > > > I asked Alan both of those questions: he wanted default cursor (not hand), and > > we agreed that having a ripple didn't make sense for vanishing UI. I'm happy > to > > push harder on either if you'd like. > > > > Also note: we can set up the right aria-role and tabindex if you'd like > > separately from whether it's a <paper-*> thing. > > > > I'm really just trying not to add weight to the page. I was even considering > > not using <paper-tooltip> (and just a div with similar styles), and maybe even > a > > unicode X like in downloads. > > > > I read that the toolbar currently accounts for like 7% of load, but feel these > > were a) micro optimizations, b) not adding much, though I think I am adding > the > > first use of paper-tooltip but discarding much of the benefit with > > [manual-mode]. > > Happy to go with this for the lightweight button. > > My main concern is for screenreader users. I think it's fine for the promo to > appear not be in the document tab order. I don't think we need to draw people's > attention to it like that. However, if a screenreader user does navigate into > the promo tooltip, they should be able to understand what the icon is. That > probably means role="button" and an aria-label, which is a bummer since we need > to plumb through an extra string. > > If you'd like to test it out on canary with a screenreader first before you make > those changes, I'd be okay with that. > > (And thanks for keeping performance in mind 😃) Done. https://codereview.chromium.org/2280513002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2280513002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:248: registry->RegisterBooleanPref(prefs::kMdHistoryMenuPromoShown, false); On 2016/09/09 06:09:39, tsergeant wrote: > Naive question from someone not very familiar with prefs: Will this sync? probably wouldn't have > > If not, does it take much effort to make this sync? but now it does! (not much effort) https://codereview.chromium.org/2280513002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:108: title="[[titleIfNotShowMenuPromo_(menuLabel, showMenuPromo)]]"> On 2016/09/09 06:09:39, tsergeant wrote: > Can you add `aria-label$="[[menuLabel]]"` so that the button is still meaningful > to screenreaders while the promo is showing? Done.
The CQ bit was checked by dbeam@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dbeam@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The trip down the rabbit hole continues. https://codereview.chromium.org/2280513002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:78: paper-tooltip { This doesn't layout correctly in RTL -- the tooltip goes off the side of the screen, and the arrow points at the wrong thing. (Also, please ignore the fact that the (i) icon is on the wrong side of the screen in RTL. I'll fix that right now) https://codereview.chromium.org/2280513002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:117: role="button" tabindex="0" aria-label$="[[closeMenuPromo]]"> When I tab to the icon, I can't actually interact with it through the keyboard (space and enter do nothing). (I'm still okay with it not being tabbable)
axed both paper-tooltip and iron-icon they weren't paying their own costs. moved to fancy <div>/<button> fixes keyboard and positioning issue. it's possible we want to use aria-labelledby= to associate the tooltip with the menu button. but i don't really have a good screenreader setup to test this matters. https://codereview.chromium.org/2280513002/diff/160001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:78: paper-tooltip { On 2016/09/16 01:23:08, tsergeant wrote: > This doesn't layout correctly in RTL -- the tooltip goes off the side of the > screen, and the arrow points at the wrong thing. > > (Also, please ignore the fact that the (i) icon is on the wrong side of the > screen in RTL. I'll fix that right now) Done. https://codereview.chromium.org/2280513002/diff/160001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.html:117: role="button" tabindex="0" aria-label$="[[closeMenuPromo]]"> On 2016/09/16 01:23:08, tsergeant wrote: > When I tab to the icon, I can't actually interact with it through the keyboard > (space and enter do nothing). > > (I'm still okay with it not being tabbable) Done.
lgtm with one little fix below. New vanilla flavour seems a good amount simpler, thanks! https://codereview.chromium.org/2280513002/diff/180001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2280513002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:79: if (this.showMenu && this.showMenuPromo && !this.showingSearch_) { this.fire('cr-menu-promo-shown'); disappeared from here, which means the promo will never stop showing (very convenient for testing!)
The CQ bit was checked by dbeam@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2280513002/diff/180001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js (right): https://codereview.chromium.org/2280513002/diff/180001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_toolbar/cr_toolbar.js:79: if (this.showMenu && this.showMenuPromo && !this.showingSearch_) { On 2016/09/19 01:21:41, tsergeant wrote: > this.fire('cr-menu-promo-shown'); > > disappeared from here, which means the promo will never stop showing (very > convenient for testing!) Done.
The CQ bit was checked by dbeam@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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org Link to the patchset: https://codereview.chromium.org/2280513002/#ps220001 (title: "merge")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dbeam@chromium.org changed reviewers: + pam@chromium.org
+pam for chrome/browser/prefs
Owner LGTM. A couple of optional nits, no need for re-review. https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_downloads/crisper.js (right): https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_downloads/crisper.js:6791: observers: [ 'possiblyShowMenuPromo_(showMenu, showMenuPromo, showingSearch_)' ], Unless there's something unusual about how this file is parsed, the line length could be fixed with a CR after the [ https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_history/app.crisper.js (right): https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_history/app.crisper.js:6079: observers: [ 'possiblyShowMenuPromo_(showMenu, showMenuPromo, showingSearch_)' ], ditto https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_history/app.vulcanized.html (right): https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_history/app.vulcanized.html:2914: this blank line seems extraneous https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_history/app.vulcanized.html:3280: <cr-toolbar id="main-toolbar" page-name="$i18n{title}" clear-label="$i18n{clearSearch}" search-prompt="$i18n{searchPrompt}" hidden$="[[itemsSelected_]]" spinner-active="[[spinnerActive]]" show-menu="[[hasDrawer]]" show-menu-promo="[[showMenuPromo_]]" menu-label="$i18n{historyMenuButton}" menu-promo="$i18n{menuPromo}" close-menu-promo="$i18n{closeMenuPromo}" on-search-changed="onSearchChanged_" on-cr-menu-promo-shown="onMenuPromoShown_"> Ew. OK, it was already like this, and there are lots of other places this file could use reformatting. So no need to change it in this CL -- but ew. https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:185: on-cr-menu-promo-shown="onMenuPromoShown_"> Yay readability! :)
hey Pam, all of those are generated files (any crisper or vulcanized files are a flattened bundle of HTML imports, stylesheets, or scripts). we don't really control their formatting, a tool named uglifyjs does (at least for js)
https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/md_history/history_toolbar.html (right): https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/resourc... chrome/browser/resources/md_history/history_toolbar.html:185: on-cr-menu-promo-shown="onMenuPromoShown_"> On 2016/09/21 04:44:12, Pam (also PM for reviews) wrote: > Yay readability! :) except for this one :) which we made pretty
The CQ bit was checked by dbeam@chromium.org
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.
Description was changed from ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Cr-Commit-Position: refs/heads/master@{#419996} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Cr-Commit-Position: refs/heads/master@{#419996}
Message was sent while issue was closed.
Manually reverted this CL due to compile error on Google Chrome Win bot: https://codereview.chromium.org/2354193002/
Message was sent while issue was closed.
On 2016/09/21 09:06:39, hbos_chromium wrote: > Manually reverted this CL due to compile error on Google Chrome Win bot: > https://codereview.chromium.org/2354193002/ As hbos encountered some error, I've reverted this manually at https://codereview.chromium.org/2355233002/
Message was sent while issue was closed.
https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/prefs/b... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2280513002/diff/220001/chrome/browser/prefs/b... chrome/browser/prefs/browser_prefs.cc:245: #include "chrome/browser/ui/webui/md_history_ui.cc" dang
Message was sent while issue was closed.
Description was changed from ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Cr-Commit-Position: refs/heads/master@{#419996} ========== to ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Cr-Commit-Position: refs/heads/master@{#419996} ==========
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, pam@chromium.org Link to the patchset: https://codereview.chromium.org/2280513002/#ps240001 (title: "damn it")
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.
Description was changed from ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Cr-Commit-Position: refs/heads/master@{#419996} ========== to ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Cr-Commit-Position: refs/heads/master@{#419996} ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Cr-Commit-Position: refs/heads/master@{#419996} ========== to ========== MD History: promote menu button to show clear browsing data in narrow mode Show this tooltip the first time a user goes into narrow mode with the new history page. R=tsergeant@chromium.org BUG=598017 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2d6fa4fda96dd212548f8422cf589224eb305766 Committed: https://crrev.com/7db5f539aa9c828a4b1db716b14028c73e462608 Cr-Original-Commit-Position: refs/heads/master@{#419996} Cr-Commit-Position: refs/heads/master@{#420095} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7db5f539aa9c828a4b1db716b14028c73e462608 Cr-Commit-Position: refs/heads/master@{#420095} |
