|
|
DescriptionInactive toast ux changes
BUG=717091
Review-Url: https://codereview.chromium.org/2904823002
Cr-Commit-Position: refs/heads/master@{#495849}
Committed: https://chromium.googlesource.com/chromium/src/+/c4f03f032136d23585d10951d6b855b40c83431c
Patch Set 1 : use real experiment code #
Total comments: 5
Patch Set 2 : minor rename (x -> horizontal) #
Total comments: 27
Patch Set 3 : review round #
Total comments: 30
Patch Set 4 : cleanup #
Total comments: 43
Patch Set 5 : review round, rename files #Patch Set 6 : remove the need for a font_list #
Total comments: 10
Patch Set 7 : switch to vector icons. Simplify new button class as a function #
Total comments: 39
Patch Set 8 : Review / style fixes #
Total comments: 5
Patch Set 9 : rebase, comments #
Total comments: 28
Patch Set 10 : review round #Patch Set 11 : added TODOs. #Patch Set 12 : move run_loop_ to a unique_ptr #
Total comments: 1
Messages
Total messages: 87 (33 generated)
Description was changed from ========== Inactive toast ux changes BUG=717091 ========== to ========== Inactive toast ux changes BUG=717091 ==========
skare@google.com changed reviewers: + grt@google.com, nikunjb@chromium.org
skare@google.com changed reviewers: + skare@google.com
sending as fyi -- will need to integrate grt's/nikunjb's changes
grt@chromium.org changed reviewers: + grt@chromium.org - grt@google.com
Hi Travis. How are things progressing here? Are you able to patch in https://codereview.chromium.org/2889323004/ and https://codereview.chromium.org/2933043002/ locally so that you can continue work on this? Let me know if you need any help on that front.
On 2017/06/13 07:19:15, grt (UTC plus 2) wrote: > Hi Travis. How are things progressing here? Are you able to patch in > https://codereview.chromium.org/2889323004/ and > https://codereview.chromium.org/2933043002/ locally so that you can continue > work on this? Let me know if you need any help on that front. Those changes have landed, so you can freely develop on trunk now.
fyi, new snapshot that resolves the TODOs for calling Experiments.
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:346: installer::ExperimentStorage storage; nit: make this a member variable of the class so that the underlying mutex isn't created/destroyed multiple times.
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:293: experiment.AssignGroup(flavor_); Installer is already Assigning group, so it may be redundant to reassign this here. Maybe a DCHECK if flavor_ and experiment.group() is different. AssignGroup will reset the state to kGroupAssigned which is probably not intended here.
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:293: experiment.AssignGroup(flavor_); On 2017/06/22 16:52:55, nikunjb1 wrote: > Installer is already Assigning group, so it may be redundant to reassign this > here. > Maybe a DCHECK if flavor_ and experiment.group() is different. > > AssignGroup will reset the state to kGroupAssigned which is probably not > intended here. I'll just remove the call if it's not needed - thanks! https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:346: installer::ExperimentStorage storage; On 2017/06/22 11:30:32, grt (UTC plus 2) wrote: > nit: make this a member variable of the class so that the underlying mutex isn't > created/destroyed multiple times. Done.
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:293: experiment.AssignGroup(flavor_); On 2017/06/22 17:07:13, skare_ wrote: > On 2017/06/22 16:52:55, nikunjb1 wrote: > > Installer is already Assigning group, so it may be redundant to reassign this > > here. > > Maybe a DCHECK if flavor_ and experiment.group() is different. > > > > AssignGroup will reset the state to kGroupAssigned which is probably not > > intended here. > > I'll just remove the call if it's not needed - thanks! Barring strange cases like user running regedit and cleaning their registry key, i think AssignGroup is not needed here. (And in these cases the state and inactive days would also be corrupted, so don't think it is worth continuing). So may be not calling AssignGroup here is best.
skare@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review changes in: chrome/browser/chrome_browser_main.cc chrome/browser/ui/views/try_chrome_dialog_view.h chrome/browser/ui/views/try_chrome_dialog_view.cc (or please reject if you're busy, thanks!)
looking pretty good. comments below. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1562: if (answer == TryChromeDialogView::NOT_NOW) { since the NOT_NOW and !OPEN_CHROME branches are the same, combine them like so: if (answer != TryChromeDialogView::OPEN_CHROME) return chrome::RESULT_CODE_NORMAL_EXIT_CANCEL; StartupBrowserCreator::WelcomeBackPage welcome_page = TryChromeDialogView::GetWelcomePageForFlavor(try_chrome_int); .... https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1567: if (welcome_page != StartupBrowserCreator::WelcomeBackPage::kNone) { nit: omit braces here https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: don't update copyright dates when modifying existing files https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:46: const unsigned int kToastWidth = 360; to the extent possible, please use constexpr rather than const for all of these compile-time constants. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:66: }; how about putting the welcome back page type in here, too? https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:68: static const ExperimentVariations kExperiments[] = { no "static" in the unnamed namespace https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:147: : flavor_(flavor), popup_(NULL), result_(COUNT) {} nit: NULL -> nullptr throughout https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:158: views::ImageView* icon = new views::ImageView(); naked pointers scare me. can this become: auto icon = base::MakeUnique<views::ImageView>(); and the line 222 becomes: layout->AddView(icon.release()); similarly for other allocated objects? https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:289: installer::ExperimentStorage storage; remove this line https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.h:20: #include "url/gurl.h" unused? https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.h:57: // |flavor| selects what strings to present and what controls are shown. flavor -> group throughout for consistency with the terminology used in installer::ExperimentMetrics and installer::Experiment. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.h:66: static StartupBrowserCreator::WelcomeBackPage GetWelcomePageForFlavor( wdyt of either having the WelcomeBackPage type being an out param of Show() or it being somehow represented in the Result (e.g., OPEN_NORMAL, OPEN_WIN10_WELCOME, OPEN_STANDARD_WELCOME)? https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:821: // Experimental. Shows a dialog asking the user to try chrome. This flag is to revert me too! https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:229: extern const char kUserExperimentUx[]; revert! ;-)
https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1562: if (answer == TryChromeDialogView::NOT_NOW) { On 2017/06/22 20:34:03, grt (UTC plus 2) wrote: > since the NOT_NOW and !OPEN_CHROME branches are the same, combine them like so: > if (answer != TryChromeDialogView::OPEN_CHROME) > return chrome::RESULT_CODE_NORMAL_EXIT_CANCEL; > StartupBrowserCreator::WelcomeBackPage welcome_page = > TryChromeDialogView::GetWelcomePageForFlavor(try_chrome_int); > .... Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1567: if (welcome_page != StartupBrowserCreator::WelcomeBackPage::kNone) { On 2017/06/22 20:34:03, grt (UTC plus 2) wrote: > nit: omit braces here Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > nit: don't update copyright dates when modifying existing files Done - apologies, originally had this as a sibling file and then ported it in to just replace the preexisting toast... missed this. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:66: }; On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > how about putting the welcome back page type in here, too? Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:68: static const ExperimentVariations kExperiments[] = { On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > no "static" in the unnamed namespace Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:147: : flavor_(flavor), popup_(NULL), result_(COUNT) {} On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > nit: NULL -> nullptr throughout Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:158: views::ImageView* icon = new views::ImageView(); On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > naked pointers scare me. can this become: > auto icon = base::MakeUnique<views::ImageView>(); > and the line 222 becomes: > layout->AddView(icon.release()); > > similarly for other allocated objects? done (left popup_ as it was but can reevaluate) https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.cc:289: installer::ExperimentStorage storage; On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > remove this line Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.h:20: #include "url/gurl.h" On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.h:57: // |flavor| selects what strings to present and what controls are shown. On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > flavor -> group throughout for consistency with the terminology used in > installer::ExperimentMetrics and installer::Experiment. Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/try_chrome_dialog_view.h:66: static StartupBrowserCreator::WelcomeBackPage GetWelcomePageForFlavor( On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > wdyt of either having the WelcomeBackPage type being an out param of Show() or > it being somehow represented in the Result (e.g., OPEN_NORMAL, > OPEN_WIN10_WELCOME, OPEN_STANDARD_WELCOME)? moved to being an out param and removed GetWelcomePageForFlavor(). also happy to have it enumerated in Result. https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:821: // Experimental. Shows a dialog asking the user to try chrome. This flag is to On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > revert me too! Done. https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:229: extern const char kUserExperimentUx[]; On 2017/06/22 20:34:04, grt (UTC plus 2) wrote: > revert! ;-) yep, had this question in email. We'll go with the existing flag. Thanks!
Patchset #1 (id:1) has been deleted
plz upload the new patch set for review. thanks!
On 2017/06/23 08:44:44, grt (UTC plus 2) wrote: > plz upload the new patch set for review. thanks! uploaded
lgtm with nits https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:10: #include "base/macros.h" remove https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:14: #include "chrome/browser/process_singleton.h" unused? https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:15: #include "chrome/browser/ui/startup/startup_browser_creator.h" nit: remove -- already included in .h https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:16: #include "chrome/common/url_constants.h" unused? https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:17: #include "chrome/grit/chromium_strings.h" i think this is unused now https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:21: #include "chrome/installer/util/experiment_storage.h" nit: remove -- already included in .h https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:22: #include "components/strings/grit/components_strings.h" i think this is unused now, too https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:24: #include "ui/aura/window.h" unused https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:25: #include "ui/aura/window_tree_host.h" unused https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:37: #include "ui/views/controls/separator.h" unused https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:39: #include "ui/views/layout/layout_provider.h" unused? https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:40: #include "ui/views/style/platform_style.h" unused? https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:41: #include "ui/views/style/typography.h" unused? https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:365: experiment.SetState(installer::ExperimentMetrics::kSelectedClose); #include "chrome/installer/util/experiment_metrics" for this https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:67: static StartupBrowserCreator::WelcomeBackPage GetWelcomePageForGroup( remove this declaration since the function is now gone.
The CQ bit was checked by skare@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...
https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:10: #include "base/macros.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > remove Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:14: #include "chrome/browser/process_singleton.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:15: #include "chrome/browser/ui/startup/startup_browser_creator.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > nit: remove -- already included in .h Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:16: #include "chrome/common/url_constants.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:17: #include "chrome/grit/chromium_strings.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > i think this is unused now Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:21: #include "chrome/installer/util/experiment_storage.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > nit: remove -- already included in .h Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:22: #include "components/strings/grit/components_strings.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > i think this is unused now, too Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:24: #include "ui/aura/window.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:25: #include "ui/aura/window_tree_host.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:37: #include "ui/views/controls/separator.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:39: #include "ui/views/layout/layout_provider.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:40: #include "ui/views/style/platform_style.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:41: #include "ui/views/style/typography.h" On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:365: experiment.SetState(installer::ExperimentMetrics::kSelectedClose); On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > #include "chrome/installer/util/experiment_metrics" for this Done. https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:67: static StartupBrowserCreator::WelcomeBackPage GetWelcomePageForGroup( On 2017/06/23 13:33:38, grt (UTC plus 2) wrote: > remove this declaration since the function is now gone. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
lgtm
https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... chrome/app/theme/theme_resources.grd:219: <structure type="chrome_scaled_image" name="IDR_INACTIVE_TOAST_CLOSE_X" file="win/inactive_toast/toast_close.png" /> It isn't particularly clear what "CLOSE_X" means. Maybe CLOSE_ICON? https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:48: enum ButtonTags { BT_NONE, BT_CLOSE_BUTTON, BT_OK_BUTTON, BT_NO_THANKS_BUTTON }; Why do you need BT_NONE? And make it enum class so that you don't need the prefix BT_. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:52: int heading_id; These need descriptions. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:92: class Win10StyleButton : public views::LabelButton { I think this would be clearer, and less code, if you have a function for this, e.g. CreateWin10StyleButton(...); Also, it isn't particularly clear what false means for affirmitive. By that I mean how the !affirmitive is used. I think an enum would be clearer here. That is: enum TryChromeButtonType { OPEN_CHROME, NO_THANKS }. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:181: constexpr int inset_top = 10; Where do these values come from? We are trying to enforce consistent dialog sizes. See LayoutProvider and it's various functions. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:215: columns->AddPaddingColumn(0, 12 - inset_left); Where does the 12 come from? Same comment about using LayoutProvider. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:253: body_text->SetAutoColorReadabilityEnabled(false); Why are you turning off readability? https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:260: if (!kExperiments[group_].has_no_thanks_button) { no {} https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:285: layout->Layout(root_view); Why do you need this layout? I would expect it after the call to SetBounds on 288. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:287: gfx::Rect pos = ComputeWindowPosition(preferred, base::i18n::IsRTL()); It looks like CompueteWindowPosition() uses the *current* bounds, but the next line sets it. That seems wrong. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView : public views::ButtonListener { I would expect a class named View to be a View, but that isn't the case here. Name TryChromeDialogController or perhaps TryChromeDialog. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView : public views::ButtonListener { Is there a test for this class? At a minimum you should have an interactive test. See https://chromium.googlesource.com/chromium/src/+/master/docs/testing/test_bro... . https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what strings to present and what controls are shown. Can group be an enum, that helps document what the type is. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:63: const ActiveModalDialogListener& listener, The callback should take the welcome_page as a value. That way there aren't any worries about lifetime of a pointer passed into this function (especially for the !modal case). https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:68: enum class kDialogType { This is super confusing as enums should not have k in the name. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:81: explicit TryChromeDialogView(size_t group); Document what group is here. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:105: size_t group_; const https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; const. And remember that time can go backwards. When measuring duration it's better to use TimeTicks, which can't go backwards.
On 2017/06/23 17:45:53, sky wrote: > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... > File chrome/app/theme/theme_resources.grd (right): > > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... > chrome/app/theme/theme_resources.grd:219: <structure type="chrome_scaled_image" > name="IDR_INACTIVE_TOAST_CLOSE_X" file="win/inactive_toast/toast_close.png" /> > It isn't particularly clear what "CLOSE_X" means. Maybe CLOSE_ICON? > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:48: enum ButtonTags { BT_NONE, > BT_CLOSE_BUTTON, BT_OK_BUTTON, BT_NO_THANKS_BUTTON }; > Why do you need BT_NONE? And make it enum class so that you don't need the > prefix BT_. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:52: int heading_id; > These need descriptions. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:92: class Win10StyleButton : > public views::LabelButton { > I think this would be clearer, and less code, if you have a function for this, > e.g. > > CreateWin10StyleButton(...); > > Also, it isn't particularly clear what false means for affirmitive. By that I > mean how the !affirmitive is used. I think an enum would be clearer here. That > is: enum TryChromeButtonType { OPEN_CHROME, NO_THANKS }. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:181: constexpr int inset_top = > 10; > Where do these values come from? We are trying to enforce consistent dialog > sizes. See LayoutProvider and it's various functions. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:215: > columns->AddPaddingColumn(0, 12 - inset_left); > Where does the 12 come from? Same comment about using LayoutProvider. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:253: > body_text->SetAutoColorReadabilityEnabled(false); > Why are you turning off readability? > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:260: if > (!kExperiments[group_].has_no_thanks_button) { > no {} > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:285: > layout->Layout(root_view); > Why do you need this layout? I would expect it after the call to SetBounds on > 288. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:287: gfx::Rect pos = > ComputeWindowPosition(preferred, base::i18n::IsRTL()); > It looks like CompueteWindowPosition() uses the *current* bounds, but the next > line sets it. That seems wrong. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/try_chrome_dialog_view.h (right): > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView : > public views::ButtonListener { > I would expect a class named View to be a View, but that isn't the case here. > Name TryChromeDialogController or perhaps TryChromeDialog. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView : > public views::ButtonListener { > Is there a test for this class? At a minimum you should have an interactive > test. See > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/test_bro... > . > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what > strings to present and what controls are shown. > Can group be an enum, that helps document what the type is. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:63: const > ActiveModalDialogListener& listener, > The callback should take the welcome_page as a value. That way there aren't any > worries about lifetime of a pointer passed into this function (especially for > the !modal case). > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:68: enum class kDialogType { > This is super confusing as enums should not have k in the name. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:81: explicit > TryChromeDialogView(size_t group); > Document what group is here. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:105: size_t group_; > const > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; > const. > And remember that time can go backwards. When measuring duration it's better to > use TimeTicks, which can't go backwards. Also, my understanding is we should be using vector icons for new assets. Can you use vector icons here?
Patchset #3 (id:60001) has been deleted
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
Thanks! svg assets are outstanding, wanted to FYI Nik on a couple of these comments, plus send the link to the spec driving the magic constants (internal: https://goto.google.com/egrck) Also, renamed some of the files to remove '_view'. If this makes review difficult I can swap back and rename them later. https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... chrome/app/theme/theme_resources.grd:219: <structure type="chrome_scaled_image" name="IDR_INACTIVE_TOAST_CLOSE_X" file="win/inactive_toast/toast_close.png" /> On 2017/06/23 17:45:52, sky wrote: > It isn't particularly clear what "CLOSE_X" means. Maybe CLOSE_ICON? done - CLOSE_ICON https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:48: enum ButtonTags { BT_NONE, BT_CLOSE_BUTTON, BT_OK_BUTTON, BT_NO_THANKS_BUTTON }; On 2017/06/23 17:45:52, sky wrote: > Why do you need BT_NONE? And make it enum class so that you don't need the > prefix BT_. Done. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:52: int heading_id; On 2017/06/23 17:45:52, sky wrote: > These need descriptions. Done. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:92: class Win10StyleButton : public views::LabelButton { On 2017/06/23 17:45:52, sky wrote: > I think this would be clearer, and less code, if you have a function for this, > e.g. > > CreateWin10StyleButton(...); apologies, function vs. having a class? This was originally to set properties of label() which is protected. > > Also, it isn't particularly clear what false means for affirmitive. By that I > mean how the !affirmitive is used. I think an enum would be clearer here. That > is: enum TryChromeButtonType { OPEN_CHROME, NO_THANKS }. agreed, had language from the specification (linked in next comment) in mind but it's not great without context. Switched to enum. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:181: constexpr int inset_top = 10; On 2017/06/23 17:45:52, sky wrote: > Where do these values come from? We are trying to enforce consistent dialog > sizes. See LayoutProvider and it's various functions. It's specced to look like a Win10 OS dialog, outside main Chrome UI surfaces (spec slides, internal: https://goto.google.com/egrck) https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:253: body_text->SetAutoColorReadabilityEnabled(false); On 2017/06/23 17:45:52, sky wrote: > Why are you turning off readability? done https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:260: if (!kExperiments[group_].has_no_thanks_button) { On 2017/06/23 17:45:52, sky wrote: > no {} Done. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:285: layout->Layout(root_view); On 2017/06/23 17:45:52, sky wrote: > Why do you need this layout? I would expect it after the call to SetBounds on > 288. honestly, not sure - this is updating an existing dialog. Did GetPreferredSize depend on Layout()? Moved to after SetBounds() and tested the variants with change. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:287: gfx::Rect pos = ComputeWindowPosition(preferred, base::i18n::IsRTL()); On 2017/06/23 17:45:52, sky wrote: > It looks like CompueteWindowPosition() uses the *current* bounds, but the next > line sets it. That seems wrong. as with the previous line, this was in an existing dialog, so it's absolutely possible I'm missing something. The bounds it gets on the first line of ComputeWindowPosition may be those of the screen the widget belongs to? https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView : public views::ButtonListener { On 2017/06/23 17:45:52, sky wrote: > Is there a test for this class? At a minimum you should have an interactive > test. See > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/test_bro... > . yes, there is a preexisting test, though it's the minimum: src/chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc (now renamed in this cl) I'll look at updating the test to run more than the 0-group experiment varient. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView : public views::ButtonListener { On 2017/06/23 17:45:52, sky wrote: > I would expect a class named View to be a View, but that isn't the case here. > Name TryChromeDialogController or perhaps TryChromeDialog. Named TryChromeDialog within this file, renamed the file. If this loses comment context I can rename back until everything's resolved. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what strings to present and what controls are shown. On 2017/06/23 17:45:53, sky wrote: > Can group be an enum, that helps document what the type is. It's passed in as an integer through the experiment framework. It could be an enum for clarify here, but would effectively be passed in as an int. It's also 14 groups that vary the dialog in relatively subtle ways, so naming might be nonintuitive (see the spec slides linked in the .cc) wdyt? @nikunjb @grt, this comment FYI. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:63: const ActiveModalDialogListener& listener, On 2017/06/23 17:45:52, sky wrote: > The callback should take the welcome_page as a value. That way there aren't any > worries about lifetime of a pointer passed into this function (especially for > the !modal case). I'll change Result to carry this info. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:68: enum class kDialogType { On 2017/06/23 17:45:52, sky wrote: > This is super confusing as enums should not have k in the name. Done. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:81: explicit TryChromeDialogView(size_t group); On 2017/06/23 17:45:53, sky wrote: > Document what group is here. Done. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:105: size_t group_; On 2017/06/23 17:45:52, sky wrote: > const Done. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; On 2017/06/23 17:45:52, sky wrote: > const. > And remember that time can go backwards. When measuring duration it's better to > use TimeTicks, which can't go backwards. (const done, ->TimeTicks outstanding) - @nikunjb FYI - can we use TimeTicks in the experiment code? Or I can internally and still send you a base::Time in the experiment reporting.
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what strings to present and what controls are shown. On 2017/06/24 01:09:00, skare_ wrote: > On 2017/06/23 17:45:53, sky wrote: > > Can group be an enum, that helps document what the type is. > > It's passed in as an integer through the experiment framework. It could be an > enum for clarify here, but would effectively be passed in as an int. > It's also 14 groups that vary the dialog in relatively subtle ways, so naming > might be nonintuitive (see the spec slides linked in the .cc) > wdyt? > @nikunjb @grt, this comment FYI. kExperiments (in the .cc file) effectively describes each group by mapping a group index to its UX-specific properties. while we could make an enum for the group identifier and give them names, i'm not sure doing so would add much. the name of a group alone doesn't tell the reader much. if |group| by itself is too ambiguous, how about changing |group| here to |group_index| so that it is at least clear that the value is an index into some table somewhere? https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:63: const ActiveModalDialogListener& listener, On 2017/06/23 17:45:52, sky wrote: > The callback should take the welcome_page as a value. That way there aren't any > worries about lifetime of a pointer passed into this function (especially for > the !modal case). i don't understand the suggestion. by "the callback", do you mean the |listener| argument?
sky@chromium.org changed reviewers: + tapted@chromium.org
+tapted for explicitly setting font on a LabelButton. See first comment. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:92: class Win10StyleButton : public views::LabelButton { On 2017/06/24 01:08:59, skare_ wrote: > On 2017/06/23 17:45:52, sky wrote: > > I think this would be clearer, and less code, if you have a function for this, > > e.g. > > > > CreateWin10StyleButton(...); > > apologies, function vs. having a class? This was originally to set properties of > label() which is protected. Ah, I see. I'm adding tapted to the review. I think this is a compelling enough reason to add back setting the font list. > > > > Also, it isn't particularly clear what false means for affirmitive. By that I > > mean how the !affirmitive is used. I think an enum would be clearer here. That > > is: enum TryChromeButtonType { OPEN_CHROME, NO_THANKS }. > > agreed, had language from the specification (linked in next comment) in mind but > it's not great without context. Switched to enum. > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:287: gfx::Rect pos = ComputeWindowPosition(preferred, base::i18n::IsRTL()); On 2017/06/24 01:08:59, skare_ wrote: > On 2017/06/23 17:45:52, sky wrote: > > It looks like CompueteWindowPosition() uses the *current* bounds, but the next > > line sets it. That seems wrong. > > as with the previous line, this was in an existing dialog, so it's absolutely > possible I'm missing something. The bounds it gets on the first line of > ComputeWindowPosition may be those of the screen the widget belongs to? Line 169 specifies an origin of 0x0. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what strings to present and what controls are shown. On 2017/06/24 01:09:00, skare_ wrote: > On 2017/06/23 17:45:53, sky wrote: > > Can group be an enum, that helps document what the type is. > > It's passed in as an integer through the experiment framework. It could be an > enum for clarify here, but would effectively be passed in as an int. > It's also 14 groups that vary the dialog in relatively subtle ways, so naming > might be nonintuitive (see the spec slides linked in the .cc) > wdyt? Agreed, keep it as an int. > @nikunjb @grt, this comment FYI.
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.cc:112: label()->SetFontList(gfx::FontList(kFontDefinitionSemiBold)); the font definition is "Segoe UI, Arial, Semi-Bold 15px" Segoe Semi-Bold is installed on all versions of Windows we support - why do you need Arial? And hardcoding 15pt will override User settings from Control Panel > Appearance and Personalization > Display -- is that intentional? (also hardcoding Semi-Bold can result in a lighter font than the user's default if they tick the 'Bold' checkbox on that settings dialog). Also the FontList won't be cached in ResourceBundle -- creating new fonts is typically slow since it needs to do IPC with a font server or access font files on disk. Can you define a style in chrome_typography.h? Then add handlers to the switch statements in HarmonyTypographyProvider and LegacyTypographyProvider. Then all these problems are solved for you, and if anyone else wants a specifically 'Font-that-matches-Windows-10-style' they can just use that style again.
I hate to walk in and throw bombs, but why is Views drawing any of this UI at all? If we want a Win 10-native-looking toast I think we should be drawing it directly with Windows APIs and not using any of the Views framework. Trying to make Views (e.g. the typography providers) able to "look like Win 10" seems like a lot of effort to go through just to get back to what we inherently get with native -- and native will auto-track future Windows design changes, whereas Chrome will have to chase them. It makes sense to use Views where we desire to write a piece of code once and have it appear on multiple platforms, but AIUI this isn't that -- this specific UI is intended to be Windows-only forever. Even if we add similar notifications on other platforms, we'd do so with different-looking UI.
On 2017/06/26 23:12:01, Peter Kasting wrote: > I hate to walk in and throw bombs, but why is Views drawing any of this UI at > all? If we want a Win 10-native-looking toast I think we should be drawing it > directly with Windows APIs and not using any of the Views framework. > > Trying to make Views (e.g. the typography providers) able to "look like Win 10" > seems like a lot of effort to go through just to get back to what we inherently > get with native -- and native will auto-track future Windows design changes, > whereas Chrome will have to chase them. > > It makes sense to use Views where we desire to write a piece of code once and > have it appear on multiple platforms, but AIUI this isn't that -- this specific > UI is intended to be Windows-only forever. Even if we add similar notifications > on other platforms, we'd do so with different-looking UI. Actively looking into native - but reasons for this approach were: -The requested spec is very close to to OS. but does have some potential differences (a variant that points to a taskbar with a pointer) and does hard-code constants, sizes, etc. -It's repurposing an existing dialog that was used and tested for this purpose before, though has been dormant. -I believe this is for a transient effort and likely won't need to track many OS changes - though I do recognize the MS Fluent Design system (neon?) is an upcoming thing to watch and test.
On 2017/06/29 17:29:40, skare_ wrote: > On 2017/06/26 23:12:01, Peter Kasting wrote: > > I hate to walk in and throw bombs, but why is Views drawing any of this UI at > > all? If we want a Win 10-native-looking toast I think we should be drawing it > > directly with Windows APIs and not using any of the Views framework. > > Actively looking into native - but reasons for this approach were: > -The requested spec is very close to to OS. but does have some potential > differences (a variant that points to a taskbar with a pointer) and does > hard-code constants, sizes, etc. In these cases, I think we should try very hard to just let the OS do what it wants. For things like sizes, usually specs/mocks have what the designer thinks is best/matches the OS, but actually being native is probably better. For things like pointing to the taskbar, if it's not a pattern the OS really supports, we probably should try not to do it either; diverging from the OS behavior should have a pretty high bar. > -It's repurposing an existing dialog that was used and tested for this purpose > before, though has been dormant. That definitely helps, but since we're Harmonizing everything, things that would typically be done as one-offs before now have to have centralized behavior. When we want something to "look native" that basically means we need to add "native appearance" support to the core systems like the typography provider as tapted mentioned, or else we have a lot of problems; similar tradeoffs occur in layout, coloring, etc. > -I believe this is for a transient effort and likely won't need to track many OS > changes - though I do recognize the MS Fluent Design system (neon?) is an > upcoming thing to watch and test. If we think this is a very brief experiment, it can make more sense to design with just the short-term implementation cost in mind. I don't know what route would minimize that, given my comments above. I'm also a bit leery of how many times in Chromium history a short-term experiment has just never gone away. Anyway, I'm confident you'll make the right decision, so I'll bow out. Just wanted to make sure it was understood that using views for this isn't necessarily trivial to get right, whereas native could probably get us better results with less impact on the surrounding system, and we could remove the old dormant dialog.
On 2017/06/23 19:25:02, sky OOO wrote: > On 2017/06/23 17:45:53, sky wrote: > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... > > File chrome/app/theme/theme_resources.grd (right): > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... > > chrome/app/theme/theme_resources.grd:219: <structure > type="chrome_scaled_image" > > name="IDR_INACTIVE_TOAST_CLOSE_X" file="win/inactive_toast/toast_close.png" /> > > It isn't particularly clear what "CLOSE_X" means. Maybe CLOSE_ICON? > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:48: enum ButtonTags { > BT_NONE, > > BT_CLOSE_BUTTON, BT_OK_BUTTON, BT_NO_THANKS_BUTTON }; > > Why do you need BT_NONE? And make it enum class so that you don't need the > > prefix BT_. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:52: int heading_id; > > These need descriptions. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:92: class Win10StyleButton : > > public views::LabelButton { > > I think this would be clearer, and less code, if you have a function for this, > > e.g. > > > > CreateWin10StyleButton(...); > > > > Also, it isn't particularly clear what false means for affirmitive. By that I > > mean how the !affirmitive is used. I think an enum would be clearer here. That > > is: enum TryChromeButtonType { OPEN_CHROME, NO_THANKS }. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:181: constexpr int inset_top > = > > 10; > > Where do these values come from? We are trying to enforce consistent dialog > > sizes. See LayoutProvider and it's various functions. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:215: > > columns->AddPaddingColumn(0, 12 - inset_left); > > Where does the 12 come from? Same comment about using LayoutProvider. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:253: > > body_text->SetAutoColorReadabilityEnabled(false); > > Why are you turning off readability? > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:260: if > > (!kExperiments[group_].has_no_thanks_button) { > > no {} > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:285: > > layout->Layout(root_view); > > Why do you need this layout? I would expect it after the call to SetBounds on > > 288. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:287: gfx::Rect pos = > > ComputeWindowPosition(preferred, base::i18n::IsRTL()); > > It looks like CompueteWindowPosition() uses the *current* bounds, but the next > > line sets it. That seems wrong. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > File chrome/browser/ui/views/try_chrome_dialog_view.h (right): > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView > : > > public views::ButtonListener { > > I would expect a class named View to be a View, but that isn't the case here. > > Name TryChromeDialogController or perhaps TryChromeDialog. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:39: class TryChromeDialogView > : > > public views::ButtonListener { > > Is there a test for this class? At a minimum you should have an interactive > > test. See > > > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/test_bro... > > . > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what > > strings to present and what controls are shown. > > Can group be an enum, that helps document what the type is. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:63: const > > ActiveModalDialogListener& listener, > > The callback should take the welcome_page as a value. That way there aren't > any > > worries about lifetime of a pointer passed into this function (especially for > > the !modal case). > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:68: enum class kDialogType { > > This is super confusing as enums should not have k in the name. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:81: explicit > > TryChromeDialogView(size_t group); > > Document what group is here. > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:105: size_t group_; > > const > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; > > const. > > And remember that time can go backwards. When measuring duration it's better > to > > use TimeTicks, which can't go backwards. > > Also, my understanding is we should be using vector icons for new assets. Can > you use vector icons here? Quick check on the vector icons - works great for x and the white Chrome logo dialog icon, shadowed caret pointer might need to be a png -- OK to mix formats for the feature? I'll update the change with a mix.
On 2017/07/04 06:09:36, skare_ wrote: > On 2017/06/23 19:25:02, sky OOO wrote: > > On 2017/06/23 17:45:53, sky wrote: > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... > > > File chrome/app/theme/theme_resources.grd (right): > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme... > > > chrome/app/theme/theme_resources.grd:219: <structure > > type="chrome_scaled_image" > > > name="IDR_INACTIVE_TOAST_CLOSE_X" file="win/inactive_toast/toast_close.png" > /> > > > It isn't particularly clear what "CLOSE_X" means. Maybe CLOSE_ICON? > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:48: enum ButtonTags { > > BT_NONE, > > > BT_CLOSE_BUTTON, BT_OK_BUTTON, BT_NO_THANKS_BUTTON }; > > > Why do you need BT_NONE? And make it enum class so that you don't need the > > > prefix BT_. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:52: int heading_id; > > > These need descriptions. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:92: class Win10StyleButton > : > > > public views::LabelButton { > > > I think this would be clearer, and less code, if you have a function for > this, > > > e.g. > > > > > > CreateWin10StyleButton(...); > > > > > > Also, it isn't particularly clear what false means for affirmitive. By that > I > > > mean how the !affirmitive is used. I think an enum would be clearer here. > That > > > is: enum TryChromeButtonType { OPEN_CHROME, NO_THANKS }. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:181: constexpr int > inset_top > > = > > > 10; > > > Where do these values come from? We are trying to enforce consistent dialog > > > sizes. See LayoutProvider and it's various functions. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:215: > > > columns->AddPaddingColumn(0, 12 - inset_left); > > > Where does the 12 come from? Same comment about using LayoutProvider. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:253: > > > body_text->SetAutoColorReadabilityEnabled(false); > > > Why are you turning off readability? > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:260: if > > > (!kExperiments[group_].has_no_thanks_button) { > > > no {} > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:285: > > > layout->Layout(root_view); > > > Why do you need this layout? I would expect it after the call to SetBounds > on > > > 288. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:287: gfx::Rect pos = > > > ComputeWindowPosition(preferred, base::i18n::IsRTL()); > > > It looks like CompueteWindowPosition() uses the *current* bounds, but the > next > > > line sets it. That seems wrong. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/try_chrome_dialog_view.h (right): > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:39: class > TryChromeDialogView > > : > > > public views::ButtonListener { > > > I would expect a class named View to be a View, but that isn't the case > here. > > > Name TryChromeDialogController or perhaps TryChromeDialog. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:39: class > TryChromeDialogView > > : > > > public views::ButtonListener { > > > Is there a test for this class? At a minimum you should have an interactive > > > test. See > > > > > > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/test_bro... > > > . > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what > > > strings to present and what controls are shown. > > > Can group be an enum, that helps document what the type is. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:63: const > > > ActiveModalDialogListener& listener, > > > The callback should take the welcome_page as a value. That way there aren't > > any > > > worries about lifetime of a pointer passed into this function (especially > for > > > the !modal case). > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:68: enum class kDialogType > { > > > This is super confusing as enums should not have k in the name. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:81: explicit > > > TryChromeDialogView(size_t group); > > > Document what group is here. > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:105: size_t group_; > > > const > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time > time_shown_; > > > const. > > > And remember that time can go backwards. When measuring duration it's better > > to > > > use TimeTicks, which can't go backwards. > > > > Also, my understanding is we should be using vector icons for new assets. Can > > you use vector icons here? > > Quick check on the vector icons - works great for x and the white Chrome logo > dialog icon, shadowed caret pointer might need to be a png -- OK to mix formats > for the feature? I'll update the change with a mix. update on the native vs. views point - I think we do want two experiment variants that aren't straightforward native -- one where the dialog will point to the taskbar, and one that has the x show and hide on mouseover (there's an action button as well). Those are doable with setwindowrgn and custom draw respectively but seem simpler with views. I have not looked into pushing back on those experiments altogether. I do understand the risk of "temporary thing lives forever" - perhaps we can cap the length of this experiment?
On 2017/07/04 06:20:36, skare_ wrote: > update on the native vs. views point - I think we do want two experiment > variants that aren't straightforward native -- one where the dialog will point > to the taskbar, and one that has the x show and hide on mouseover (there's an > action button as well). Those are doable with setwindowrgn and custom draw > respectively but seem simpler with views. Yes, we very much want to have the variants in the UI Review doc and present it over Chrome's taskbar icon whenever possible. > I have not looked into pushing back on those experiments altogether. I do > understand the risk of "temporary thing lives forever" - perhaps we can cap the > length of this experiment? The experiment will likely last ~6mo while we evaluate the different groups. A final variant of the prompt may live on much longer. Can we run the experiment with the views code and then switch to native (if appropriate) when a final variant is chosen?
On 2017/06/26 22:54:25, tapted wrote: > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog_view.cc:112: > label()->SetFontList(gfx::FontList(kFontDefinitionSemiBold)); > the font definition is "Segoe UI, Arial, Semi-Bold 15px" > > Segoe Semi-Bold is installed on all versions of Windows we support - why do you > need Arial? > > And hardcoding 15pt will override User settings from Control Panel > Appearance > and Personalization > Display -- is that intentional? (also hardcoding Semi-Bold > can result in a lighter font than the user's default if they tick the 'Bold' > checkbox on that settings dialog). > > Also the FontList won't be cached in ResourceBundle -- creating new fonts is > typically slow since it needs to do IPC with a font server or access font files > on disk. > > Can you define a style in chrome_typography.h? Then add handlers to the switch > statements in HarmonyTypographyProvider and LegacyTypographyProvider. Then all > these problems are solved for you, and if anyone else wants a specifically > 'Font-that-matches-Windows-10-style' they can just use that style again. @tapted -- Removed Arial from the list, thanks - I thought this wasn't always installed on prior versions of windows but was mistaken. This should only run on win10 in non-testing scenarios, in any case. Coding the size and weight was intentional to match the spec. Looking at It's ok if loading the fonts is slow, in case it's preferable to keep this from getting added more broadly -- this toast shows on installing an update, and a Chrome browser window won't be visible. I'm unaware of any other cases this style will be used -- but still definitely happy to add it to chrome_typography though. In that case, adding to the switch statement wouldn't parallel anything in there, correct? e.g. it'd be something of the form: switch (style) { case STYLE_EMPHASIZED: font_weight = gfx::Font::Weight::BOLD; break; >>> case STYLE_WINDOWS_10: return gfx::FontList(kWindows10FontDef); <<< } return ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta( size_delta, kFontStyle, font_weight); or is the intention to use styles there for weights but not the face? thanks!
On 2017/07/06 07:31:28, skare_ wrote: > On 2017/06/26 22:54:25, tapted wrote: > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > chrome/browser/ui/views/try_chrome_dialog_view.cc:112: > > label()->SetFontList(gfx::FontList(kFontDefinitionSemiBold)); > > the font definition is "Segoe UI, Arial, Semi-Bold 15px" > > > > Segoe Semi-Bold is installed on all versions of Windows we support - why do > you > > need Arial? > > > > And hardcoding 15pt will override User settings from Control Panel > > Appearance > > and Personalization > Display -- is that intentional? (also hardcoding > Semi-Bold > > can result in a lighter font than the user's default if they tick the 'Bold' > > checkbox on that settings dialog). > > > > Also the FontList won't be cached in ResourceBundle -- creating new fonts is > > typically slow since it needs to do IPC with a font server or access font > files > > on disk. > > > > Can you define a style in chrome_typography.h? Then add handlers to the switch > > statements in HarmonyTypographyProvider and LegacyTypographyProvider. Then all > > these problems are solved for you, and if anyone else wants a specifically > > 'Font-that-matches-Windows-10-style' they can just use that style again. > > @tapted -- > > Removed Arial from the list, thanks - I thought this wasn't always installed on > prior versions of windows but was mistaken. This should only run on win10 in > non-testing scenarios, in any case. > > Coding the size and weight was intentional to match the spec. Looking at In my experience it's been unusual for a spec to acknowledge that users can actually override their font sizes on a system-wide basis, but Chrome certainly tries to respect that. On Mac and ChromeOS it's very hard.. I don't really know where to start with Linux either, but on Windows changing the system font size is very easy. Searching the start menu for "font size" brings up Control Panel\Appearance and Personalization\Display . Changing the font size for "Message Boxes" is what Chrome uses to guide most of its UI. I'd say it's likely (although not a given..) that the native Windows 10 toasts use this value too. > > It's ok if loading the fonts is slow, in case it's preferable to keep this from > getting added more broadly -- this toast shows on installing an update, and a > Chrome browser window won't be visible. I'm unaware of any other cases this > style will be used -- but still definitely happy to add it to chrome_typography > though. Yeah performance for the specific case here isn't really the issue. But exposing a `SetFontList` on the public API is what opens the door for developers to make subtly bad performance choices in future. E.g. SetFont(GetFont().Derive(1)) gives you a font "one point larger", but that's super-slow compared to obtaining the font via ResourceBundle or typography styles. > > In that case, adding to the switch statement wouldn't parallel anything in > there, correct? > e.g. it'd be something of the form: > > switch (style) { > case STYLE_EMPHASIZED: > font_weight = gfx::Font::Weight::BOLD; > break; > >>> > case STYLE_WINDOWS_10: > return gfx::FontList(kWindows10FontDef); > <<< > } > return ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta( > size_delta, kFontStyle, font_weight); > > or is the intention to use styles there for weights but not the face? So the boldness would actually match what we use for CONTEXT_BUTTON_MD . Although it says "medium" there's no such thing as Segoe-UI-Medium, so you get SemiBold. (other platforms all want "medium", so it didn't seem worth it to #ifdef WIN if it didn't actually make a difference). But you need to bump up the size. To avoid repeated code, I suggest a helper, in chrome_typography.h like enum ChromeTextContext { ... // Appropriate comment CONTEXT_WINDOWS10_NATIVE, ... } void ApplyCommonFontStyles(in context, int style, int* size_delta, gfx::Font::Weight* weight) { if (context == CONTEXT_WINDOWS10_NATIVE) { *size_delta = 15 - gfx::PlatformFont::kDefaultBaseFontSize; *weight = WeightNotLighterThanNormal(gfx::Font::Weight::MEDIUM); } } Then call ApplyCommonFontStyles(..) from HarmonyTypographyProvider::GetFont and LegacyTypographyProvider::GetFont (i.e. just after the call to ash::ApplyAshFontStyles) (And note the default font on Windows for Chrome is Segoe, so there's no need to specify the typeface)
On 2017/07/06 07:52:23, tapted wrote: > On 2017/07/06 07:31:28, skare_ wrote: > > On 2017/06/26 22:54:25, tapted wrote: > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): > > > > > > > > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... > > > chrome/browser/ui/views/try_chrome_dialog_view.cc:112: > > > label()->SetFontList(gfx::FontList(kFontDefinitionSemiBold)); > > > the font definition is "Segoe UI, Arial, Semi-Bold 15px" > > > > > > Segoe Semi-Bold is installed on all versions of Windows we support - why do > > you > > > need Arial? > > > > > > And hardcoding 15pt will override User settings from Control Panel > > > Appearance > > > and Personalization > Display -- is that intentional? (also hardcoding > > Semi-Bold > > > can result in a lighter font than the user's default if they tick the 'Bold' > > > checkbox on that settings dialog). > > > > > > Also the FontList won't be cached in ResourceBundle -- creating new fonts is > > > typically slow since it needs to do IPC with a font server or access font > > files > > > on disk. > > > > > > Can you define a style in chrome_typography.h? Then add handlers to the > switch > > > statements in HarmonyTypographyProvider and LegacyTypographyProvider. Then > all > > > these problems are solved for you, and if anyone else wants a specifically > > > 'Font-that-matches-Windows-10-style' they can just use that style again. > > > > @tapted -- > > > > Removed Arial from the list, thanks - I thought this wasn't always installed > on > > prior versions of windows but was mistaken. This should only run on win10 in > > non-testing scenarios, in any case. > > > > Coding the size and weight was intentional to match the spec. Looking at > > In my experience it's been unusual for a spec to acknowledge that users can > actually override their font sizes on a system-wide basis, but Chrome certainly > tries to respect that. On Mac and ChromeOS it's very hard.. I don't really know > where to start with Linux either, but on Windows changing the system font size > is very easy. Searching the start menu for "font size" brings up Control > Panel\Appearance and Personalization\Display . Changing the font size for > "Message Boxes" is what Chrome uses to guide most of its UI. I'd say it's likely > (although not a given..) that the native Windows 10 toasts use this value too. > > > > > It's ok if loading the fonts is slow, in case it's preferable to keep this > from > > getting added more broadly -- this toast shows on installing an update, and a > > Chrome browser window won't be visible. I'm unaware of any other cases this > > style will be used -- but still definitely happy to add it to > chrome_typography > > though. > > Yeah performance for the specific case here isn't really the issue. But exposing > a `SetFontList` on the public API is what opens the door for developers to make > subtly bad performance choices in future. E.g. SetFont(GetFont().Derive(1)) > gives you a font "one point larger", but that's super-slow compared to obtaining > the font via ResourceBundle or typography styles. > > > > > In that case, adding to the switch statement wouldn't parallel anything in > > there, correct? > > e.g. it'd be something of the form: > > > > switch (style) { > > case STYLE_EMPHASIZED: > > font_weight = gfx::Font::Weight::BOLD; > > break; > > >>> > > case STYLE_WINDOWS_10: > > return gfx::FontList(kWindows10FontDef); > > <<< > > } > > return ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta( > > size_delta, kFontStyle, font_weight); > > > > or is the intention to use styles there for weights but not the face? > > > So the boldness would actually match what we use for CONTEXT_BUTTON_MD . > Although it says "medium" there's no such thing as Segoe-UI-Medium, so you get > SemiBold. (other platforms all want "medium", so it didn't seem worth it to > #ifdef WIN if it didn't actually make a difference). > > But you need to bump up the size. > > To avoid repeated code, I suggest a helper, in chrome_typography.h like > > enum ChromeTextContext { > ... > // Appropriate comment > CONTEXT_WINDOWS10_NATIVE, > ... > } > > void ApplyCommonFontStyles(in context, int style, int* size_delta, > gfx::Font::Weight* weight) { > if (context == CONTEXT_WINDOWS10_NATIVE) { > *size_delta = 15 - gfx::PlatformFont::kDefaultBaseFontSize; > *weight = WeightNotLighterThanNormal(gfx::Font::Weight::MEDIUM); > } > } > > Then call ApplyCommonFontStyles(..) from HarmonyTypographyProvider::GetFont and > LegacyTypographyProvider::GetFont (i.e. just after the call to > ash::ApplyAshFontStyles) > > > > (And note the default font on Windows for Chrome is Segoe, so there's no need to > specify the typeface) thanks, that works great and removes the font_list dependency. Simplified with a new snapshot, if it looks good I can turn the derived class into a function.
the typography approach looks reasonable to me - hopefully sky is on board with it too I only did a quick skim of some other bits https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.h (right): https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:33: // Appropriate comment I see you took my code fragment literally... I guess // Text for titles, body text and buttons that appear in dialogs attempting to // mimic the native Windows 10 look and feel. https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:58: STYLE_WINDOWS_10, I don't think this is used (The parallel I use for context vs style is along the lines of CSS for an <element> vs CSS for a .class) https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:65: // Sets the |size_delta| and |font_weight| for specific text contexts. // Sets the |size_delta| and |font_weight| for text that should not be affected by the Harmony spec. https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:66: static void ApplyCommonFontStyles(int context, split declaration/definition https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:173: constexpr int inset_top = 10; I think you can constexpr gfx::Insets kInsets(10, 10, 12, 3); and refer to these as kInsets.top() etc.
Patchset #7 (id:220001) has been deleted
Patchset #1 (id:20001) has been deleted
On 2017/07/07 01:51:21, tapted wrote: > the typography approach looks reasonable to me - hopefully sky is on board with > it too > > I only did a quick skim of some other bits > > https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... > File chrome/browser/ui/views/harmony/chrome_typography.h (right): > > https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/chrome_typography.h:33: // Appropriate comment > I see you took my code fragment literally... I guess > > // Text for titles, body text and buttons that appear in dialogs attempting to > // mimic the native Windows 10 look and feel. > > https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/chrome_typography.h:58: STYLE_WINDOWS_10, > I don't think this is used (The parallel I use for context vs style is along the > lines of CSS for an <element> vs CSS for a .class) > > https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/chrome_typography.h:65: // Sets the |size_delta| > and |font_weight| for specific text contexts. > // Sets the |size_delta| and |font_weight| for text that should not be affected > by the Harmony spec. > > https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/chrome_typography.h:66: static void > ApplyCommonFontStyles(int context, > split declaration/definition > > https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... > File chrome/browser/ui/views/try_chrome_dialog.cc (right): > > https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... > chrome/browser/ui/views/try_chrome_dialog.cc:173: constexpr int inset_top = 10; > I think you can > > constexpr gfx::Insets kInsets(10, 10, 12, 3); > > and refer to these as kInsets.top() etc. thanks - apologies, should have stated that was just checking things were in the right places. Cleared up comments. Removing the font_list_ allowed simplifying the class to a function.
https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.h (right): https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:33: // Appropriate comment On 2017/07/07 01:51:21, tapted wrote: > I see you took my code fragment literally... I guess > > // Text for titles, body text and buttons that appear in dialogs attempting to > // mimic the native Windows 10 look and feel. Done (sorry, just checking the approach in an intermediate snapshot) https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:58: STYLE_WINDOWS_10, On 2017/07/07 01:51:21, tapted wrote: > I don't think this is used (The parallel I use for context vs style is along the > lines of CSS for an <element> vs CSS for a .class) Done. https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:65: // Sets the |size_delta| and |font_weight| for specific text contexts. On 2017/07/07 01:51:21, tapted wrote: > // Sets the |size_delta| and |font_weight| for text that should not be affected > by the Harmony spec. Done. https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:66: static void ApplyCommonFontStyles(int context, On 2017/07/07 01:51:21, tapted wrote: > split declaration/definition Done. https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:173: constexpr int inset_top = 10; On 2017/07/07 01:51:21, tapted wrote: > I think you can > > constexpr gfx::Insets kInsets(10, 10, 12, 3); > > and refer to these as kInsets.top() etc. Done.
lgtm with some nits https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:51: // Sets the |size_delta| and |font_weight| for text that should not be affected nit: duplicate comment not required https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:53: void ApplyCommonFontStyles(int context, nit: move to top of the file so it matches the declaration order https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.h (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:9: #include "ui/gfx/platform_font.h" nit: move to .cc. Here, I guess we need gfx::Font::Weight, but that's in ui/gfx/font.h
On 2017/07/07 03:32:20, tapted wrote: > lgtm with some nits > > https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... > File chrome/browser/ui/views/harmony/chrome_typography.cc (right): > > https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/chrome_typography.cc:51: // Sets the > |size_delta| and |font_weight| for text that should not be affected > nit: duplicate comment not required > > https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/chrome_typography.cc:53: void > ApplyCommonFontStyles(int context, > nit: move to top of the file so it matches the declaration order > > https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... > File chrome/browser/ui/views/harmony/chrome_typography.h (right): > > https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... > chrome/browser/ui/views/harmony/chrome_typography.h:9: #include > "ui/gfx/platform_font.h" > nit: move to .cc. Here, I guess we need gfx::Font::Weight, but that's in > ui/gfx/font.h @sky, is this ok for the subthread you +'ed tapted on? Note assets were also changed to vector icons.
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; On 2017/06/24 01:09:00, skare_ wrote: > On 2017/06/23 17:45:52, sky wrote: > > const. > > And remember that time can go backwards. When measuring duration it's better > to > > use TimeTicks, which can't go backwards. > > (const done, ->TimeTicks outstanding) - > > @nikunjb FYI - can we use TimeTicks in the experiment code? Or I can internally > and still send you a base::Time in the experiment reporting. Was this comment resolved? Again, I think you should be using timeticks for this code, otherwise the time delta may be negative. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1572: if (answer == TryChromeDialog::NOT_NOW) { no {} That said, wouldn't a switch be better here? https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:127: if (group >= arraysize(kExperiments)) { Under what conditions can this happen? Generally we DCHECK this sort of code. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:132: TryChromeDialog::Result result = no need for local variable here. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:171: constexpr int label_spacing = 10; Be consistent. If you're going to use kInsets on 167, then use kLabelSpacing. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:197: columns->AddPaddingColumn(0, 12 - kInsets.left()); Use a constant for the 12 (you use 12 on line 203 too). https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:257: gfx::Size preferred = layout->GetPreferredSize(root_view); root_view->GetPreferredSize(). https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:260: layout->Layout(root_view); root_view->Layout(); That said, I'm not really sure you need this line. I suspect you don't. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:283: // If the dialog is not modal, we don't control when it is going to be I don't understand this comment. This code could easily track when popup_ is destroyed if it needs to. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:294: gfx::Rect TryChromeDialog::ComputeWindowPosition(const gfx::Size& size, As this computes the bounds, how about ComputeWindowBounds? Or given you named the member popup, ComputePopupBounds? https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:310: // The user pressed No Thanks or the [x] button. optional: this comment and that on 313 documents exactly what the code does and isn't particularly useful, nuke it. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:341: base::MessageLoop::current()->QuitWhenIdle(); Why do you need the quit when idle here? https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.h (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.h:107: UsageType usage_type_; Make sure you initialize this. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.h:114: gfx::Rect chrome_taskbar_rect_; AFAICT this member is not used at all. Please remove (if you are going to need it at a later date, add it then).
https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:127: if (group >= arraysize(kExperiments)) { On 2017/07/18 17:10:32, sky wrote: > Under what conditions can this happen? Generally we DCHECK this sort of code. |group| originates from the command line, so it's possible for it to have any bogus value. what if this were rolled into the clause above so that any out of range value is interpreted as a test value? https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:171: constexpr int label_spacing = 10; On 2017/07/18 17:10:32, sky wrote: > Be consistent. If you're going to use kInsets on 167, then use kLabelSpacing. that, and use "static constexpr" so that the values are all in read-only data rather than pushed onto the stack.
https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:127: if (group >= arraysize(kExperiments)) { On 2017/07/20 05:58:58, grt (UTC plus 2) wrote: > On 2017/07/18 17:10:32, sky wrote: > > Under what conditions can this happen? Generally we DCHECK this sort of code. > > |group| originates from the command line, so it's possible for it to have any > bogus value. Ok, makes sense. Please add a comment. > what if this were rolled into the clause above so that any out of range value is > interpreted as a test value?
The CQ bit was checked by skare@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...
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; On 2017/07/18 17:10:31, sky wrote: > On 2017/06/24 01:09:00, skare_ wrote: > > On 2017/06/23 17:45:52, sky wrote: > > > const. > > > And remember that time can go backwards. When measuring duration it's better > > to > > > use TimeTicks, which can't go backwards. > > > > (const done, ->TimeTicks outstanding) - > > > > @nikunjb FYI - can we use TimeTicks in the experiment code? Or I can > internally > > and still send you a base::Time in the experiment reporting. > > Was this comment resolved? Again, I think you should be using timeticks for this > code, otherwise the time delta may be negative. used ticks to compute the delta, Time::Now() for the single-point time-shown requested by the experiment data interface. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1572: if (answer == TryChromeDialog::NOT_NOW) { On 2017/07/18 17:10:31, sky wrote: > no {} That said, wouldn't a switch be better here? case'd https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:51: // Sets the |size_delta| and |font_weight| for text that should not be affected On 2017/07/07 03:32:19, tapted wrote: > nit: duplicate comment not required Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:53: void ApplyCommonFontStyles(int context, On 2017/07/07 03:32:19, tapted wrote: > nit: move to top of the file so it matches the declaration order Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.h (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.h:9: #include "ui/gfx/platform_font.h" On 2017/07/07 03:32:19, tapted wrote: > nit: move to .cc. Here, I guess we need gfx::Font::Weight, but that's in > ui/gfx/font.h Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:127: if (group >= arraysize(kExperiments)) { On 2017/07/18 17:10:32, sky wrote: > Under what conditions can this happen? Generally we DCHECK this sort of code. It's not expected to happen. The group is driven by four bits in an omaha experiment config sent on the commandline (as grt@ replied), so hitting it would probably be due to corruption, a bug in the server piece of the system, bad custom commandline, etc. Rolled it into the test case. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:132: TryChromeDialog::Result result = On 2017/07/18 17:10:32, sky wrote: > no need for local variable here. Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:171: constexpr int label_spacing = 10; On 2017/07/18 17:10:32, sky wrote: > Be consistent. If you're going to use kInsets on 167, then use kLabelSpacing. Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:197: columns->AddPaddingColumn(0, 12 - kInsets.left()); On 2017/07/18 17:10:32, sky wrote: > Use a constant for the 12 (you use 12 on line 203 too). Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:257: gfx::Size preferred = layout->GetPreferredSize(root_view); On 2017/07/18 17:10:32, sky wrote: > root_view->GetPreferredSize(). Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:260: layout->Layout(root_view); On 2017/07/18 17:10:32, sky wrote: > root_view->Layout(); That said, I'm not really sure you need this line. I > suspect you don't. Removed https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:283: // If the dialog is not modal, we don't control when it is going to be On 2017/07/18 17:10:32, sky wrote: > I don't understand this comment. This code could easily track when popup_ is > destroyed if it needs to. My original reading - certainly could be wrong - was that only the modal version invokes the RunLoop and waits for action, so in modeless cases this function itself returns before the dialog has closed. Agreed "cannot" may be too strong - could be worked around if needed. Rewrote the comment and ShowDialog header a bit. Probably relevant - this dialog historically and currently runs before a browser window has opened, and we may or may not open a browser window depending on the dialog choice. Only the modal case is used in the field, modeless is for a test and perhaps for historical reasons I'm unaware of. I changed MODELESS to MODELESS_FOR_TEST so that's not implicit. Additionally the test might be overloaded slightly to validate some early shutdown behavior. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:294: gfx::Rect TryChromeDialog::ComputeWindowPosition(const gfx::Size& size, On 2017/07/18 17:10:31, sky wrote: > As this computes the bounds, how about ComputeWindowBounds? Or given you named > the member popup, ComputePopupBounds? Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:310: // The user pressed No Thanks or the [x] button. On 2017/07/18 17:10:32, sky wrote: > optional: this comment and that on 313 documents exactly what the code does and > isn't particularly useful, nuke it. sure, removed https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:341: base::MessageLoop::current()->QuitWhenIdle(); On 2017/07/18 17:10:32, sky wrote: > Why do you need the quit when idle here? I think it's to exit the RunLoop on :287 of this snapshot (testing removing it confirms) https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.h (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.h:107: UsageType usage_type_; On 2017/07/18 17:10:32, sky wrote: > Make sure you initialize this. Done. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.h:114: gfx::Rect chrome_taskbar_rect_; On 2017/07/18 17:10:32, sky wrote: > AFAICT this member is not used at all. Please remove (if you are going to need > it at a later date, add it then). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:122: if (group > arraysize(kExperiments)) { ">=" since |group| is an index into the array? https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:297: return display::Screen::GetScreen()->ScreenToDIPRectInWindow( have you tried this on a hidpi display? in https://crbug.com/731026, i thought i determined that this extra scaling was incorrect.
https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:283: // If the dialog is not modal, we don't control when it is going to be On 2017/07/21 03:11:35, skare_ wrote: > On 2017/07/18 17:10:32, sky wrote: > > I don't understand this comment. This code could easily track when popup_ is > > destroyed if it needs to. > > My original reading - certainly could be wrong - was that only the modal version > invokes the RunLoop and waits for action, so in modeless cases this function > itself returns before the dialog has closed. Agreed "cannot" may be too strong - > could be worked around if needed. > Rewrote the comment and ShowDialog header a bit. > > Probably relevant - this dialog historically and currently runs before a browser > window has opened, and we may or may not open a browser window depending on the > dialog choice. Only the modal case is used in the field, modeless is for a test > and perhaps for historical reasons I'm unaware of. I changed MODELESS to > MODELESS_FOR_TEST so that's not implicit. Additionally the test might be > overloaded slightly to validate some early shutdown behavior. The modeless version certainly doesn't block here, but why does that mean you can't run the Callback? If it's only expected that the callback run for the modal case, that's fine, just document it. https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:341: base::MessageLoop::current()->QuitWhenIdle(); On 2017/07/21 03:11:35, skare_ wrote: > On 2017/07/18 17:10:32, sky wrote: > > Why do you need the quit when idle here? > > I think it's to exit the RunLoop on :287 of this snapshot (testing removing it > confirms) Can you elaborate on why you need to run a nested message loop? Generally doing that can lead to all sorts of problems that aren't easily anticipated.
sky@ - apologies, thought I addressed your nested runloop question earlier. Rebased, addressed comments: https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:283: // If the dialog is not modal, we don't control when it is going to be On 2017/07/21 14:39:36, sky wrote: > On 2017/07/21 03:11:35, skare_ wrote: > > On 2017/07/18 17:10:32, sky wrote: > > > I don't understand this comment. This code could easily track when popup_ is > > > destroyed if it needs to. > > > > My original reading - certainly could be wrong - was that only the modal > version > > invokes the RunLoop and waits for action, so in modeless cases this function > > itself returns before the dialog has closed. Agreed "cannot" may be too strong > - > > could be worked around if needed. > > Rewrote the comment and ShowDialog header a bit. > > > > Probably relevant - this dialog historically and currently runs before a > browser > > window has opened, and we may or may not open a browser window depending on > the > > dialog choice. Only the modal case is used in the field, modeless is for a > test > > and perhaps for historical reasons I'm unaware of. I changed MODELESS to > > MODELESS_FOR_TEST so that's not implicit. Additionally the test might be > > overloaded slightly to validate some early shutdown behavior. > > The modeless version certainly doesn't block here, but why does that mean you > can't run the Callback? If it's only expected that the callback run for the > modal case, that's fine, just document it. Done - added a comment in .h and renamed the DialogType::MODELESS to MODELESS_FOR_TESTING to differentiate https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:341: base::MessageLoop::current()->QuitWhenIdle(); On 2017/07/21 14:39:36, sky wrote: > On 2017/07/21 03:11:35, skare_ wrote: > > On 2017/07/18 17:10:32, sky wrote: > > > Why do you need the quit when idle here? > > > > I think it's to exit the RunLoop on :287 of this snapshot (testing removing it > > confirms) > > Can you elaborate on why you need to run a nested message loop? Generally doing > that can lead to all sorts of problems that aren't easily anticipated. This uses an existing dialog so I could be mistaken, but I believe it's because in non-test scenarios this shows before the main browser window is visible (and if this dialog is declined we'll never show one) -- in PreMainMessageLoopRunImpl. https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:122: if (group > arraysize(kExperiments)) { On 2017/07/21 08:15:06, grt (UTC plus 2) wrote: > ">=" since |group| is an index into the array? Done. https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:297: return display::Screen::GetScreen()->ScreenToDIPRectInWindow( On 2017/07/21 08:15:05, grt (UTC plus 2) wrote: > have you tried this on a hidpi display? in https://crbug.com/731026, i thought i > determined that this extra scaling was incorrect. I wasn't able to get to a real hidpi monitor on Windows last week, will check with robliao who I think has one - don't want to delay this further so I ported that in and tested with --force-device-scale-factor=2.
looks pretty good, and appears in the right place on my HiDPI display. https://chromium-review.googlesource.com/c/618688 is needed to fix a bug i found while testing. do you have a list somewhere of the features that remain to be implemented (x-to-close appearing on hover, position over taskbar pin, +++)? https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:297: return display::Screen::GetScreen()->ScreenToDIPRectInWindow( On 2017/08/17 03:01:44, skare_ wrote: > On 2017/07/21 08:15:05, grt (UTC plus 2) wrote: > > have you tried this on a hidpi display? in https://crbug.com/731026, i thought > i > > determined that this extra scaling was incorrect. > > I wasn't able to get to a real hidpi monitor on Windows last week, will check > with robliao who I think has one - don't want to delay this further so I ported > that in and tested with --force-device-scale-factor=2. It looks like you picked up the important part of Ananta's fix in r489707, so I think this is good. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1531: case TryChromeDialog::OPEN_CHROME_DEFAULT: nit: the pedant in me would like to see this as the second case so that the order here is the same as the ordering of the enum itself. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:15: if (context == CONTEXT_WINDOWS10_NATIVE) { do non-win platforms need this code? should there be a #if defined(OS_WIN) around this? https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:16: *size_delta = 15 - gfx::PlatformFont::kDefaultBaseFontSize; could you drop a comment explaining 15? https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (left): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:210: l10n_util::GetStringUTF16(IDS_TRY_TOAST_TRY_OPT), kRadioGroupID); please delete IDS_TRY_TOAST_TRY_OPT, IDS_TRY_TOAST_SET_DEFAULT, and IDS_TRY_TOAST_CANCEL from the .grd files. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:10: #include "base/run_loop.h" omit; already included in .h https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:26: #include "ui/views/controls/button/button.h" omit; already included in .h https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:46: enum class ButtonTags { CLOSE_BUTTON, OK_BUTTON, NO_THANKS_BUTTON }; nit: singular ButtonTag https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:90: {IDS_WIN10_TOAST_SWITCH_SMART_AND_SECURE, IDS_WIN10_TOAST_RECOMMENDATION, variant #14 should not have the x-to-close button. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:110: button->SetMaxSize(gfx::Size(0, 32)); maybe dumb q: why is the max width smaller than the min? what does that do? https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:328: popup_->Close(); the Widget is destroyed sometime after this call (while pumping messages), thereby making popup_ a dangling pointer. please null it out here to avoid an accidental UAF. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:329: run_loop_.QuitWhenIdle(); i suppose this is harmless if it it's a MODELESS_FOR_TEST run? https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.h (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.h:86: // selection, and is used in testing. nit: is it relevant that the function unconditionally returns NOT_NOW when dialog_type is MODELESS_FOR_TEST?
Hi Greg - here are comments on your comments. I'll need at least one more change - after rebasing the affirmative button color is not painted (looking at 617407 and other things that landed yesterday) https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1531: case TryChromeDialog::OPEN_CHROME_DEFAULT: On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > nit: the pedant in me would like to see this as the second case so that the > order here is the same as the ordering of the enum itself. Done. Didn't change this code but moved OPEN_CHROME_DEFAULT to the bottom of the enum. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/chrome_typography.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:15: if (context == CONTEXT_WINDOWS10_NATIVE) { On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > do non-win platforms need this code? should there be a > #if defined(OS_WIN) > around this? I don't think they need this. ifdef'ed out https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/chrome_typography.cc:16: *size_delta = 15 - gfx::PlatformFont::kDefaultBaseFontSize; On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > could you drop a comment explaining 15? Done. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (left): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:210: l10n_util::GetStringUTF16(IDS_TRY_TOAST_TRY_OPT), kRadioGroupID); On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > please delete IDS_TRY_TOAST_TRY_OPT, IDS_TRY_TOAST_SET_DEFAULT, and > IDS_TRY_TOAST_CANCEL from the .grd files. removed these will take a look at removing the other obsolete TOAST strings (header et. al.) and attrition_experiments.h in another change. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:10: #include "base/run_loop.h" On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > omit; already included in .h Done. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:26: #include "ui/views/controls/button/button.h" On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > omit; already included in .h Done. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:46: enum class ButtonTags { CLOSE_BUTTON, OK_BUTTON, NO_THANKS_BUTTON }; On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > nit: singular ButtonTag Done. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:90: {IDS_WIN10_TOAST_SWITCH_SMART_AND_SECURE, IDS_WIN10_TOAST_RECOMMENDATION, On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > variant #14 should not have the x-to-close button. ack - currently all the variants do. The hover behavior and not running for tablet is outstanding https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:110: button->SetMaxSize(gfx::Size(0, 32)); On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > maybe dumb q: why is the max width smaller than the min? what does that do? The height was to force 32; setting the width to 0 is so max_width is ignored in some checks in /ui/views/controls/button/label_button.cc. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:328: popup_->Close(); On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > the Widget is destroyed sometime after this call (while pumping messages), > thereby making popup_ a dangling pointer. please null it out here to avoid an > accidental UAF. Done. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:329: run_loop_.QuitWhenIdle(); On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > i suppose this is harmless if it it's a MODELESS_FOR_TEST run? I believe so from experimentation and its presence in the existing dialog_view. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.h (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.h:86: // selection, and is used in testing. On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > nit: is it relevant that the function unconditionally returns NOT_NOW when > dialog_type is MODELESS_FOR_TEST? Done.
lgtm w/ nits and a q https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:90: {IDS_WIN10_TOAST_SWITCH_SMART_AND_SECURE, IDS_WIN10_TOAST_RECOMMENDATION, On 2017/08/18 11:37:34, skare_ wrote: > On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > > variant #14 should not have the x-to-close button. > > ack - currently all the variants do. Please drop a "TODO(skare): Suppress x-to-close." and a "TODO(skare): Implement hover behavior for x-to-close." for these. Thanks. > The hover behavior and not running for > tablet is outstanding The installer takes care of not launching Chrome to show the UX if the machine is a tablet device. I don't think you need any special handling here for that. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:110: button->SetMaxSize(gfx::Size(0, 32)); On 2017/08/18 11:37:34, skare_ wrote: > On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > > maybe dumb q: why is the max width smaller than the min? what does that do? > > The height was to force 32; setting the width to 0 is so max_width is ignored in > some checks in /ui/views/controls/button/label_button.cc. Ah, thanks. Is this worthy of an explanatory comment, or is it obvious to anyone familiar with the views arts (which I'm not)?
@sky, welcome comments on views items. I commented on the earlier nested run loop question, but that is still present (and I believe intended) @grt, fixed those two nits. @everyone, please ignore my earlier 'button background color change required' comment, looked like a temporary regression. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:90: {IDS_WIN10_TOAST_SWITCH_SMART_AND_SECURE, IDS_WIN10_TOAST_RECOMMENDATION, On 2017/08/18 12:05:24, grt (UTC plus 2) wrote: > On 2017/08/18 11:37:34, skare_ wrote: > > On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > > > variant #14 should not have the x-to-close button. > > > > ack - currently all the variants do. > > Please drop a "TODO(skare): Suppress x-to-close." and a "TODO(skare): Implement > hover behavior for x-to-close." for these. Thanks. > > > The hover behavior and not running for > > tablet is outstanding > > The installer takes care of not launching Chrome to show the UX if the machine > is a tablet device. I don't think you need any special handling here for that. Done. Great for the tablet case. https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:110: button->SetMaxSize(gfx::Size(0, 32)); On 2017/08/18 12:05:24, grt (UTC plus 2) wrote: > On 2017/08/18 11:37:34, skare_ wrote: > > On 2017/08/17 11:26:32, grt (UTC plus 2) wrote: > > > maybe dumb q: why is the max width smaller than the min? what does that do? > > > > The height was to force 32; setting the width to 0 is so max_width is ignored > in > > some checks in /ui/views/controls/button/label_button.cc. > > Ah, thanks. Is this worthy of an explanatory comment, or is it obvious to anyone > familiar with the views arts (which I'm not)? sure, added (though I'm less familiar with views arts and did this by views codepath inspection, so it's potentially unnecessary now, checking different resolutions, sizes, i18n lengths)
Thanks for the comments, and it makes sense. LGTM
The CQ bit was checked by skare@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by skare@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...
thanks for reviews, everyone FYI, moved run_loop_ to a unique_ptr<> so its ctor doesn't run on tests (modal version not compatible with the task manager there). It was originally on the stack but moved it to an instance for recent changes to base::RunLoop. Thanks!
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 skare@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nikunjb@chromium.org, tapted@chromium.org, grt@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2904823002/#ps360001 (title: "move run_loop_ to a unique_ptr")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 360001, "attempt_start_ts": 1503263126627780, "parent_rev": "6ce68a3fb3e459bb4e9676a508a9260d4e67bd10", "commit_rev": "c4f03f032136d23585d10951d6b855b40c83431c"}
Message was sent while issue was closed.
Description was changed from ========== Inactive toast ux changes BUG=717091 ========== to ========== Inactive toast ux changes BUG=717091 Review-Url: https://codereview.chromium.org/2904823002 Cr-Commit-Position: refs/heads/master@{#495849} Committed: https://chromium.googlesource.com/chromium/src/+/c4f03f032136d23585d10951d6b8... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:360001) as https://chromium.googlesource.com/chromium/src/+/c4f03f032136d23585d10951d6b8...
Message was sent while issue was closed.
On 2017/08/20 21:11:46, commit-bot: I haz the power wrote: > Committed patchset #12 (id:360001) as > https://chromium.googlesource.com/chromium/src/+/c4f03f032136d23585d10951d6b8... w00t!
Message was sent while issue was closed.
https://codereview.chromium.org/2904823002/diff/360001/chrome/browser/ui/view... File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/360001/chrome/browser/ui/view... chrome/browser/ui/views/try_chrome_dialog.cc:333: run_loop_->QuitWhenIdle(); should there be "if (run_loop_)" ahead of this since run_loop_ will be an empty pointer in tests? |