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

Issue 11440020: Add an outdated upgrade bubble view. (Closed)

Created:
8 years ago by MAD
Modified:
7 years, 10 months ago
CC:
chromium-reviews, tfarina, Ben Goodger (Google)
Visibility:
Public.

Description

Add an outdated upgrade bubble view. Show a new bubble when the current install is more than on major revision away from what's available. BUG=151996 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182626

Patch Set 1 : #

Total comments: 59

Patch Set 2 : Sync'd to ToT #

Patch Set 3 : Addressed CR comments... #

Total comments: 36

Patch Set 4 : More CR comments addressed and now a testable version without current time test yet. #

Total comments: 7

Patch Set 5 : First complete version, with more CR comments addressed #

Patch Set 6 : Addressed compile issues on non Windows platforms #

Patch Set 7 : Sync to ToT #

Patch Set 8 : One more cross-platform compile issue... damn! #

Patch Set 9 : Exclude Chrome OS correctly #

Total comments: 16

Patch Set 10 : More CR comments, minimized URLFetches and updated to new spec of poping bubble from menu. #

Patch Set 11 : Added Field Trial. #

Patch Set 12 : Sync'd to ToT. #

Patch Set 13 : Only check for policy on CHROME_BUILD. #

Total comments: 37

Patch Set 14 : More and More CR comments addressed. #

Patch Set 15 : Sync'd to ToT. #

Patch Set 16 : Some more self-CR cleanups #

Total comments: 22

Patch Set 17 : Refactoring based on latest review comments. #

Total comments: 41

Patch Set 18 : Post-refactoring fine tuning. #

Patch Set 19 : Sync'd to ToT. #

Total comments: 23

Patch Set 20 : Yet some more CR addressed, mostly nits. #

Patch Set 21 : Sync to ToT... #

Total comments: 4

Patch Set 22 : Another sync to ToT. #

Patch Set 23 : Some nits fix! #

Patch Set 24 : Comments updates. #

Patch Set 25 : Enabled variations service network time fetch + Sync to ToT. #

Total comments: 4

Patch Set 26 : Updated comments based on CR nits and Sync'd to ToT. #

Patch Set 27 : #

Patch Set 28 : Sync'd to ToT #

Total comments: 6

Patch Set 29 : Addressed OWNERS comments... #

