Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(851)

Issue 2904823002: Inactive toast ux changes (Closed)

Created:
3 years, 7 months ago by skare_
Modified:
3 years, 4 months ago
CC:
chromium-reviews, tfarina, pennymac+watch_chromium.org, wfh+watch_chromium.org, oshima+watch_chromium.org, grt+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+399 lines, -1031 lines) Patch
M chrome/app/chromium_strings.grd View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -16 lines 0 comments Download
M chrome/app/google_chrome_strings.grd View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -16 lines 0 comments Download
M chrome/app/vector_icons/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/app/vector_icons/inactive_toast_close.icon View 1 2 3 4 5 6 1 chunk +20 lines, -0 lines 0 comments Download
A chrome/app/vector_icons/inactive_toast_logo.icon View 1 2 3 4 5 6 1 chunk +31 lines, -0 lines 0 comments Download
M chrome/browser/DEPS View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 2 chunks +13 lines, -13 lines 0 comments Download
A + chrome/browser/first_run/try_chrome_dialog_browsertest.cc View 1 2 3 4 5 6 7 4 chunks +7 lines, -7 lines 0 comments Download
D chrome/browser/first_run/try_chrome_dialog_view_browsertest.cc View 1 2 3 4 1 chunk +0 lines, -92 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/harmony/chrome_typography.h View 1 2 3 4 5 6 7 3 chunks +12 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/harmony/chrome_typography.cc View 1 2 3 4 5 6 7 8 9 2 chunks +17 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/harmony/harmony_typography_provider.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
A + chrome/browser/ui/views/try_chrome_dialog.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +59 lines, -75 lines 0 comments Download
A + chrome/browser/ui/views/try_chrome_dialog.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +232 lines, -280 lines 1 comment Download
M chrome/browser/ui/views/try_chrome_dialog_view.h View 1 2 3 4 1 chunk +0 lines, -146 lines 0 comments Download
D chrome/browser/ui/views/try_chrome_dialog_view.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -382 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 87 (33 generated)
skare
sending as fyi -- will need to integrate grt's/nikunjb's changes
3 years, 7 months ago (2017-05-24 07:00:23 UTC) #4
grt (UTC plus 2)
Hi Travis. How are things progressing here? Are you able to patch in https://codereview.chromium.org/2889323004/ and ...
3 years, 6 months ago (2017-06-13 07:19:15 UTC) #6
grt (UTC plus 2)
On 2017/06/13 07:19:15, grt (UTC plus 2) wrote: > Hi Travis. How are things progressing ...
3 years, 6 months ago (2017-06-16 12:01:03 UTC) #7
skare_
fyi, new snapshot that resolves the TODOs for calling Experiments.
3 years, 6 months ago (2017-06-22 11:09:41 UTC) #8
grt (UTC plus 2)
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode346 chrome/browser/ui/views/try_chrome_dialog_view.cc:346: installer::ExperimentStorage storage; nit: make this a member variable of ...
3 years, 6 months ago (2017-06-22 11:30:32 UTC) #9
nikunjb1
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode293 chrome/browser/ui/views/try_chrome_dialog_view.cc:293: experiment.AssignGroup(flavor_); Installer is already Assigning group, so it may ...
3 years, 6 months ago (2017-06-22 16:52:55 UTC) #10
skare_
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode293 chrome/browser/ui/views/try_chrome_dialog_view.cc:293: experiment.AssignGroup(flavor_); On 2017/06/22 16:52:55, nikunjb1 wrote: > Installer is ...
3 years, 6 months ago (2017-06-22 17:07:13 UTC) #11
nikunjb1
https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/40001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode293 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 ...
3 years, 6 months ago (2017-06-22 17:14:49 UTC) #12
skare_
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!)
3 years, 6 months ago (2017-06-22 17:30:55 UTC) #14
grt (UTC plus 2)
looking pretty good. comments below. https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_browser_main.cc#newcode1562 chrome/browser/chrome_browser_main.cc:1562: if (answer == TryChromeDialogView::NOT_NOW) ...
3 years, 6 months ago (2017-06-22 20:34:04 UTC) #15
skare_
https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2904823002/diff/80001/chrome/browser/chrome_browser_main.cc#newcode1562 chrome/browser/chrome_browser_main.cc:1562: if (answer == TryChromeDialogView::NOT_NOW) { On 2017/06/22 20:34:03, grt ...
3 years, 6 months ago (2017-06-22 23:25:18 UTC) #16
grt (UTC plus 2)
plz upload the new patch set for review. thanks!
3 years, 6 months ago (2017-06-23 08:44:44 UTC) #18
skare_
On 2017/06/23 08:44:44, grt (UTC plus 2) wrote: > plz upload the new patch set ...
3 years, 6 months ago (2017-06-23 11:50:13 UTC) #19
grt (UTC plus 2)
lgtm with nits https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode10 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/views/try_chrome_dialog_view.cc#newcode14 chrome/browser/ui/views/try_chrome_dialog_view.cc:14: #include ...
3 years, 6 months ago (2017-06-23 13:33:39 UTC) #20
skare_
https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/100001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode10 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) ...
3 years, 6 months ago (2017-06-23 14:49:30 UTC) #23
nikunjb1
lgtm
3 years, 6 months ago (2017-06-23 17:33:16 UTC) #26
nikunjb1
lgtm
3 years, 6 months ago (2017-06-23 17:33:19 UTC) #27
sky
https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme_resources.grd File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme_resources.grd#newcode219 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 ...
3 years, 6 months ago (2017-06-23 17:45:53 UTC) #28
sky
On 2017/06/23 17:45:53, sky wrote: > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme_resources.grd > File chrome/app/theme/theme_resources.grd (right): > > https://codereview.chromium.org/2904823002/diff/120001/chrome/app/theme/theme_resources.grd#newcode219 > ...
3 years, 6 months ago (2017-06-23 19:25:02 UTC) #29
skare_
Thanks! svg assets are outstanding, wanted to FYI Nik on a couple of these comments, ...
3 years, 6 months ago (2017-06-24 01:09:00 UTC) #34
grt (UTC plus 2)
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.h File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.h#newcode56 chrome/browser/ui/views/try_chrome_dialog_view.h:56: // |group| selects what strings to present and what ...
3 years, 6 months ago (2017-06-24 20:55:11 UTC) #35
sky
+tapted for explicitly setting font on a LabelButton. See first comment. https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): ...
3 years, 5 months ago (2017-06-26 16:12:06 UTC) #37
tapted
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.cc File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode112 chrome/browser/ui/views/try_chrome_dialog_view.cc:112: label()->SetFontList(gfx::FontList(kFontDefinitionSemiBold)); the font definition is "Segoe UI, Arial, Semi-Bold ...
3 years, 5 months ago (2017-06-26 22:54:25 UTC) #38
Peter Kasting
I hate to walk in and throw bombs, but why is Views drawing any of ...
3 years, 5 months ago (2017-06-26 23:12:01 UTC) #39
skare_
On 2017/06/26 23:12:01, Peter Kasting wrote: > I hate to walk in and throw bombs, ...
3 years, 5 months ago (2017-06-29 17:29:40 UTC) #40
Peter Kasting
On 2017/06/29 17:29:40, skare_ wrote: > On 2017/06/26 23:12:01, Peter Kasting wrote: > > I ...
3 years, 5 months ago (2017-06-29 17:44:59 UTC) #41
skare_
On 2017/06/23 19:25:02, sky OOO wrote: > On 2017/06/23 17:45:53, sky wrote: > > > ...
3 years, 5 months ago (2017-07-04 06:09:36 UTC) #42
skare_
On 2017/07/04 06:09:36, skare_ wrote: > On 2017/06/23 19:25:02, sky OOO wrote: > > On ...
3 years, 5 months ago (2017-07-04 06:20:36 UTC) #43
grt (UTC plus 2)
On 2017/07/04 06:20:36, skare_ wrote: > update on the native vs. views point - I ...
3 years, 5 months ago (2017-07-04 08:39:32 UTC) #44
skare_
On 2017/06/26 22:54:25, tapted wrote: > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.cc > File chrome/browser/ui/views/try_chrome_dialog_view.cc (right): > > https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.cc#newcode112 > ...
3 years, 5 months ago (2017-07-06 07:31:28 UTC) #45
tapted
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/views/try_chrome_dialog_view.cc ...
3 years, 5 months ago (2017-07-06 07:52:23 UTC) #46
skare_
On 2017/07/06 07:52:23, tapted wrote: > On 2017/07/06 07:31:28, skare_ wrote: > > On 2017/06/26 ...
3 years, 5 months ago (2017-07-07 01:24:23 UTC) #47
tapted
the typography approach looks reasonable to me - hopefully sky is on board with it ...
3 years, 5 months ago (2017-07-07 01:51:21 UTC) #48
skare_
On 2017/07/07 01:51:21, tapted wrote: > the typography approach looks reasonable to me - hopefully ...
3 years, 5 months ago (2017-07-07 02:56:44 UTC) #51
skare_
https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/views/harmony/chrome_typography.h File chrome/browser/ui/views/harmony/chrome_typography.h (right): https://codereview.chromium.org/2904823002/diff/240001/chrome/browser/ui/views/harmony/chrome_typography.h#newcode33 chrome/browser/ui/views/harmony/chrome_typography.h:33: // Appropriate comment On 2017/07/07 01:51:21, tapted wrote: > ...
3 years, 5 months ago (2017-07-07 03:04:10 UTC) #52
tapted
lgtm with some nits https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/harmony/chrome_typography.cc File chrome/browser/ui/views/harmony/chrome_typography.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/harmony/chrome_typography.cc#newcode51 chrome/browser/ui/views/harmony/chrome_typography.cc:51: // Sets the |size_delta| and ...
3 years, 5 months ago (2017-07-07 03:32:20 UTC) #53
skare_
On 2017/07/07 03:32:20, tapted wrote: > lgtm with some nits > > https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/harmony/chrome_typography.cc > File ...
3 years, 5 months ago (2017-07-18 01:51:46 UTC) #54
sky
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.h File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.h#newcode115 chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; On 2017/06/24 01:09:00, skare_ wrote: > On ...
3 years, 5 months ago (2017-07-18 17:10:32 UTC) #55
grt (UTC plus 2)
https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/try_chrome_dialog.cc File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/try_chrome_dialog.cc#newcode127 chrome/browser/ui/views/try_chrome_dialog.cc:127: if (group >= arraysize(kExperiments)) { On 2017/07/18 17:10:32, sky ...
3 years, 5 months ago (2017-07-20 05:58:58 UTC) #56
sky
https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/try_chrome_dialog.cc File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/try_chrome_dialog.cc#newcode127 chrome/browser/ui/views/try_chrome_dialog.cc:127: if (group >= arraysize(kExperiments)) { On 2017/07/20 05:58:58, grt ...
3 years, 5 months ago (2017-07-20 17:33:01 UTC) #57
skare_
https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.h File chrome/browser/ui/views/try_chrome_dialog_view.h (right): https://codereview.chromium.org/2904823002/diff/120001/chrome/browser/ui/views/try_chrome_dialog_view.h#newcode115 chrome/browser/ui/views/try_chrome_dialog_view.h:115: base::Time time_shown_; On 2017/07/18 17:10:31, sky wrote: > On ...
3 years, 5 months ago (2017-07-21 03:11:36 UTC) #60
grt (UTC plus 2)
https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/views/try_chrome_dialog.cc File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/280001/chrome/browser/ui/views/try_chrome_dialog.cc#newcode122 chrome/browser/ui/views/try_chrome_dialog.cc:122: if (group > arraysize(kExperiments)) { ">=" since |group| is ...
3 years, 5 months ago (2017-07-21 08:15:06 UTC) #63
sky
https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/try_chrome_dialog.cc File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/try_chrome_dialog.cc#newcode283 chrome/browser/ui/views/try_chrome_dialog.cc:283: // If the dialog is not modal, we don't ...
3 years, 5 months ago (2017-07-21 14:39:36 UTC) #64
skare_
sky@ - apologies, thought I addressed your nested runloop question earlier. Rebased, addressed comments: https://codereview.chromium.org/2904823002/diff/260001/chrome/browser/ui/views/try_chrome_dialog.cc ...
3 years, 4 months ago (2017-08-17 03:01:44 UTC) #65
grt (UTC plus 2)
looks pretty good, and appears in the right place on my HiDPI display. https://chromium-review.googlesource.com/c/618688 is ...
3 years, 4 months ago (2017-08-17 11:26:33 UTC) #66
skare_
Hi Greg - here are comments on your comments. I'll need at least one more ...
3 years, 4 months ago (2017-08-18 11:37:34 UTC) #67
grt (UTC plus 2)
lgtm w/ nits and a q https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/views/try_chrome_dialog.cc File chrome/browser/ui/views/try_chrome_dialog.cc (right): https://codereview.chromium.org/2904823002/diff/300001/chrome/browser/ui/views/try_chrome_dialog.cc#newcode90 chrome/browser/ui/views/try_chrome_dialog.cc:90: {IDS_WIN10_TOAST_SWITCH_SMART_AND_SECURE, IDS_WIN10_TOAST_RECOMMENDATION, On ...
3 years, 4 months ago (2017-08-18 12:05:24 UTC) #68
skare_
@sky, welcome comments on views items. I commented on the earlier nested run loop question, ...
3 years, 4 months ago (2017-08-18 20:25:20 UTC) #69
sky
Thanks for the comments, and it makes sense. LGTM
3 years, 4 months ago (2017-08-18 22:31:29 UTC) #70
skare_
thanks for reviews, everyone FYI, moved run_loop_ to a unique_ptr<> so its ctor doesn't run ...
3 years, 4 months ago (2017-08-20 19:01:35 UTC) #77
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2904823002/360001
3 years, 4 months ago (2017-08-20 21:05:42 UTC) #82
commit-bot: I haz the power
Committed patchset #12 (id:360001) as https://chromium.googlesource.com/chromium/src/+/c4f03f032136d23585d10951d6b855b40c83431c
3 years, 4 months ago (2017-08-20 21:11:46 UTC) #85
grt (UTC plus 2)
On 2017/08/20 21:11:46, commit-bot: I haz the power wrote: > Committed patchset #12 (id:360001) as ...
3 years, 4 months ago (2017-08-21 10:28:26 UTC) #86
grt (UTC plus 2)
3 years, 4 months ago (2017-08-21 12:04:45 UTC) #87
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?

Powered by Google App Engine
This is Rietveld 408576698