|
|
Created:
8 years ago by MAD Modified:
7 years, 10 months ago CC:
chromium-reviews, tfarina, Ben Goodger (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd 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... #Messages
Total messages: 39 (0 generated)
Hey Finnur, seems like you wrote the critical update bubble, can you review this new "outdated upgrade" bubble we want to add? There's a spec for it here: https://docs.google.com/a/google.com/document/d/1Wkxc2VIvmkN82er-hSL9IDE0Belq... and some minor discussions about it in the associated crbug issue. Let me know if you think I should ask someone else for the Code Review... Takk fyrir mig! BYE MAD P.S.: Note that we may decide to add some plumbing to prevent the "Update Google Chrome" option to be added to the wrench menu while still showing the warning menu icon. This is still TBD whether we want it in this CL or in a subsequent one. I would still like you to start looking at the CL to make sure it's in the right direction so far.
General comments: Would like to see screenshot of it in action. Suggest a TEST= line in the CL description. Rest below. Verði þér að góðu! :) https://codereview.chromium.org/11440020/diff/2001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:8460: + Reinstall Chrome I realize auto-update is Chrome only, but I forget... is this where we've been putting strings that we don't have separate Chrome/Chromium versions for? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/to... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/to... chrome/browser/ui/views/toolbar_view.cc:560: ShowOutdatedInstallNotification(); So... Are we certain that we can not have both this and the critical notification showing at the same time? What if the user has been updated in the background for three major versions of Chrome in a row (including a critical notification) and not restarted in the meantime? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... File chrome/browser/ui/views/upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:25: // Fixed with of the text label with the description of the bubble. nit: Technically, this is the fixed width of the _column_ that contains the text label (text will differ on width based on locale). https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:26: const int kWidthOfDescriptionText = 330; I don't know how much room there is to spare, but are you sure you have enough width to cover locales such as German, which is often more verbose? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:40: return; Under what conditions does this happen? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:72: HandleButtonPressed(later_button_); I maybe totally off here, but I don't remember having to do this. I would have thought that if you pressed Enter when the button focused that that would generate a click on the button without you having to explicitly plumb it through. Is that not the case? Or was that only for the spacebar? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:86: reinstall_button_->SetIsDefault(true); Just wondering... Reinstall currently just takes you to the install page in a new tab, right? If we were to change that and reinstall, wouldn't this become potentially a data-loss situation if you are writing a comment on a blog and the bubble appears just as you hit Enter? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:131: cs->AddPaddingColumn(0, views::kRelatedButtonHSpacing - 2); nit: Make a constant at top and move the comment up to accompany it. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:145: layout->StartRow(0, 2); You should make local consts with descriptive names (kHeadlineColumnId or something) out of those column ids and use them as params to StartRow instead of 0, 1, 2. See const on line 124, for example. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:163: HandleButtonPressed(sender); I would probably say we should not be clicking the button on say right-click, only on left-click. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:169: content::UserMetricsAction("UpgradeBubble_Reinstall")); Same comment applies here (this is bit too general). AutoUpgradeErrorBubble_Reinstall, perhaps? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:176: content::UserMetricsAction("UpgradeBubble_Later")); Ditto. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... File chrome/browser/ui/views/upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:19: // upgrade is long overdue. Don't create a UpgradeBubbleView directly, nit: Perhaps reword: UpgradeBubbleView is a view that warns the user that an upgrade is long overdue. It is intended to be used as the content of a bubble anchored off of the Chrome toolbar. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:21: class UpgradeBubbleView : public views::BubbleDelegateView, Hmm... I find this name a little bit too general as the bubble is more specific than that and there is potential for people to confuse this with the CriticalUpgrade bubble view. Are there other names we could use, that are more specific? Such as AutoUpgradeErrorBubbleView (or something to that effect)? If you change it, remember to change the comment above, too. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:26: virtual ~UpgradeBubbleView(); I forget... Didn't we have a rule of thumb to move destructors to be private for things we didn't want people to construct directly? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:39: virtual void Init() OVERRIDE; Why protected? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:45: UpgradeBubbleView(views::View* anchor_view, Browser* browser); Constructor before static methods, according to style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:47: // Overridden from views::ButtonListener: nit: You've used 'foo method.' convention above, so I suggest sticking to that. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:61: // Button to be reminded later. nit: What does the button need to be reminded of? :) Suggest rewording. Same goes for the reinstall_button_ above. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:64: // The browser to use for opening the download Chrome URL. nit: Suggest capitalize d in Download https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:77: // This code can be used by both regular and outdated upgrades. nit: suggest: by both regular upgrades and for checking for severely outdated version. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:168: #if defined(OS_WIN) I would probably have moved this to a _win.cc file, rather than have it in a cross-platform file, but would probably also suggest that for another CL to keep this review simpler. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:203: // The google update class is never supposed to sent these two to listeners. nit: s/sent/send/ https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:227: got_available_version_ = true; This variable is actually misnamed because you don't always get an available version number, right? Same with got_installed_version. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:236: // We only check for outdated when there are no group policies preventing s/outdated/outdated version/ https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:244: // An uninitialized install version prevents an outdated notification. Not sure I understand this comment... https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:261: void CheckForOutdated() { nit: Suggest: CheckIfOutdatedVersion or NotifyIfOutdatedVersion https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.h:39: void NotifyOutdated(); Suggest NotifyOutdatedVersion or NotifyOutdatedChromeVersion, or something.
Thanks for the comments... Sorry about the long delay, we have been discussing the specification and changed our mind about M25, so I moved to other more urgent 1993 issues... Now we want this in M26, so I'm back at it and addressed your comments, mainly in the bubble view code, since the upgrade detector code will change a lot (it's only partway there now, so you can ignore it)... Also, I changed the name of the bubble class as you suggested and also changed the name of the file, but rietveld doesn't see the move correctly so you won't be able to see the diff... I could rename the file back to what it was just for the review and rename it again if you want... Note that I still have some work to do with this CL but I wanted to send you an upload with the changes inspired by you review comments... Feel free to ignore this and wait for the final CL... Or let me know if you have other comments about the bubble or would like me to try to bring back the file names so you see the incremental diffs... Again, thanks! BYE MAD https://codereview.chromium.org/11440020/diff/2001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:8460: + Reinstall Chrome You're right, I didn't know... I'll move these to google_chrome_strings.grd https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/to... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/to... chrome/browser/ui/views/toolbar_view.cc:560: ShowOutdatedInstallNotification(); Thanks, I asked the PMs, but I guess the outdated one would win since if we're outdated, the critical update is not likely to work... Right? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... File chrome/browser/ui/views/upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:25: // Fixed with of the text label with the description of the bubble. On 2012/12/06 19:53:16, Finnur wrote: > nit: Technically, this is the fixed width of the _column_ that contains the text > label (text will differ on width based on locale). Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:26: const int kWidthOfDescriptionText = 330; On 2012/12/06 19:53:16, Finnur wrote: > I don't know how much room there is to spare, but are you sure you have enough > width to cover locales such as German, which is often more verbose? Good point, I will leave a TODO in the mean time. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:40: return; On 2012/12/06 19:53:16, Finnur wrote: > Under what conditions does this happen? Again, stolen from bookmark bar, but the IsShowing() and global instance is now becoming useful to avoid having the critical update notification at the same time as this bubble. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:72: HandleButtonPressed(later_button_); On 2012/12/06 19:53:16, Finnur wrote: > I maybe totally off here, but I don't remember having to do this. I would have > thought that if you pressed Enter when the button focused that that would > generate a click on the button without you having to explicitly plumb it > through. Is that not the case? Or was that only for the spacebar? Again, stolen from bookmark bar... But you are right, not needed... https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:86: reinstall_button_->SetIsDefault(true); On 2012/12/06 19:53:16, Finnur wrote: > Just wondering... Reinstall currently just takes you to the install page in a > new tab, right? If we were to change that and reinstall, wouldn't this become > potentially a data-loss situation if you are writing a comment on a blog and the > bubble appears just as you hit Enter? The plan is to always go to an install page not do an automatic reinstall since this is for case where automatic installs stop working... But you could state your concern in the design doc here: https://docs.google.com/a/google.com/document/d/1Wkxc2VIvmkN82er-hSL9IDE0Belq... https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:131: cs->AddPaddingColumn(0, views::kRelatedButtonHSpacing - 2); On 2012/12/06 19:53:16, Finnur wrote: > nit: Make a constant at top and move the comment up to accompany it. Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:145: layout->StartRow(0, 2); On 2012/12/06 19:53:16, Finnur wrote: > You should make local consts with descriptive names (kHeadlineColumnId or > something) out of those column ids and use them as params to StartRow instead of > 0, 1, 2. See const on line 124, for example. Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:163: HandleButtonPressed(sender); On 2012/12/06 19:53:16, Finnur wrote: > I would probably say we should not be clicking the button on say right-click, > only on left-click. CriticalNotificationBubbleView::ButtonPressed doesn't seem to do this... Do you want me to fix it there too? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:169: content::UserMetricsAction("UpgradeBubble_Reinstall")); On 2012/12/06 19:53:16, Finnur wrote: > Same comment applies here (this is bit too general). > AutoUpgradeErrorBubble_Reinstall, perhaps? Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:176: content::UserMetricsAction("UpgradeBubble_Later")); On 2012/12/06 19:53:16, Finnur wrote: > Ditto. Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... File chrome/browser/ui/views/upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:19: // upgrade is long overdue. Don't create a UpgradeBubbleView directly, On 2012/12/06 19:53:16, Finnur wrote: > nit: Perhaps reword: > UpgradeBubbleView is a view that warns the user that an upgrade is long overdue. > It is intended to be used as the content of a bubble anchored off of the Chrome > toolbar. Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:21: class UpgradeBubbleView : public views::BubbleDelegateView, On 2012/12/06 19:53:16, Finnur wrote: > Hmm... I find this name a little bit too general as the bubble is more specific > than that and there is potential for people to confuse this with the > CriticalUpgrade bubble view. Are there other names we could use, that are more > specific? Such as AutoUpgradeErrorBubbleView (or something to that effect)? If > you change it, remember to change the comment above, too. Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:26: virtual ~UpgradeBubbleView(); On 2012/12/06 19:53:16, Finnur wrote: > I forget... Didn't we have a rule of thumb to move destructors to be private for > things we didn't want people to construct directly? Done. I just followed what was done with the bookmark bubble view, I guess I should have started with the critical update one instead... https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:39: virtual void Init() OVERRIDE; On 2012/12/06 19:53:16, Finnur wrote: > Why protected? Done. Again, as is done in bookmark bubble view, should I fix it there too? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:45: UpgradeBubbleView(views::View* anchor_view, Browser* browser); On 2012/12/06 19:53:16, Finnur wrote: > Constructor before static methods, according to style guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:47: // Overridden from views::ButtonListener: On 2012/12/06 19:53:16, Finnur wrote: > nit: You've used 'foo method.' convention above, so I suggest sticking to that. Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:61: // Button to be reminded later. On 2012/12/06 19:53:16, Finnur wrote: > nit: What does the button need to be reminded of? :) Suggest rewording. Same > goes for the reinstall_button_ above. Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:64: // The browser to use for opening the download Chrome URL. On 2012/12/06 19:53:16, Finnur wrote: > nit: Suggest capitalize d in Download Done. https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:77: // This code can be used by both regular and outdated upgrades. On 2012/12/06 19:53:16, Finnur wrote: > nit: suggest: by both regular upgrades and for checking for severely outdated > version. This is gone... Reworking the whole thing... https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:168: #if defined(OS_WIN) On 2012/12/06 19:53:16, Finnur wrote: > I would probably have moved this to a _win.cc file, rather than have it in a > cross-platform file, but would probably also suggest that for another CL to keep > this review simpler. Gone! https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:203: // The google update class is never supposed to sent these two to listeners. On 2012/12/06 19:53:16, Finnur wrote: > nit: s/sent/send/ Gone! https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:227: got_available_version_ = true; On 2012/12/06 19:53:16, Finnur wrote: > This variable is actually misnamed because you don't always get an available > version number, right? Same with got_installed_version. Gone! https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:236: // We only check for outdated when there are no group policies preventing On 2012/12/06 19:53:16, Finnur wrote: > s/outdated/outdated version/ Gone! https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:244: // An uninitialized install version prevents an outdated notification. On 2012/12/06 19:53:16, Finnur wrote: > Not sure I understand this comment... Gone! https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.cc:261: void CheckForOutdated() { On 2012/12/06 19:53:16, Finnur wrote: > nit: Suggest: CheckIfOutdatedVersion or NotifyIfOutdatedVersion Gone! https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/upgrade_det... chrome/browser/upgrade_detector_impl.h:39: void NotifyOutdated(); On 2012/12/06 19:53:16, Finnur wrote: > Suggest NotifyOutdatedVersion or NotifyOutdatedChromeVersion, or something. Done.
https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... File chrome/browser/ui/views/upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:72: HandleButtonPressed(later_button_); Again, check with the owner. Maybe they put this in because of ChromeOS or something. I'm not sure. On 2013/01/22 15:18:29, MAD wrote: > On 2012/12/06 19:53:16, Finnur wrote: > > I maybe totally off here, but I don't remember having to do this. I would have > > thought that if you pressed Enter when the button focused that that would > > generate a click on the button without you having to explicitly plumb it > > through. Is that not the case? Or was that only for the spacebar? > > Again, stolen from bookmark bar... But you are right, not needed... https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.cc:163: HandleButtonPressed(sender); That would be good, thank you (can be another CL). On 2013/01/22 15:18:29, MAD wrote: > On 2012/12/06 19:53:16, Finnur wrote: > > I would probably say we should not be clicking the button on say right-click, > > only on left-click. > > CriticalNotificationBubbleView::ButtonPressed doesn't seem to do this... Do you > want me to fix it there too? https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... File chrome/browser/ui/views/upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/2001/chrome/browser/ui/views/up... chrome/browser/ui/views/upgrade_bubble_view.h:39: virtual void Init() OVERRIDE; So... I would probably contact the owner of that view and ask if you should fix the issues that stem from that class being the basis of this class. Maybe there is a reason. Maybe it is just an error and should be fixed in both places. On 2013/01/22 15:18:29, MAD wrote: > On 2012/12/06 19:53:16, Finnur wrote: > > Why protected? > > Done. Again, as is done in bookmark bubble view, should I fix it there too? https://codereview.chromium.org/11440020/diff/20008/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/20008/chrome/app/generated_reso... chrome/app/generated_resources.grd:8569: + <!-- Upgrade bubble messages --> You mentioned in previous round that you'd move those to chrome_strings.grd. I'm not sure what is the right thing to do, but if we keep them here then at a minimum you should note that these strings are not used in Chromium. https://codereview.chromium.org/11440020/diff/20008/chrome/app/generated_reso... chrome/app/generated_resources.grd:8571: + Reinstall Chrome You should use a param for Chrome and specify IDS_PRODUCT_NAME_SHORT at call site. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:26: // TODO(mad): Make sure there is enought room for all languages. s/enought/enough. Out of curiosity, were you planning to take a stab at this when the translations are in? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:33: // to bring the separation visually in line with the row separation seperation height? Do you mean separator height? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:66: // destroyed asynchronously and the shown state will be checked before then. What is the shown state referring to? Is this from the bookmark view? |bubble_| does not exist... I wonder if this is needed in this bubble? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:69: } nit: Weird indentation. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:92: image_view->SetImage(rb.GetImageSkiaNamed(IDR_UPDATE_MENU3)); What was the reasoning behind UPDATE_MENU3 and not a different number (or a custom icon)? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:97: static const int kIconTitleColumnSetId = 0; Why not just const int? (same below) https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Happy new year! :) https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:17: // OutdatedUpgradeBubbleView is a view that warns the user that an upgrade is nit: 'is a view that' is redundant. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:41: // Closes the bubble or opens the download Chrome URL. nit: I'd probably drop this comment (line 41). https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:51: // Button for the user to reinstall Chrome now, by navigating to the nit: More concise and to the point: // Button that takes the user to the Chrome download page. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:160: remove? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:190: #endif remove? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:39: void NotifyOutdatedChromeVersio(); s/Versio/Version/ But no code in this patch calls this?? Are there files missing from this CL?
Addressed/answered all comments and also made the code manually testable... Though not yet complete... I still need to get the current "sane time" (e.g., using a URLRequest to a secured connection to Google) and launch the bubble when the we're 12 weeks passed the build date... Almost there... Takk fyrir þolinmæðina! :-) BYE MAD https://codereview.chromium.org/11440020/diff/20008/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/20008/chrome/app/generated_reso... chrome/app/generated_resources.grd:8569: + <!-- Upgrade bubble messages --> Yeah, I said I will, and I will, I just didn't get there yet... As I mentioned in the update comments, I still have some work to do, I mainly wanted to answer your comments about the bubble... The rest is coming... :-) https://codereview.chromium.org/11440020/diff/20008/chrome/app/generated_reso... chrome/app/generated_resources.grd:8571: + Reinstall Chrome Done... I should have thought about that... Thanks! https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:26: // TODO(mad): Make sure there is enought room for all languages. Yes... https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:33: // to bring the separation visually in line with the row separation On 2013/01/23 10:39:17, Finnur wrote: > seperation height? Do you mean separator height? Again, I stole this from the bookmark bubble, I took the comment as is... :-( https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:66: // destroyed asynchronously and the shown state will be checked before then. On 2013/01/23 10:39:17, Finnur wrote: > What is the shown state referring to? Is this from the bookmark view? > > |bubble_| does not exist... > > I wonder if this is needed in this bubble? Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:69: } On 2013/01/23 10:39:17, Finnur wrote: > nit: Weird indentation. Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:92: image_view->SetImage(rb.GetImageSkiaNamed(IDR_UPDATE_MENU3)); The icon was chosen by the UI designers... https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:97: static const int kIconTitleColumnSetId = 0; On 2013/01/23 10:39:17, Finnur wrote: > Why not just const int? (same below) Why not static? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/23 10:39:17, Finnur wrote: > Happy new year! :) Hamingjusamur Nýtt Ár :-) https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:17: // OutdatedUpgradeBubbleView is a view that warns the user that an upgrade is On 2013/01/23 10:39:17, Finnur wrote: > nit: 'is a view that' is redundant. Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:41: // Closes the bubble or opens the download Chrome URL. On 2013/01/23 10:39:17, Finnur wrote: > nit: I'd probably drop this comment (line 41). Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:51: // Button for the user to reinstall Chrome now, by navigating to the On 2013/01/23 10:39:17, Finnur wrote: > nit: More concise and to the point: > // Button that takes the user to the Chrome download page. Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:160: On 2013/01/23 10:39:17, Finnur wrote: > remove? Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:190: #endif On 2013/01/23 10:39:17, Finnur wrote: > remove? Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:39: void NotifyOutdatedChromeVersio(); On 2013/01/23 10:39:17, Finnur wrote: > s/Versio/Version/ > > But no code in this patch calls this?? Are there files missing from this CL? Done. As I said in the overall patch comments... This is not complete yet... I mainly wanted to validate the bubble updates with this patch.
https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:46: DCHECK(!IsShowing()); I kind of liked that function. It made the code more readable. Any reason you've decided to inline expand it instead? :) https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:97: static const int kIconTitleColumnSetId = 0; Mark Mentovai addressed this in an email thread on chromium-dev on November 15th, 2012: >> Lambros Lambrou wrote: >> // Should this be 'static'? >> const int kThings = 42; > Mark Mentovai wrote: > Here, static doesn’t actually contribute anything*. > Write “const int kThings;”. “static” is a no-op and a distraction. On 2013/01/23 20:57:59, MAD wrote: > On 2013/01/23 10:39:17, Finnur wrote: > > Why not just const int? (same below) > > Why not static? https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: It's Gleðilegt nýtt ár! :) On 2013/01/23 20:57:59, MAD wrote: > On 2013/01/23 10:39:17, Finnur wrote: > > Happy new year! :) > > Hamingjusamur Nýtt Ár :-) > https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser... chrome/browser/ui/browser_commands.cc:920: content::UserMetricsAction("OutdatedUpgradeReinstall")); This is a general purpose browser commands file, so it feels weird to send out a OutdatedUpgradeReinstall. What if some other code starts using this function? https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:903: OutdatedUpgradeBubbleView::ShowBubble(app_menu_, browser_); This is now included on ChromeOS, which updates differently than Chrome. Is this intended? https://codereview.chromium.org/11440020/diff/17004/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector.h (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:56: bool IsOutdatedInstall() const { Are you sure we consider one && and one ! so heavyweight that we forgo the unix hacker style (is_outdated_install())?
OK, thanks again... This new version should be complete now... Let me know if you have other comments... BYE MAD https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:46: DCHECK(!IsShowing()); On 2013/01/24 10:13:32, Finnur wrote: > I kind of liked that function. It made the code more readable. Any reason you've > decided to inline expand it instead? :) Because I use it in a single place within the class, whereas I used to have it exposed to the toolbar view could call it... I'll bring it back... :-) https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:97: static const int kIconTitleColumnSetId = 0; On 2013/01/24 10:13:32, Finnur wrote: > Mark Mentovai addressed this in an email thread on chromium-dev on November > 15th, 2012: > > >> Lambros Lambrou wrote: > >> // Should this be 'static'? > >> const int kThings = 42; > > > Mark Mentovai wrote: > > Here, static doesn’t actually contribute anything*. > > Write “const int kThings;”. “static” is a no-op and a distraction. > > > On 2013/01/23 20:57:59, MAD wrote: > > On 2013/01/23 10:39:17, Finnur wrote: > > > Why not just const int? (same below) > > > > Why not static? > I guess I missed that thread... Thanks! Done. https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/20008/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. :-) https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser... chrome/browser/ui/browser_commands.cc:920: content::UserMetricsAction("OutdatedUpgradeReinstall")); On 2013/01/24 10:13:33, Finnur wrote: > This is a general purpose browser commands file, so it feels weird to send out a > OutdatedUpgradeReinstall. What if some other code starts using this function? I thought actually that it might be useful to have it there in case some other use cases may need it, but I think you are right, I'll inline it below... Unless you prefer we move this somewhere else... https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:903: OutdatedUpgradeBubbleView::ShowBubble(app_menu_, browser_); On 2013/01/24 10:13:33, Finnur wrote: > This is now included on ChromeOS, which updates differently than Chrome. Is this > intended? No, wasn't intended, thanks! Done. https://codereview.chromium.org/11440020/diff/17004/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector.h (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:56: bool IsOutdatedInstall() const { On 2013/01/24 10:13:33, Finnur wrote: > Are you sure we consider one && and one ! so heavyweight that we forgo the unix > hacker style (is_outdated_install())? I thought hacker style was just when we return the variable as is, but you are right, again :-), the coding style just says simple. Takk... again... :-)
https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/11440020/diff/17004/chrome/browser/ui/browser... chrome/browser/ui/browser_commands.cc:920: content::UserMetricsAction("OutdatedUpgradeReinstall")); I was thinking either making the UMA metric more general (because this function is in a general location and might be needed for uses other than outdated upgrades) or move the UMA metric call to 'outdated upgrade'-code that uses it. But I guess this change is OK too. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/12029/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:25: static bool IsShowing() { return upgrade_bubble_ != NULL; } Doesn't need to be public now. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:58: const char kTimeServerURL[] = "https://www.google.com"; This worries me. Chrome's user base is, as you know, extremely large and I doubt this is an approved way of getting the accurate time for such a large number of users. You should look up the thread "Accurate clocks and Chromium" on chromium-dev, because at a glance it looks like akalin was coming up with a more general purpose solution to this problem back in August of last year. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:188: GoogleUpdateSettings::AUTOMATIC_UPDATES) { I didn't think that the 'prevent any updates' enterprise policy was Windows only. Is that the case? https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:225: UpgradeDetected(); I don't quite grok this... If we fail to parse the simulated build date from the command line (result == false), we assume the build is outdated and trigger the notification. Huh? Is this so you can either specify a time (and wait for it to prompt you) or no time (and have it prompt you immediately)? Can you perhaps comment what you are after here with the outer if clause (line 213) and the inner if-else clause (line 218)? https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:272: #else // defined(OS_WIN) nit: shouldn't this just be on the endif? https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:370: DVLOG(1) << "Variations server request failed."; Of course, this might be a moot point (if you switch away from this method) but why is it called Variations server? And is there no error code you want to output as well? https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:378: if (response_date - build_date_ > base::TimeDelta::FromDays(12 * 7)) Chrome has issued 1 major stable release per quarter, if I'm not mistaken. If you are twelve months behind from the release date, then you are more than one major revision behind, are you not? Or did we relax this restriction? By the way, CL description says "on major revision" (not one major revision). https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:381: } Essentially you are setting the |is_outdated_install_| flag and patiently waiting for the next update check to happen? Correct? It might help the uninitiated to document that.
OK, one more round... Sorry about the extra changes but some of your comments spawn some new discussions with the designers and prompted specification changes... Let me know what you think! Takk Fyrir Mig!!! BYE MAD https://codereview.chromium.org/11440020/diff/12029/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/12029/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:25: static bool IsShowing() { return upgrade_bubble_ != NULL; } On 2013/01/25 11:39:22, Finnur wrote: > Doesn't need to be public now. Done. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:58: const char kTimeServerURL[] = "https://www.google.com"; I had looked it up and all I found was the following command line app: http://src.chromium.org/viewvc/chrome/trunk/src/net/tools/get_server_time/get... I don't think the general purpose util is available yet, though I could check with akalin to get more accurate status on this.. Also,thanks for mentioning this because my initial goal was to request the sane time only once per session, and then I forgot about it... Bad MAD, no cookie! :-) https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:188: GoogleUpdateSettings::AUTOMATIC_UPDATES) { On 2013/01/25 11:39:22, Finnur wrote: > I didn't think that the 'prevent any updates' enterprise policy was Windows > only. Is that the case? I couldn't find an implementation for other platforms. Except for Chrome OS, for which we don't want to show the outdated bubble anyway. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:225: UpgradeDetected(); On 2013/01/25 11:39:22, Finnur wrote: > I don't quite grok this... > > If we fail to parse the simulated build date from the command line (result == > false), we assume the build is outdated and trigger the notification. Huh? > > Is this so you can either specify a time (and wait for it to prompt you) or no > time (and have it prompt you immediately)? Can you perhaps comment what you are > after here with the outer if clause (line 213) and the inner if-else clause > (line 218)? Done. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:272: #else // defined(OS_WIN) On 2013/01/25 11:39:22, Finnur wrote: > nit: shouldn't this just be on the endif? Done. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:370: DVLOG(1) << "Variations server request failed."; On 2013/01/25 11:39:22, Finnur wrote: > Of course, this might be a moot point (if you switch away from this method) but > why is it called Variations server? > And is there no error code you want to output as well? Copy/Paste error... Cleared... Not really needed... https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:378: if (response_date - build_date_ > base::TimeDelta::FromDays(12 * 7)) On 2013/01/25 11:39:22, Finnur wrote: > Chrome has issued 1 major stable release per quarter, if I'm not mistaken. If > you are twelve months behind from the release date, then you are more than one > major revision behind, are you not? > > Or did we relax this restriction? By the way, CL description says "on major > revision" (not one major revision). This is the spec as debated with JeffC and ALaforge, it's 12 weeks, when we have a 6 weeks release cycle. https://codereview.chromium.org/11440020/diff/12029/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:381: } On 2013/01/25 11:39:22, Finnur wrote: > Essentially you are setting the |is_outdated_install_| flag and patiently > waiting for the next update check to happen? Correct? It might help the > uninitiated to document that. Ho... Actually, I needed to add a call to UpgradeDetected here, thanks for catching it... It's weird that it worked anyway with the command line switch to test with an outdated time, in that case, UpgradeDetected should not have been called...
https://codereview.chromium.org/11440020/diff/42017/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/42017/chrome/app/generated_reso... 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 title-case vs. sentence-case rules on different platforms (Mac vs. Windows specifically)? But, I would probably advise against reusing labels like this. Probably better to keep them separate. I was at one point told duplicates in grd are dropped at later stages of compiling the translations, so there shouldn't be a penalty if they are the same. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/browser... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/browser... chrome/browser/ui/browser_commands.cc:92: nit: Space after but not before? https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:50: // Don't show the bubble if it's already there. nit: The code is now self documenting and this comment is not needed. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:74: string16 product_name = l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME); nit: As I recall, people prefer: string16 foo(bar); over string16 foo = bar; https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:19: // Chrome toolbar. Don't create a OutdatedUpgradeBubbleView directly, nit: s/ a / an / https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:20: // instead use the static Show method. Do you mean ShowBubble? https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector.h (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:55: // Whether the upgrade is a outdated Chrome (no updates for too long). nit: "... due to Chrome being outdated" https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:59: return is_outdated_install_ && !is_critical_upgrade_; The use of is_critical_upgrade_ here does not make sense. If we've managed to upgrade (whether or not the upgrade was critical), then we should not show the Outdated install bubble. I think you mean to check that the UpgradeLevel > UPGRADE_ANNOYANCE_NONE. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:58: // Default server of Variations seed info. Here is the 'Variations' comment also. Is that intentional? https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:63: const char kOutadedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls"; There are three cases of Outdaded misspellings here on these two lines. :) https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:253: // so validate updatability to set |is_outdated_reinstall_allowed_|. I don't understand the second line of this comment. Specifically, 'validate to set' is hard to grok. Is this a case of s/to/and/ ? Also, there was more room on line 252 -- you didn't need to move that many words to the next line. :) https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:261: is_outdated_reinstall_allowed_ = true; If we ever remove the field trial guard, then this will start returning true for Chromium... https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:276: return; Can we DCHECK or something if tricky combinations are given (e.g. kSimulateOutdated and kSimulate[Critical]Update)? https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:293: void UpgradeDetectorImpl::CompareBuildTimeToSaneTime() { Check the design doc for a proposal that does not involve a network request in the vast majority of the cases. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:307: pending_time_request_->SetMaxRetriesOn5xx(kMaxRetryTimeFetch); I don't think we should be retry-ing on 500 errors. The value you are after isn't super critical. If it fails, so be it. We'll probably catch it on the next startup. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:314: // TODO(mad): Add server side control of the time delta. What does this mean? https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:315: if (sane_time - build_date_ > base::TimeDelta::FromDays(12 * 7)) { nit: Add constant kOutdatedInDays, or something, at top? https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:408: CompareBuildTimeToSaneTime(); Wow! Holy Duncan Ferguson! Stop the press! :) This will be an infinite loop on errors. If people are unhappy with pinging, they'll be super unhappy with infinite pings on failure. :) I'd say, if server time is not set, abort and just try again next startup. We need to let this fail. See also my suggestion on the design doc to drastically reduce the number of times we need to do a ping.
One more round... With a pending dependency on a change currently under review at 12096096. Takk! BYE MAD... https://codereview.chromium.org/11440020/diff/42017/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/42017/chrome/app/generated_reso... chrome/app/generated_resources.grd:8616: + <ph name="IDS_SHORT_PRODUCT_NAME">$1<ex>Chrome</ex></ph> auto-update error On 2013/01/30 15:50:48, Finnur wrote: > Don't we have different title-case vs. sentence-case rules on different > platforms (Mac vs. Windows specifically)? > > But, I would probably advise against reusing labels like this. Probably better > to keep them separate. I was at one point told duplicates in grd are dropped at > later stages of compiling the translations, so there shouldn't be a penalty if > they are the same. Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/browser... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/browser... chrome/browser/ui/browser_commands.cc:92: On 2013/01/30 15:50:48, Finnur wrote: > nit: Space after but not before? Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:50: // Don't show the bubble if it's already there. On 2013/01/30 15:50:48, Finnur wrote: > nit: The code is now self documenting and this comment is not needed. Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:74: string16 product_name = l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME); On 2013/01/30 15:50:48, Finnur wrote: > nit: As I recall, people prefer: > string16 foo(bar); > over > string16 foo = bar; Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:19: // Chrome toolbar. Don't create a OutdatedUpgradeBubbleView directly, On 2013/01/30 15:50:48, Finnur wrote: > nit: s/ a / an / Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:20: // instead use the static Show method. On 2013/01/30 15:50:48, Finnur wrote: > Do you mean ShowBubble? Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector.h (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:55: // Whether the upgrade is a outdated Chrome (no updates for too long). On 2013/01/30 15:50:48, Finnur wrote: > nit: "... due to Chrome being outdated" Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:59: return is_outdated_install_ && !is_critical_upgrade_; The problem is that as soon as we identify is_outdated_install_, we set upgrade_notification_stage_ to UPGRADE_ANNOYANCE_CRITICAL... So that won't work... :-( Actually, I just had a better idea... If we successfully identify any upgrade, we should clear is_outdated_install_ and never set it if upgrade_notification_stage_ > UPGRADE_ANNOYANCE_NONE. This means that is_outdated_install_ and is_critical_update_ should never be both set, might as well use a single value with multiple state... https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:58: // Default server of Variations seed info. On 2013/01/30 15:50:48, Finnur wrote: > Here is the 'Variations' comment also. Is that intentional? Damn... Really sorry about that, I should have been more careful with this copy paste thingy... :-( https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:63: const char kOutadedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls"; On 2013/01/30 15:50:48, Finnur wrote: > There are three cases of Outdaded misspellings here on these two lines. :) Copy/Paste from hell!!! :-) https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:253: // so validate updatability to set |is_outdated_reinstall_allowed_|. On 2013/01/30 15:50:48, Finnur wrote: > I don't understand the second line of this comment. > Specifically, 'validate to set' is hard to grok. > Is this a case of s/to/and/ ? > > Also, there was more room on line 252 -- you didn't need to move that many words > to the next line. :) Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:261: is_outdated_reinstall_allowed_ = true; On 2013/01/30 15:50:48, Finnur wrote: > If we ever remove the field trial guard, then this will start returning true for > Chromium... D'Ho! Added && defined(OS_POSIX) which is || to WIN && OFFICIAL above. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:276: return; On 2013/01/30 15:50:48, Finnur wrote: > Can we DCHECK or something if tricky combinations are given (e.g. > kSimulateOutdated and kSimulate[Critical]Update)? Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:293: void UpgradeDetectorImpl::CompareBuildTimeToSaneTime() { On 2013/01/30 15:50:48, Finnur wrote: > Check the design doc for a proposal that does not involve a network request in > the vast majority of the cases. Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:307: pending_time_request_->SetMaxRetriesOn5xx(kMaxRetryTimeFetch); On 2013/01/30 15:50:48, Finnur wrote: > I don't think we should be retry-ing on 500 errors. The value you are after > isn't super critical. If it fails, so be it. We'll probably catch it on the next > startup. Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:314: // TODO(mad): Add server side control of the time delta. On 2013/01/30 15:50:48, Finnur wrote: > What does this mean? It was about a discussion we had about changing the 12 weeks delta from a value returned from a server (e.g., a Finch experiment) but we decided to only use Finch as an on/off experiment instead. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:315: if (sane_time - build_date_ > base::TimeDelta::FromDays(12 * 7)) { On 2013/01/30 15:50:48, Finnur wrote: > nit: Add constant kOutdatedInDays, or something, at top? Done. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:408: CompareBuildTimeToSaneTime(); On 2013/01/30 15:50:48, Finnur wrote: > Wow! Holy Duncan Ferguson! Stop the press! :) > > This will be an infinite loop on errors. If people are unhappy with pinging, > they'll be super unhappy with infinite pings on failure. :) > > I'd say, if server time is not set, abort and just try again next startup. We > need to let this fail. > > See also my suggestion on the design doc to drastically reduce the number of > times we need to do a ping. Triple D'Ho!... I often feel stupid when I write bad code... This is one of these cases... :-(
In case it would need explaining, the reason I now use a static member function to run in the FILE thread, and it calls back the non-static member function in the UI with values as opposed to be passed data member addresses, is to avoid race conditions since one of the values can also be read and set from the UI thread at different moments (i.e., when comparing the build time to sane time). And it now needs to be a static member function to be able to call back a private method with parameters (as opposed to a cooked up void (void) callback pointer). I hope this helps...
First of all, my apologies for replying so late. I got sidetracked into another review before I got to this one and that took up way too much time. Then I basically spent the remaining hours of today reviewing and thinking at this. It is probably not much of a consolation, but the late reply is not due to lack of priority of this as I prioritized this above addressing review comments in my own review today (as I tend to do, given the timezone considerations). I also took extra care with this CL because I feel it is super important to get this right, not just because this will potentially help a lot of stranded users, but also because I want to make sure the existing update reminders still work and that the code does not get too complex. As a result, this took way too long and my brain is now totally fried too. :) I'm telling you this because I wanted to lessen the blow for you when getting the news that I need to (again) have you restructure parts of it. I believe there is a problem with the current proposal, as I laid out in the comments below, but it is in a different area than last time. Thank you for indulging me on the sane time issue, by the way. https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/42017/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:408: CompareBuildTimeToSaneTime(); No worries. We've all been there. :) On 2013/01/31 21:31:42, MAD wrote: > On 2013/01/30 15:50:48, Finnur wrote: > > Wow! Holy Duncan Ferguson! Stop the press! :) > > > > This will be an infinite loop on errors. If people are unhappy with pinging, > > they'll be super unhappy with infinite pings on failure. :) > > > > I'd say, if server time is not set, abort and just try again next startup. We > > need to let this fail. > > > > See also my suggestion on the design doc to drastically reduce the number of > > times we need to do a ping. > > Triple D'Ho!... I often feel stupid when I write bad code... This is one of > these cases... :-( https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector.h (right): https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:52: // Whether the upgrade is due to Chrome being outdated. nit: I would probably inject the word 'recommendation' between 'upgrade' and 'is'. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:53: bool IsOutdatedInstall() const { But... this function is still simple, no? Why change it to Title-case? https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:97: enum UpgradeRequired { nit: Required is a bit of a strong word. We only require updates for critical updates, which happen mostly behinds the scene so the user shouldn't notice them. I thought about calling this UpgradeAvailable and the enum values UPGRADE_AVAILABLE_* and the last one UPGRADE_NEEDED_OUTDATED_INSTALL, or something. Use as a suggestion if you feel it is an improvement. But feel free to ignore. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:105: // If no update to Chrome has been installed for more than the allowed time. nit: s/allowed/recommended/ https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:119: // - kDisableBackgroundNetworking prevents any of the other tests to work. nit: You prevent something _from_ working, but you enable something _to_ work (if I'm not mistaken). But how do the tests factor into this? Should you replace "test" with "command line flag"? https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:181: base::Bind(&DetectUpdatability, I believe we have a race condition here for the Windows Release channel and I don't think it is the one you mentioned. We fire off two tasks, first CheckForUpgrade and then DetectUpdatability, but the one that is posted *first* depends on a variable (is_outdated_reinstall_allowed_) that the latter sets. That seems like ripe for a race condition... Also, I think that variable isn't really needed because this whole class shouldn't do anything if that variable becomes false. Right? If you can't update you can't update. Outdated update is just one of the things it can't do. However, I think this CL necessitates a restructure of the ctor a bit anyway, because it is getting too difficult to understand, especially with the new ifdefs and the new outdated variable. My thinking is that restructuring will not only simplify all this logic but also gets rid of the is_outdated_reinstall_allowed_ variable altogether. Here's what I have in mind: I suspect I'd keep everything in the ctor down to line 155 as is, but at the end just fire off a CheckForUpgradability async task on the FILE thread (then the ctor is done and does nothing more). That new task, however, would look something like this: void CheckForUpgradability() { #if defined(OS_WINDOW) && defined(GOOGLE_CHROME_BUILD) if (!DetectUpdatability()) return; // Upgrade is blocked by enterprise policy. #elif defined(OS_WINDOW) && !defined(GOOGLE_CHROME_BUILD) return; // Chromium has no upgrade channel. #elif defined(OS_MACOSX) if (!keystone_glue::KeystoneEnabled()) return; // Keystone updater not enabled. #elif !defined(OS_POSIX) return; #endif [Jump jubilantly to the UI thread to perform CheckForUpgrade()] } Now, I'm assuming KeystoneEnabled can be called on the FILE thread, as that would be the simplest and cleanest implementation. If that doesn't work then, just call this function on the UI thread directly from the ctor (then there is no need to jump jubilantly, just call it directly). Then the top becomes: #if defined(OS_WINDOW) && defined(GOOGLE_CHROME_BUILD) [Jump to DetectUpdatability on the FILE thread, which will result in a jump to the UI thread for CheckforUpgrade if Chrome is upgradable] return; #elif ... etc There might be other options too if you take a step back and think about this from a higher level. Also note that I think we don't need to check for upgrade if we find that Chrome is outdated, so we can skip that part and early return on outdate builds (after notifying the user). Maybe that helps us simplify the restructuring a bit. I can't really comment on the change to static. Maybe my brain is fried but on the face of it I don't think it is necessary. However, this is enough for you to digest for now. We can talk about that later (unless it becomes a moot point because of restructuring, which I'm kind of hoping). https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:271: if (is_outdated_reinstall_allowed_ && This would be a field trial check instead (according to my proposal). https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:305: // return; I likey. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:306: I think you should add a check, just to be sure. Something like... if (build_date_ > network_time) { NOTREACHED(); return; } https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:307: DCHECK(!network_time.is_null()); You could pull that into the if above and check that build_date_ is not null, perhaps? At least early return if network_time is null. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:321: else if (upgrade_required_ == UPGRADE_REQUIRED_NONE) I'm sure that a few months down the line we're going to look at this and go... "wait, why are we checking for NONE here"? Maybe you should document that you do this because of the outdated codepath?
How about this? https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector.h (right): https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:52: // Whether the upgrade is due to Chrome being outdated. On 2013/02/01 17:09:15, Finnur wrote: > nit: I would probably inject the word 'recommendation' between 'upgrade' and > 'is'. Done. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:53: bool IsOutdatedInstall() const { On 2013/02/01 17:09:15, Finnur wrote: > But... this function is still simple, no? Why change it to Title-case? Done. I thought it was only for accessors, i.e., getting the value of a data member, but you're right, it kind of is an accessor to a specific state of a data member, good enough... :-) https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:97: enum UpgradeRequired { On 2013/02/01 17:09:15, Finnur wrote: > nit: Required is a bit of a strong word. We only require updates for critical > updates, which happen mostly behinds the scene so the user shouldn't notice > them. I thought about calling this UpgradeAvailable and the enum values > UPGRADE_AVAILABLE_* and the last one UPGRADE_NEEDED_OUTDATED_INSTALL, or > something. > > Use as a suggestion if you feel it is an improvement. But feel free to ignore. Done. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector.h:105: // If no update to Chrome has been installed for more than the allowed time. On 2013/02/01 17:09:15, Finnur wrote: > nit: s/allowed/recommended/ Done. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:119: // - kDisableBackgroundNetworking prevents any of the other tests to work. On 2013/02/01 17:09:15, Finnur wrote: > nit: You prevent something _from_ working, but you enable something _to_ work > (if I'm not mistaken). > > But how do the tests factor into this? Should you replace "test" with "command > line flag"? Done. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:181: base::Bind(&DetectUpdatability, OK, I agree that things need to be cleaned up, but for the cases where there is nothing to do on the FILE thread, we might as well stay on the UI thread. No? Let see what you think of the refactoring I did. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:271: if (is_outdated_reinstall_allowed_ && On 2013/02/01 17:09:15, Finnur wrote: > This would be a field trial check instead (according to my proposal). Done. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:305: // return; On 2013/02/01 17:09:15, Finnur wrote: > I likey. B-) https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:306: On 2013/02/01 17:09:15, Finnur wrote: > I think you should add a check, just to be sure. Something like... > > if (build_date_ > network_time) { > NOTREACHED(); > return; > } Done. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:307: DCHECK(!network_time.is_null()); On 2013/02/01 17:09:15, Finnur wrote: > You could pull that into the if above and check that build_date_ is not null, > perhaps? At least early return if network_time is null. Done. https://codereview.chromium.org/11440020/diff/56001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:321: else if (upgrade_required_ == UPGRADE_REQUIRED_NONE) Refactoring took care of this.
https://codereview.chromium.org/11440020/diff/62003/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/62003/chrome/app/generated_reso... chrome/app/generated_resources.grd:14785: + nit: There are three whitespace changes here, two linebreaks being added and one removed. It is hard to judge it when one doesn't have the side-by-side diff and more context. I'm not saying this should be changed, just asking if this was intentional? I'm fine if it is. If so, just ignore. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:28: // Fixed with of the column holding the description label of the bubble. s/with/width/ https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:172: content::UserMetricsAction("OutdatedUpgradeBubble_Later")); You use two different ways of reporting which action took place, one is 'foo_action' and the other 'fooaction'. Also, only one refers to the Bubble, the other does not. Suggest picking one over the other. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } Also: In my experience the UMA_HISTOGRAM macros are most valuable (more so, or perhaps in addition to the RecordAction calls), as they give you a nice histogram showing how often the user picked one action over another. Of course, that's only required if you need to know the data (and will require an additional step of adding an enum to the histograms.xml). If you don't care about the more detailed metrics, then that's fine and you can ignore this comment entirely. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:98: void DetectUpdatability(const base::Closure& start_timer) { s/start_timer/callback_task/ (or |task|, or something) https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:172: } I like where this is heading, but can you replace lines 157-171 with: #if defined(OS_WINDOW) && defined(GOOGLE_CHROME_BUILD) [Do what you do on line 163-170 above (post the task)] return; [THIS IS IMPORTANT!] :) #elif defined(OS_WINDOW) && !defined(GOOGLE_CHROME_BUILD) return; // Chromium has no upgrade channel. #elif defined(OS_MACOSX) if (!keystone_glue::KeystoneEnabled()) return; // Keystone updater not enabled. #elif !defined(OS_POSIX) return; #endif StartCheckForUpgradeTimer(); } This should be functionally equivalent but is so much easier to read and understand. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:257: void UpgradeDetectorImpl::StartCheckForUpgradeTimer() { Suggest renaming: StartTimerForUpgradeCheck Not only is it less ambiguous, it means you can search for 'CheckForUpgrade' and not find this function definition nor its call sites. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:264: // No need to look fur upgrades if the install is outdated. s/für/for/ :) https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) Suggest renaming this function: IsOutdatedInstall() However, there is a bit of a problem here as the |is_unstable_channel_| variable is not valid at this point because it is set in DetectUpgradeTask which doesn't get called until line 274. This will lead to the wrong clause being selected in the if statement on line 339 (when CheckForOutdatedInstall calls UpgradeDetected). I suspect you need to move the CheckForOutdatedInstall call to the DetectUpgradeTask. It means you need to jump to the UI thread to call UpgradeDetected, but that task already does so, so you can maybe leverage that instead of calling UpgradeDetected on line 301. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:301: UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL, is_unstable_channel_); It would probably be cleaner to have this function just check and not notify (and move this line (line 301) over to line 266 (before the return)). But see my comment on line 265 first. Maybe you can just get rid of this call (from this function). https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:304: // If we simlated an oputdated install with a date, we don't want to keep s/oputdated/outdated/ https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:37: // Returns true after calling UpgradeDetected if current install is outdated. If you change what I suggested, then you can remove the UpgradeDetected reference here. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( Does this still need to be static. If so, I'd like to learn more about the data race you referred to. It looks to me like we are back to the liniar mode of running this, so there shouldn't be a data race (except on the variable I mentioned, because it is set so late in the game).
Here are some answers to you comments without a code update just yet, because I'm still unsure how to best resolve some of the issues you raise, so maybe you'll have some other suggestions as you read my answers... Thanks again! BYE MAD https://codereview.chromium.org/11440020/diff/62003/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/11440020/diff/62003/chrome/app/generated_reso... chrome/app/generated_resources.grd:14785: + Fixed the removed vertical white space, I had missed the fact that sections are separated by white space... The others I didn't do explicitly, I'm guessing it's because my IDE automatically removes trailing white spaces... https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:28: // Fixed with of the column holding the description label of the bubble. On 2013/02/04 11:12:04, Finnur wrote: > s/with/width/ Done. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:172: content::UserMetricsAction("OutdatedUpgradeBubble_Later")); On 2013/02/04 11:12:04, Finnur wrote: > You use two different ways of reporting which action took place, one is > 'foo_action' and the other 'fooaction'. Also, only one refers to the Bubble, the > other does not. Suggest picking one over the other. Done. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } On 2013/02/04 11:12:04, Finnur wrote: > Also: In my experience the UMA_HISTOGRAM macros are most valuable (more so, or > perhaps in addition to the RecordAction calls), as they give you a nice > histogram showing how often the user picked one action over another. Of course, > that's only required if you need to know the data (and will require an > additional step of adding an enum to the histograms.xml). If you don't care > about the more detailed metrics, then that's fine and you can ignore this > comment entirely. I believe more metrics is almost always better... Let me know if you are OK with what I added... https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:98: void DetectUpdatability(const base::Closure& start_timer) { On 2013/02/04 11:12:04, Finnur wrote: > s/start_timer/callback_task/ > (or |task|, or something) Done. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:172: } Agreed, much better, thanks a lot! :-) Done! https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:257: void UpgradeDetectorImpl::StartCheckForUpgradeTimer() { On 2013/02/04 11:12:04, Finnur wrote: > Suggest renaming: StartTimerForUpgradeCheck > Not only is it less ambiguous, it means you can search for 'CheckForUpgrade' and > not find this function definition nor its call sites. Done. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:264: // No need to look fur upgrades if the install is outdated. On 2013/02/04 11:12:04, Finnur wrote: > s/für/for/ :) Done. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) OK, I think I agree with you... I was trying to do the outdated check in the UI thread since it doesn't need to be done on the file thread. And I ignored the issue of the unstable channel because in the case of the outdated install, the code is the same whether is_unstable_channel is true or false, we end up in the UPGRADE_ANNOYANCE_CRITICAL state either way... But things might change in the future and so we might as well make it more clear, at the price at going to the file thread for the outdated check even if we don't really need. Although it will add complexity on the sane/network time provider which will need to support being called from the file thread... :-/ Another way to avoid this would be to set is_unstable_channel earlier, since it only needs to be done once, we could do it at the same time as the policy check, but since the policy check is not done on all platform combination, this makes it a bit more complex... I'll see if I can find something clean in that direction... https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:301: UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL, is_unstable_channel_); On 2013/02/04 11:12:04, Finnur wrote: > It would probably be cleaner to have this function just check and not notify > (and move this line (line 301) over to line 266 (before the return)). But see my > comment on line 265 first. Maybe you can just get rid of this call (from this > function). This makes it much more complex to support simulation of outdated builds on non CHROME builds, which need to avoid doing the rest of the work in CheckForUpgrade, yet still want to check for the date... I'll see if I can find a way to still support that... somehow... :-/ https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:304: // If we simlated an oputdated install with a date, we don't want to keep On 2013/02/04 11:12:04, Finnur wrote: > s/oputdated/outdated/ Done.
OK, as mentioned in the new answers. I couldn't find a clean way to do the check for outdated installs in the file thread, so I kept it in the UI thread. To avoid the problems you mention, I moved the setting of the is_unstable_channel_ data member to tasks running only once, launched from the constructor. I also moved up the invalidation of the weak pointer before we call into the check for outdated install. I know it doesn't make a big difference since this is all synchronized from the same thread, but it makes it more obvious that there is no chance that UpgradeDetected would be called twice, once from a pending file thread tasks and another directly from within CheckForOutdatedInstall. I hope you will be OK with this, otherwise, we need a way to make sure that the rest of the upgrade version check is not done when we are running with the simulate outdated switch using a date that has not gone through the deadline just yet (to validate the timer checks). We could do it by returning false in that case and then do yet another check for the switch in DetectUpgradeTask(), or use a tri-state return value, but I thought this was uglier than what we have now... Feel free to disagree, or suggest something else I didn't think of... Let me know if you need more clarifications... Thanks again! BYE MAD https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) So after trying a few different things, I couldn't find a simple way to make it work with the check for outdated install being done on the file thread. I think doing it in the UI thread like this is a cleaner solution after all... Let me know if you disagree... https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:301: UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL, is_unstable_channel_); Didn't find anything I was happy with. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:37: // Returns true after calling UpgradeDetected if current install is outdated. On 2013/02/04 11:12:04, Finnur wrote: > If you change what I suggested, then you can remove the UpgradeDetected > reference here. I didn't... I hope it's OK... :-) https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( On 2013/02/04 11:12:04, Finnur wrote: > Does this still need to be static. If so, I'd like to learn more about the data > race you referred to. It looks to me like we are back to the liniar mode of > running this, so there shouldn't be a data race (except on the variable I > mentioned, because it is set so late in the game). For the same reason that we call InvalidateWeakPtrs in case we get to CheckForUpgrade before DetectUpgradeTask is done, then it could be messing up with data member in the file thread as we might be interacting with them in the UI thread. I prefer passing the value we want for upgrade_available to UpgradeDetected() instead of passing the address of the data member to the file thread and get it changed there. Don't you agree that this is cleaner? This way we ensure that value is always get/set from the UI thread. Or am I missing something that gets you to prefer doing it the old way?
https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) I'll take a closer look at the code tomorrow morning (just wanted to interject something)... I think moving the check for unstable channel further up is fine. Perhaps a little more complex than what I thought of after reading this. If I would have seen the comments earlier, I would have suggested changing the param for UpgradeDetected to fast_track, or something, to decouple it from unstable_channel. Then you could have hard coded that param to false (or true) instead of passing in is_unstable_channel_ (which isn't really initialized yet and looks like it shouldn't be used at that point). The places that have the right value for unstable_channel would of course be able to keep using that value. But this is probably fine too. You still have time if you want to make changes (or longer if you ask me to hold off on reviewing). Otherwise I'll assume this is the way to go. I'll take a better look tomorrow with a less fried brain. :) I'd still like the UpgradeDetected call moved out of the CheckForOutdatedInstall function though. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( I need a fresh mind to think about this. I'll try to tackle this tomorrow. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:105: // version. s/version/channel/ https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:121: // it unconditionnally calls back the provided task. no double n in unconditionally.
Thanks a lot, I don't know if you noticed, but it's not as much a rush anymore since Jeff decided to punt this out of M26... ;-(... BYE MAD... sent from Mobile Answering Device! Le 4 févr. 2013 16:30, <finnur@chromium.org> a écrit : > > https://codereview.chromium.**org/11440020/diff/62003/** > chrome/browser/upgrade_**detector_impl.cc<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<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 code tomorrow morning (just wanted to > interject something)... > > I think moving the check for unstable channel further up is fine. > Perhaps a little more complex than what I thought of after reading this. > If I would have seen the comments earlier, I would have suggested > changing the param for UpgradeDetected to fast_track, or something, to > decouple it from unstable_channel. Then you could have hard coded that > param to false (or true) instead of passing in is_unstable_channel_ > (which isn't really initialized yet and looks like it shouldn't be used > at that point). The places that have the right value for > unstable_channel would of course be able to keep using that value. > > But this is probably fine too. You still have time if you want to make > changes (or longer if you ask me to hold off on reviewing). Otherwise > I'll assume this is the way to go. I'll take a better look tomorrow with > a less fried brain. :) > > I'd still like the UpgradeDetected call moved out of the > CheckForOutdatedInstall function though. > > https://codereview.chromium.**org/11440020/diff/62003/** > chrome/browser/upgrade_**detector_impl.h<https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_detector_impl.h> > File chrome/browser/upgrade_**detector_impl.h (right): > > https://codereview.chromium.**org/11440020/diff/62003/** > chrome/browser/upgrade_**detector_impl.h#newcode47<https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_detector_impl.h#newcode47> > chrome/browser/upgrade_**detector_impl.h:47: static void > DetectUpgradeTask( > I need a fresh mind to think about this. I'll try to tackle this > tomorrow. > > https://codereview.chromium.**org/11440020/diff/78001/** > chrome/browser/upgrade_**detector_impl.cc<https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_detector_impl.cc> > File chrome/browser/upgrade_**detector_impl.cc (right): > > https://codereview.chromium.**org/11440020/diff/78001/** > chrome/browser/upgrade_**detector_impl.cc#newcode105<https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_detector_impl.cc#newcode105> > chrome/browser/upgrade_**detector_impl.cc:105: // version. > s/version/channel/ > > https://codereview.chromium.**org/11440020/diff/78001/** > chrome/browser/upgrade_**detector_impl.cc#newcode121<https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_detector_impl.cc#newcode121> > chrome/browser/upgrade_**detector_impl.cc:121: // it unconditionnally > calls back the provided task. > no double n in unconditionally. > > https://codereview.chromium.**org/11440020/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } If it were me, I'd probably want to know how many choose to reinstall. A simple: UMA_HISTOGRAM_BOOLEAN("OutdatedUpgradeBubble.ReinstallChosen", true); ... on line 182 and ... UMA_HISTOGRAM_BOOLEAN("OutdatedUpgradeBubble.ReinstallChosen", false); ... on line 188 is then what I'd expect to see. You'd have to keep in mind, though that the data gets skewed by the fact that the bubble is shown again for those that say no. I guess the really interesting metric is how long it takes until people say yes. Sort of what you have above, but since I'd expect only one bubble per launch of Chrome you'd have to keep track of that number in the prefs (across restarts of Chrome). :/ Bottom line, though: I can't judge what data you should be looking for. For example, if nobody will be checking the UMA dashboard for this then probably no data should be gathered. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) > Although it will add complexity on the sane/network time provider > which will need to support being called from the file thread.. Might be a moot point, but if this is a concern then you can always get the sane time in the ctor on the UI thread, or better yet: do it at StartCheckForUpgradeTimer, which also runs on the UI thread (and runs once only). https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( So, to answer your question: No, I don't prefer the old way. I like sending the value over to the UI thread. However, by doing so, the function no longer touches data members of the class and therefore does not need to be static. Correct? If I am mistaken, can you spell it out in a little more detail which variables get touched and at what point, so I can understand the concern better? https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:51: // The numer of times the user ignored the bubble before it finally chose to Suggest: ... before finally choosing to reinstall. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:53: static int num_ignored_bubbles_; I'm a little confused as to why you are keeping track of how many bubbles are shown. I would have expected this bubble to show only once per session (not make a reappearance until next startup once you dismiss it). Lets make it clear: If I choose Later, what should Later mean? 15 minutes? An hour? An hour after I next restart Chrome? Basically, I'm worried we might have an annoying bubble that you can't get rid of. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:59: const char kOutdatedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls"; I assume you've filed this an experiment request with the Finch team? (they like to keep track of these with a special form they had me submit when I was doing mine) https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:115: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback_task); I think a direct call to ... CheckForUnstableChannel(callback_task, is_unstable_channel); ... instead of this PostTask would consolidate the unstable check into one location instead of two, which should make it less brittle and make it easier to debug (because then you'll never accidentally set the breakpoint on line 109 when debugging on non-Windows platforms because you can then delete line 109). https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:288: // Interrupt any (unlikely) unfinished business. Can you expand a bit on this comment (meaning: that we are talking about unfinished *DetectUpgradeTask* business, and not unfinished StartTimerForUpgradeCheck business). Perhaps mention the point of doing this before checking for outdated. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:327: UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL); You can move this line to line 293 without adverse effect since that is the only consumer of this function. That should make this function do just what it says in its name (and the name should probably be IsOutdatedInstall, btw). https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:330: // If we simlated an outdated install with a date, we don't want to keep I think this is correct, but just to be sure: We will not get here if no outdated date was simulated?
> Thanks a lot, I don't know if you noticed, but it's not as much > a rush anymore since Jeff decided to punt this out of M26... ;-(... Sorry to hear that.
Addressed/answered your comments... back to you. Thanks again! https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } On 2013/02/05 10:52:19, Finnur wrote: > If it were me, I'd probably want to know how many choose to reinstall. A simple: > > UMA_HISTOGRAM_BOOLEAN("OutdatedUpgradeBubble.ReinstallChosen", true); > > ... on line 182 and ... > > UMA_HISTOGRAM_BOOLEAN("OutdatedUpgradeBubble.ReinstallChosen", false); > > ... on line 188 is then what I'd expect to see. You'd have to keep in mind, > though that the data gets skewed by the fact that the bubble is shown again for > those that say no. > > I guess the really interesting metric is how long it takes until people say yes. > Sort of what you have above, but since I'd expect only one bubble per launch of > Chrome you'd have to keep track of that number in the prefs (across restarts of > Chrome). :/ > > Bottom line, though: I can't judge what data you should be looking for. For > example, if nobody will be checking the UMA dashboard for this then probably no > data should be gathered. OK, thanks, I'll ask Jeff what he would like to see. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) On 2013/02/05 10:52:19, Finnur wrote: > > Although it will add complexity on the sane/network time provider > > which will need to support being called from the file thread.. > > Might be a moot point, but if this is a concern then you can always get the sane > time in the ctor on the UI thread, or better yet: do it at > StartCheckForUpgradeTimer, which also runs on the UI thread (and runs once > only). But for people not restarting Chrome very often, we want to keep checking for the current time. So we don't want to do it just once. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( On 2013/02/05 10:52:19, Finnur wrote: > So, to answer your question: No, I don't prefer the old way. I like sending the > value over to the UI thread. However, by doing so, the function no longer > touches data members of the class and therefore does not need to be static. > Correct? > > If I am mistaken, can you spell it out in a little more detail which variables > get touched and at what point, so I can understand the concern better? Ha! Now I see where the confusion is... It used to not be a member function because it was calling an anonymous callback. Now we can don't this if we pass in arguments to the function. So either we make this a member function or we make the call back function (UpgradeDetected) public. Clear? https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:51: // The numer of times the user ignored the bubble before it finally chose to On 2013/02/05 10:52:19, Finnur wrote: > Suggest: ... before finally choosing to reinstall. Done. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:53: static int num_ignored_bubbles_; On 2013/02/05 10:52:19, Finnur wrote: > I'm a little confused as to why you are keeping track of how many bubbles are > shown. I would have expected this bubble to show only once per session (not make > a reappearance until next startup once you dismiss it). > > Lets make it clear: If I choose Later, what should Later mean? 15 minutes? An > hour? An hour after I next restart Chrome? Basically, I'm worried we might have > an annoying bubble that you can't get rid of. There is a way to get the bubble back. Clicking on the wrench menu item usually called upgrade Chrome, renamed to auto update error (or something like that). But I agree that it might not be that useful. I asked Jeff about this in the Spec document. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:59: const char kOutdatedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls"; On 2013/02/05 10:52:19, Finnur wrote: > I assume you've filed this an experiment request with the Finch team? (they like > to keep track of these with a special form they had me submit when I was doing > mine) Not yet, but I do know about this, I sit with the Finch team... :-) I even used to be the TL when the team was founded... B-) https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:105: // version. On 2013/02/04 21:30:17, Finnur wrote: > s/version/channel/ Done. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:115: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback_task); On 2013/02/05 10:52:19, Finnur wrote: > I think a direct call to ... > > CheckForUnstableChannel(callback_task, is_unstable_channel); > > ... instead of this PostTask would consolidate the unstable check into one > location instead of two, which should make it less brittle and make it easier to > debug (because then you'll never accidentally set the breakpoint on line 109 > when debugging on non-Windows platforms because you can then delete line 109). Done. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:121: // it unconditionnally calls back the provided task. On 2013/02/04 21:30:17, Finnur wrote: > no double n in unconditionally. Done. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:288: // Interrupt any (unlikely) unfinished business. On 2013/02/05 10:52:19, Finnur wrote: > Can you expand a bit on this comment (meaning: that we are talking about > unfinished *DetectUpgradeTask* business, and not unfinished > StartTimerForUpgradeCheck business). Perhaps mention the point of doing this > before checking for outdated. Done. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:327: UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL); On 2013/02/05 10:52:19, Finnur wrote: > You can move this line to line 293 without adverse effect since that is the only > consumer of this function. That should make this function do just what it says > in its name (and the name should probably be IsOutdatedInstall, btw). OK, sorry for not being clear, but this won't work unless I do another check for simulate_outdated or return a tri-state from this method, i.e., 1) no outdated detected nor simulated, so go on to check for version upgrade (the current false), 2) an outdated was detected, so don't continue, UpgradeDetected was already called (the current true), 3) not outdated yet, but we have a simulation with a date that should eventually reach outdated state, so don't continue with version upgrade check, but I didn't call UpgradeDetected yet, so call me again (also returning true right now). How about we rename the function to DetectOutdatedInstall? And just as DetectUpgradeTask, it's allowed to call UpgradeDetected. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:330: // If we simlated an outdated install with a date, we don't want to keep On 2013/02/05 10:52:19, Finnur wrote: > I think this is correct, but just to be sure: We will not get here if no > outdated date was simulated? It is possible to get here when we have no current simulation and we are not outdated yet, this is the most frequent case of returning false.
https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:173: } Don't feel the need to wait on just this. The UMA changes can be a separate changelist. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:265: if (CheckForOutdatedInstall()) > But for people not restarting Chrome very often, we want to keep checking for > the current time. So we don't want to do it just once. Yes, you are right. You'd have to keep track of the delta, which is overkill. This is fine. https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( Member function would be my preference. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:179: 0, kMaxIgnored, kNumIgnoredBuckets); OK, now that I understand the context a little better I realize that you are tracking here whether the user Upgraded on the first sight of the bubble or whether they manually made the bubble reappear and then upgraded. The count, however, is only keeping track since Chrome was restarted and as such I'm not sure it is a meaningful statistic. But if it is meaningful to you or Jeff then I'm fine with it as is. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:53: static int num_ignored_bubbles_; Oh, I see. So the menu item stays and you can recall the bubble manually. It's not like the bubble will automatically re-appear again in 15 minutes. In that case this is fine. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:59: const char kOutdatedInstallCheck12WeeksGroupName[] = "12WeeksOutdatedIntalls"; > Not yet, but I do know about this, I sit with the Finch team... :-) I even used > to be the TL when the team was founded... B-) Hah! :) https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:327: UpgradeDetected(UPGRADE_NEEDED_OUTDATED_INSTALL); Oh, I see. Good point. I forgot about the simulate cmd line param. Fine. https://codereview.chromium.org/11440020/diff/78001/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:330: // If we simlated an outdated install with a date, we don't want to keep Yes, my question was more along the lines of "I'm curious as to why the comment specifies _with a date_". This is probably right, just making sure. https://codereview.chromium.org/11440020/diff/90002/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/90002/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:126: } Why do you need the else for? If the app update policy != 'automatic updates' then you are done, right? You only need is_unstable_channel for the upgrade detection, which is not going to occur because you don't call CheckForUnstableChannel... https://codereview.chromium.org/11440020/diff/90002/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:290: // least prevent the callback to be executed, because we will potentially s/to/from/
About the metrics, Jeff said he would like to have cross session but can live with per session if it's much less work. I may add a cross session count in an upcoming CL then. Thanks! BYE MAD https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( On 2013/02/05 22:14:50, Finnur wrote: > Member function would be my preference. You mean not static? (cause... you know... technically... a static method... is... a member function... :-)... The reason why I used a static was to prevent using the this pointer in the FILE thread and force usage of a weak pointer... So I tried to see if I could build this differently, e.g., bring back the function out of the class and use a partially bound callback, so that we could put the weak pointer in it, yet let the caller of the callback specify the other argument... But as I read the documentation for all this, I realize that weak pointers are not supposed to be used across threads... So this code was already broken... The weak pointer invalidation useless, and even dangerous... So before I start changing this I would like to have your opinion. I'm thinking of using an unretained this pointer to post the calls across the threads, and maybe use an atomically thread safe gate to make sure we don't cross call UpgradeDetected... Do you have other ideas? https://codereview.chromium.org/11440020/diff/90002/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.cc (right): https://codereview.chromium.org/11440020/diff/90002/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:126: } On 2013/02/05 22:14:50, Finnur wrote: > Why do you need the else for? > If the app update policy != 'automatic updates' then you are done, right? You > only need is_unstable_channel for the upgrade detection, which is not going to > occur because you don't call CheckForUnstableChannel... D'ho!ne. https://codereview.chromium.org/11440020/diff/90002/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.cc:290: // least prevent the callback to be executed, because we will potentially On 2013/02/05 22:14:50, Finnur wrote: > s/to/from/ Done.
LGTM, with one nit and a caveat (that network sane time is commented out due to a pending CL (and discussion) in-flight, so the feature isn't really live yet). https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/62003/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: static void DetectUpgradeTask( I see. > Do you have other ideas? No. If you can boil this down to a comment for the static function, then I'm fine with this as is. On 2013/02/06 03:14:28, MAD wrote: > On 2013/02/05 22:14:50, Finnur wrote: > > Member function would be my preference. > > You mean not static? > (cause... you know... technically... a static method... is... a member > function... :-)... > > The reason why I used a static was to prevent using the this pointer in the FILE > thread and force usage of a weak pointer... > > So I tried to see if I could build this differently, e.g., bring back the > function out of the class and use a partially bound callback, so that we could > put the weak pointer in it, yet let the caller of the callback specify the other > argument... But as I read the documentation for all this, I realize that weak > pointers are not supposed to be used across threads... So this code was already > broken... The weak pointer invalidation useless, and even dangerous... > > So before I start changing this I would like to have your opinion. I'm thinking > of using an unretained this pointer to post the calls across the threads, and > maybe use an atomically thread safe gate to make sure we don't cross call > UpgradeDetected... Do you have other ideas?
Added requested comments and also enabled variations service network time access since it's not available... Ben, can you do an OWNERS review for this, or should I ask someone else? Note that this just got kicked out of M26 and punted into M27, so not a rush... Although, if we think it's safe enough, we might decide to put it into M26 since it's currently controlled by a Finch experiment anyway... I'll talk to Jeff and Anthony about it. If you want details, there is a spec doc here: https://docs.google.com/a/google.com/document/d/1Wkxc2VIvmkN82er-hSL9IDE0Belq... Thanks! BYE MAD...
I just got a confirmation from the [T]PMs that we can commit this in M26, if we get an approval before the branch point...
Still LGTM, with a couple of nits. Over to you, Ben. https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:46: // method receiving a WeakPtr<> to the this pointer so that we can interrupt nit: s/the this pointer/this object/ https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: // it. Having this method non-static and using the this pointer directly nit: s/it/this task before it runs/ nit: s/the this pointer/|this|/
Thanks again... BYE MAD https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_de... File chrome/browser/upgrade_detector_impl.h (right): https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:46: // method receiving a WeakPtr<> to the this pointer so that we can interrupt On 2013/02/07 09:17:44, Finnur wrote: > nit: s/the this pointer/this object/ Done. https://codereview.chromium.org/11440020/diff/80011/chrome/browser/upgrade_de... chrome/browser/upgrade_detector_impl.h:47: // it. Having this method non-static and using the this pointer directly On 2013/02/07 09:17:44, Finnur wrote: > nit: s/it/this task before it runs/ > nit: s/the this pointer/|this|/ Done.
Ping? Ben, should I ask someone else for the OWNERS review? Thank! BYE MAD...
Pong? BYE MAD :-)
Ben seems too busy.... Scott, can you take care of this OWNERS review or should I ask Peter? Thanks! BYE MAD
Ben was out the past couple of days, but AFAIK he is doing reviews now.
https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:9: #include "chrome/browser/ui/browser_tabstrip.h" ... then you could remove these two #includes. https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:33: OutdatedUpgradeBubbleView(views::View* anchor_view, Browser* browser); this code could be a little bit more isolated if it took a PageNavigator instead of a Browser. Browser is effectively a PageNavigator.
https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:191: #if defined(OS_WIN) || defined(OS_MACOSX) || \ instead of having these ifdefs everywhere, how about an oracle function in the header you include above that has the ifdefs in one place. then just call it in each of these places.
All done... Thanks! Anything else? BYE MAD https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.cc (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.cc:9: #include "chrome/browser/ui/browser_tabstrip.h" On 2013/02/13 22:34:51, Ben Goodger (Google) wrote: > ... then you could remove these two #includes. Done. https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... File chrome/browser/ui/views/outdated_upgrade_bubble_view.h (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/o... chrome/browser/ui/views/outdated_upgrade_bubble_view.h:33: OutdatedUpgradeBubbleView(views::View* anchor_view, Browser* browser); On 2013/02/13 22:34:51, Ben Goodger (Google) wrote: > this code could be a little bit more isolated if it took a PageNavigator instead > of a Browser. Browser is effectively a PageNavigator. Cool, I didn't know about this interface... Thanks! https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11440020/diff/97001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar_view.cc:191: #if defined(OS_WIN) || defined(OS_MACOSX) || \ On 2013/02/13 22:35:44, Ben Goodger (Google) wrote: > instead of having these ifdefs everywhere, how about an oracle function in the > header you include above that has the ifdefs in one place. > > then just call it in each of these places. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/11440020/91067
Message was sent while issue was closed.
Change committed as 182626 |