Unified diffs Side-by-side diffs Delta from patch set Stats (+625 lines, -80 lines) Patch
M chrome/app/generated_resources.grd View 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_commands.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +12 lines, -2 lines 0 comments Download
M chrome/browser/ui/toolbar/wrench_menu_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 3 chunks +17 lines, -2 lines 0 comments Download
A chrome/browser/ui/views/outdated_upgrade_bubble_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +78 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/outdated_upgrade_bubble_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +202 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/toolbar_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/toolbar_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 4 chunks +13 lines, -0 lines 0 comments Download
M chrome/browser/upgrade_detector.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 3 chunks +18 lines, -6 lines 0 comments Download
M chrome/browser/upgrade_detector.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 2 chunks +20 lines, -8 lines 0 comments Download
M chrome/browser/upgrade_detector_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +19 lines, -1 line 0 comments Download
M chrome/browser/upgrade_detector_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 8 chunks +213 lines, -61 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_notification_types.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
MAD
Hey Finnur, seems like you wrote the critical update bubble, can you review this new ...
8 years ago (2012-12-06 00:46:47 UTC) #1
Finnur
General comments: Would like to see screenshot of it in action. Suggest a TEST= line ...
8 years ago (2012-12-06 19:53:16 UTC) #2
MAD
Thanks for the comments... Sorry about the long delay, we have been discussing the specification ...
7 years, 11 months ago (2013-01-22 15:18:29 UTC) #3
Finnur
https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/upgrade_bubble_view.cc File chrome/browser/ui/views/upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/upgrade_bubble_view.cc#newcode72 chrome/browser/ui/views/upgrade_bubble_view.cc:72: HandleButtonPressed(later_button_); Again, check with the owner. Maybe they put ...
7 years, 11 months ago (2013-01-23 10:39:16 UTC) #4
MAD
Addressed/answered all comments and also made the code manually testable... Though not yet complete... I ...
7 years, 11 months ago (2013-01-23 20:57:59 UTC) #5
Finnur
https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc#newcode46 chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:46: DCHECK(!IsShowing()); I kind of liked that function. It made ...
7 years, 11 months ago (2013-01-24 10:13:32 UTC) #6
MAD
OK, thanks again... This new version should be complete now... Let me know if you ...
7 years, 11 months ago (2013-01-24 17:29:34 UTC) #7
Finnur
https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser_commands.cc File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser_commands.cc#newcode920 chrome/browser/ui/browser_commands.cc:920: content::UserMetricsAction("OutdatedUpgradeReinstall")); I was thinking either making the UMA metric ...
7 years, 11 months ago (2013-01-25 11:39:22 UTC) #8
MAD
OK, one more round... Sorry about the extra changes but some of your comments spawn ...
7 years, 10 months ago (2013-01-29 21:13:10 UTC) #9
Finnur
https://codereview.chromium.org/11440020/diff/42017/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/42017/chrome/app/generated_resources.grd#newcode8616 chrome/app/generated_resources.grd:8616: + <ph name="IDS_SHORT_PRODUCT_NAME">$1<ex>Chrome</ex></ph> auto-update error Don't we have different ...
7 years, 10 months ago (2013-01-30 15:50:48 UTC) #10
MAD
One more round... With a pending dependency on a change currently under review at 12096096. ...
7 years, 10 months ago (2013-01-31 21:31:42 UTC) #11
MAD
In case it would need explaining, the reason I now use a static member function ...
7 years, 10 months ago (2013-02-01 12:20:54 UTC) #12
Finnur
First of all, my apologies for replying so late. I got sidetracked into another review ...
7 years, 10 months ago (2013-02-01 17:09:15 UTC) #13
MAD
How about this? https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_detector.h File chrome/browser/upgrade_detector.h (right): https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_detector.h#newcode52 chrome/browser/upgrade_detector.h:52: // Whether the upgrade is due ...
7 years, 10 months ago (2013-02-02 03:55:33 UTC) #14
Finnur
https://codereview.chromium.org/11440020/diff/62003/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/62003/chrome/app/generated_resources.grd#newcode14785 chrome/app/generated_resources.grd:14785: + nit: There are three whitespace changes here, two ...
7 years, 10 months ago (2013-02-04 11:12:04 UTC) #15
MAD
Here are some answers to you comments without a code update just yet, because I'm ...
7 years, 10 months ago (2013-02-04 16:01:29 UTC) #16
MAD
OK, as mentioned in the new answers. I couldn't find a clean way to do ...
7 years, 10 months ago (2013-02-04 17:17:02 UTC) #17
Finnur
https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_detector_impl.cc File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_detector_impl.cc#newcode265 chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) I'll take a closer look at the ...
7 years, 10 months ago (2013-02-04 21:30:17 UTC) #18
MAD
Thanks a lot, I don't know if you noticed, but it's not as much a ...
7 years, 10 months ago (2013-02-04 22:24:37 UTC) #19
Finnur
https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc#newcode173 chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } If it were me, I'd probably want to ...
7 years, 10 months ago (2013-02-05 10:52:19 UTC) #20
Finnur
> Thanks a lot, I don't know if you noticed, but it's not as much ...
7 years, 10 months ago (2013-02-05 10:52:57 UTC) #21
MAD
Addressed/answered your comments... back to you. Thanks again! https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc#newcode173 chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } ...
7 years, 10 months ago (2013-02-05 16:49:16 UTC) #22
Finnur
https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc#newcode173 chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } Don't feel the need to wait on just ...
7 years, 10 months ago (2013-02-05 22:14:50 UTC) #23
MAD
About the metrics, Jeff said he would like to have cross session but can live ...
7 years, 10 months ago (2013-02-06 03:14:28 UTC) #24
Finnur
LGTM, with one nit and a caveat (that network sane time is commented out due ...
7 years, 10 months ago (2013-02-06 10:26:49 UTC) #25
MAD
Added requested comments and also enabled variations service network time access since it's not available... ...
7 years, 10 months ago (2013-02-06 20:49:02 UTC) #26
MAD
I just got a confirmation from the [T]PMs that we can commit this in M26, ...
7 years, 10 months ago (2013-02-06 23:37:59 UTC) #27
Finnur
Still LGTM, with a couple of nits. Over to you, Ben. https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_detector_impl.h File chrome/browser/upgrade_detector_impl.h (right): ...
7 years, 10 months ago (2013-02-07 09:17:44 UTC) #28
MAD
Thanks again... BYE MAD https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_detector_impl.h File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_detector_impl.h#newcode46 chrome/browser/upgrade_detector_impl.h:46: // method receiving a WeakPtr<> ...
7 years, 10 months ago (2013-02-07 14:46:45 UTC) #29
MAD
Ping? Ben, should I ask someone else for the OWNERS review? Thank! BYE MAD...
7 years, 10 months ago (2013-02-08 13:47:43 UTC) #30
MAD
Pong? BYE MAD :-)
7 years, 10 months ago (2013-02-12 14:04:18 UTC) #31
MAD
Ben seems too busy.... Scott, can you take care of this OWNERS review or should ...
7 years, 10 months ago (2013-02-13 14:09:40 UTC) #32
sky
Ben was out the past couple of days, but AFAIK he is doing reviews now.
7 years, 10 months ago (2013-02-13 15:41:37 UTC) #33
Ben Goodger (Google)
https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc#newcode9 chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:9: #include "chrome/browser/ui/browser_tabstrip.h" ... then you could remove these two ...
7 years, 10 months ago (2013-02-13 22:34:51 UTC) #34
Ben Goodger (Google)
https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/toolbar_view.cc File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/toolbar_view.cc#newcode191 chrome/browser/ui/views/toolbar_view.cc:191: #if defined(OS_WIN) || defined(OS_MACOSX) || \ instead of having ...
7 years, 10 months ago (2013-02-13 22:35:44 UTC) #35
MAD
All done... Thanks! Anything else? BYE MAD https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc#newcode9 chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:9: #include "chrome/browser/ui/browser_tabstrip.h" ...
7 years, 10 months ago (2013-02-14 14:04:06 UTC) #36
Ben Goodger (Google)
lgtm
7 years, 10 months ago (2013-02-14 23:23:15 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/11440020/91067
7 years, 10 months ago (2013-02-15 00:33:32 UTC) #38
commit-bot: I haz the power
7 years, 10 months ago (2013-02-15 05:54:38 UTC) #39
Message was sent while issue was closed.
Change committed as 182626

Powered by Google App Engine
This is Rietveld 408576698