Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493973003:
Display "Restart to update" dialog to Chrome OS users. (Closed)
DescriptionDisplay "Restart to update" dialog to Chrome OS users.
This displays the "restart to update" message in the system tray to
Chrome OS users when a flash update is available.
BUG=630420
Committed: https://crrev.com/5aa7ae3264bd376e48eb283a4977dfc86b98dc11
Cr-Commit-Position: refs/heads/master@{#436782}
Patch Set 1 #Patch Set 2 : Display "Restart to update" dialog to Chrome OS users. #
Total comments: 19
Patch Set 3 : Display "Restart to update" dialog to Chrome OS users. #
Total comments: 12
Patch Set 4 : Display "Restart to update" dialog to Chrome OS users. #Patch Set 5 : Display "Restart to update" dialog to Chrome OS users. #
Total comments: 6
Patch Set 6 : Display "Restart to update" dialog to Chrome OS users. #
Total comments: 8
Patch Set 7 : Display "Restart to update" dialog to Chrome OS users. #
Total comments: 3
Patch Set 8 : Display "Restart to update" dialog to Chrome OS users. #Patch Set 9 : Rebased CL #Patch Set 10 : Rebased CL #
Total comments: 1
Messages
Total messages: 74 (42 generated)
kerrnel@chromium.org changed reviewers: + jamescook@chromium.org
PTAL. Thanks, Greg
kerrnel@chromium.org changed reviewers: + waffles@chromium.org
waffles@chromium.org: Please review changes in component_updater/pepper_flash_component_installer.cc Thanks, Greg
waffles@chromium.org: Please review changes in component_updater/pepper_flash_component_installer.cc Thanks, Greg
lgtm https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:72: bool result) { Maybe it's time to add a DCHECK_CURRENTLY_ON(content::BrowserThread::UI) here. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:84: tray->SetFlashUpdateAvailable(); As an FYI, the updater uses a lot of CONTINUE_ON_SHUTDOWN, so we can wind up calling this pretty late in the browser's destruction. As long as this is only run on the UI thread I think this should be safe.
https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:185: g_instance = this; DCHECK(!g_instance) above just to make sure no one is instantiating two instances https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:234: SystemTrayDelegateChromeOS::~SystemTrayDelegateChromeOS() { DCHECK_EQ(this, g_instance); g_instance = nullptr; https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:328: if (!info->factory_reset_required && flash_update_available_) Any particular reason not to set update_required if factory_reset_required is set? https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1033: ash::UpdateInfo info; Why not call GetSystemUpdateInfo() and use that result? That will make sure GetUpdateInfo() is always called, so if someone adds something there then this won't break. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1039: } Can you add a test for this? It's unlikely that flash updates are going to get tested very often by QA. I would suggest adding something to the browser test (the unit test seems to have a flake problem you probably don't want to deal with). I think there is a GetTrayUpdateForTest() method in ash you can use to check if the update icon actually appears. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right): https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.h:128: static SystemTrayDelegateChromeOS* Get(); put this at the top, near the constructor https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.h:129: void SetFlashUpdateAvailable(); function comment please
For the test you probably want something like ash::Shell::GetInstance()->GetPrimarySystemTray()->GetTrayUpdateForTesting();
Patchset #3 (id:40001) has been deleted
To explain why so many more files are being touched here, it turned out that if a system update was not available, Chrome would just exit the session manager and not reboot. I tried to remove that logic, but much of the system relies on that property. For example, you cannot even deploy a new Chrome build to your chromebook if you take that out. So looking through it, I felt the best option was just to plumb through an "always_reboot" flag. I called it "always_reboot" and not "component_update" to limit as much as possible how much of the system knows about component updates. PTAL. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/componen... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:72: bool result) { On 2016/11/17 00:39:56, waffles wrote: > Maybe it's time to add a DCHECK_CURRENTLY_ON(content::BrowserThread::UI) here. Done. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:84: tray->SetFlashUpdateAvailable(); On 2016/11/17 00:39:56, waffles wrote: > As an FYI, the updater uses a lot of CONTINUE_ON_SHUTDOWN, so we can wind up > calling this pretty late in the browser's destruction. As long as this is only > run on the UI thread I think this should be safe. Acknowledged. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:185: g_instance = this; On 2016/11/17 00:43:43, James Cook wrote: > DCHECK(!g_instance) above just to make sure no one is instantiating two > instances Done. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:234: SystemTrayDelegateChromeOS::~SystemTrayDelegateChromeOS() { On 2016/11/17 00:43:43, James Cook wrote: > DCHECK_EQ(this, g_instance); > g_instance = nullptr; Done. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:328: if (!info->factory_reset_required && flash_update_available_) On 2016/11/17 00:43:43, James Cook wrote: > Any particular reason not to set update_required if factory_reset_required is > set? Done. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1033: ash::UpdateInfo info; On 2016/11/17 00:43:43, James Cook wrote: > Why not call GetSystemUpdateInfo() and use that result? > > That will make sure GetUpdateInfo() is always called, so if someone adds > something there then this won't break. Done. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1039: } On 2016/11/17 00:43:43, James Cook wrote: > Can you add a test for this? It's unlikely that flash updates are going to get > tested very often by QA. I would suggest adding something to the browser test > (the unit test seems to have a flake problem you probably don't want to deal > with). I think there is a GetTrayUpdateForTest() method in ash you can use to > check if the update icon actually appears. What do you mean by "has a flake problem"? I imagine the unit test is simplest. Otherwise can you point me to which browsertest to add this too? https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right): https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.h:128: static SystemTrayDelegateChromeOS* Get(); On 2016/11/17 00:43:43, James Cook wrote: > put this at the top, near the constructor Done. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.h:129: void SetFlashUpdateAvailable(); On 2016/11/17 00:43:43, James Cook wrote: > function comment please Done.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for sticking with this. Sorry about the SystemTrayClient / SystemTrayDelegate awkwardness -- the code is mid-refactor. https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/20001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1039: } On 2016/11/30 19:30:30, Greg K wrote: > On 2016/11/17 00:43:43, James Cook wrote: > > Can you add a test for this? It's unlikely that flash updates are going to get > > tested very often by QA. I would suggest adding something to the browser test > > (the unit test seems to have a flake problem you probably don't want to deal > > with). I think there is a GetTrayUpdateForTest() method in ash you can use to > > check if the update icon actually appears. > > What do you mean by "has a flake problem"? I imagine the unit test is simplest. > Otherwise can you point me to which browsertest to add this too? A unit test would be nice, but if you look at system_tray_delegate_chromeos_unittest.cc you'll see a single unit test that is disabled for flakiness. I suspect any attempt to create a SystemTrayDelegateChromeOS will trigger the same flake. https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/system_tray_delega... I would add a test to https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/system_tray_delega... https://codereview.chromium.org/2493973003/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray_controller.h (right): https://codereview.chromium.org/2493973003/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray_controller.h:54: void RequestRestartForUpdate(bool always_reboot); Can the "always_reboot" information be cached in the src/chrome layer so that src/ash doesn't need to know about it? From the UI perspective it just needs to show the reboot menu item, it doesn't care why. https://codereview.chromium.org/2493973003/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray_delegate.h (right): https://codereview.chromium.org/2493973003/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray_delegate.h:70: bool component_update_required; nit (if you keep this): Document this. Might be helpful to say that Flash is an example component. https://codereview.chromium.org/2493973003/diff/60001/ash/common/system/updat... File ash/common/system/update/tray_update.cc (right): https://codereview.chromium.org/2493973003/diff/60001/ash/common/system/updat... ash/common/system/update/tray_update.cc:150: views::Label* label_; nit (if you keep this): either label_ = nullptr here or component_update_required_(false) in constructor for consistency https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:278: chrome::NotifyAndTerminate(/*fast_path=*/false,/*always_reboot=*/false); nit: chrome/browser usually does (false /* fast_path */) or declares const bool fast_path = false; https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:135: chrome::NotifyAndTerminate(/*fast_path=*/true,/*always_reboot=*/false); ditto, and throughout https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:378: // // or else signal the session manager to log out. fix formatting please https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.h (right): https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.h:79: void NotifyAndTerminate(bool fast_path, bool always_reboot); nit: document why you might pass true for always_reboot. I'm OK with mentioning Flash and/or component updates. optional: default arguments are allowed, so this could also be always_reboot = false, up to you https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_client.cc (right): https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_client.cc:320: chrome::NotifyAndTerminate(true /* fast_path */, always_reboot); For example, this could query SystemTrayDelegateChromeOS::Get()->GetFlashUpdateAvailable(). (If you do that, be sure to check that SystemTrayDelegateChromeOS::Get() is not null, for my project there's a case where SystemTrayClient exists but SystemTrayDelegate does not. This code is mid-refactor, sorry.) Alternately, this object could store the flash_update_available_ boolean, but you'd still need to call into SystemTrayDelegateChromeOS to trigger showing the update UI.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/browser_... File chrome/browser/browser_shutdown.cc (right): https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/browser_... chrome/browser/browser_shutdown.cc:278: chrome::NotifyAndTerminate(/*fast_path=*/false,/*always_reboot=*/false); On 2016/11/30 23:01:41, James Cook wrote: > nit: chrome/browser usually does (false /* fast_path */) or declares const bool > fast_path = false; Done. https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.cc (right): https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:135: chrome::NotifyAndTerminate(/*fast_path=*/true,/*always_reboot=*/false); On 2016/11/30 23:01:41, James Cook wrote: > ditto, and throughout Done. https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.cc:378: // // or else signal the session manager to log out. On 2016/11/30 23:01:41, James Cook wrote: > fix formatting please Done. https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... File chrome/browser/lifetime/application_lifetime.h (right): https://codereview.chromium.org/2493973003/diff/60001/chrome/browser/lifetime... chrome/browser/lifetime/application_lifetime.h:79: void NotifyAndTerminate(bool fast_path, bool always_reboot); On 2016/11/30 23:01:41, James Cook wrote: > nit: document why you might pass true for always_reboot. I'm OK with mentioning > Flash and/or component updates. > > optional: default arguments are allowed, so this could also be always_reboot = > false, up to you Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_client.cc (right): https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_client.cc:321: #if defined(OS_CHROMEOS) These ifdefs are not needed. chrome/browser/ui/ash is chromeos-only. https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1023: SystemTrayDelegateChromeOS* SystemTrayDelegateChromeOS::Get() { nit: reorder this function (and the ones below) to match the header file declaration order https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc:157: } Nice test. Simple and well documented.
(lgtm sustained)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kerrnel@chromium.org changed reviewers: + thakis@chromium.org
thakis@chromium.org: Please review changes in chrome/ Thanks, Greg https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_client.cc (right): https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_client.cc:321: #if defined(OS_CHROMEOS) On 2016/12/01 20:41:12, James Cook wrote: > These ifdefs are not needed. chrome/browser/ui/ash is chromeos-only. Done. https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:1023: SystemTrayDelegateChromeOS* SystemTrayDelegateChromeOS::Get() { On 2016/12/01 20:41:13, James Cook wrote: > nit: reorder this function (and the ones below) to match the header file > declaration order Done. https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos_browsertest_chromeos.cc:157: } On 2016/12/01 20:41:13, James Cook wrote: > Nice test. Simple and well documented. Acknowledged.
lgtm (Mr Cook, wanna be a chrome/ owner?) https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/lifetim... File chrome/browser/lifetime/application_lifetime.h (right): https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/lifetim... chrome/browser/lifetime/application_lifetime.h:81: // component flash update was applied. Consider using enums instead of bools. Writing enum RebootStyle { kForceReboot, kNoReboot }; void NotifyAndTerminate(bool fast_path, bool always_reboot = kNoReboot); is not any more work, but at the calling side foo->NotifyAndTerminal(foo, ApplicationLifetime::kForceReboot); is way more readable. (Also, style guide discourages default args iirc) https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:113: SystemTrayDelegateChromeOS* g_instance = nullptr; If you hadn't seen it, there's also base/memory/singleton.h but I'm never sure when it's preferable over what you have here (so keep this as-is if you want) https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:237: g_instance = nullptr; oh, i guess in this case Singleton isn't an option anyhow. https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right): https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.h:74: static SystemTrayDelegateChromeOS* Get(); I think we usually call these "instance()": https://cs.chromium.org/search/?q=::instance%5C(%5C)&sq=package:chromium&type=cs
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/02 01:20:07, Nico wrote: > lgtm > > (Mr Cook, wanna be a chrome/ owner?) > > https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/lifetim... > File chrome/browser/lifetime/application_lifetime.h (right): > > https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/lifetim... > chrome/browser/lifetime/application_lifetime.h:81: // component flash update was > applied. > Consider using enums instead of bools. Writing > > enum RebootStyle { kForceReboot, kNoReboot }; > void NotifyAndTerminate(bool fast_path, bool always_reboot = kNoReboot); > > is not any more work, but at the calling side > > foo->NotifyAndTerminal(foo, ApplicationLifetime::kForceReboot); > > is way more readable. (Also, style guide discourages default args iirc) > > https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): > > https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:113: > SystemTrayDelegateChromeOS* g_instance = nullptr; > If you hadn't seen it, there's also base/memory/singleton.h but I'm never sure > when it's preferable over what you have here (so keep this as-is if you want) > > https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:237: g_instance = > nullptr; > oh, i guess in this case Singleton isn't an option anyhow. > > https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right): > > https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/system_tray_delegate_chromeos.h:74: static > SystemTrayDelegateChromeOS* Get(); > I think we usually call these "instance()": > https://cs.chromium.org/search/?q=::instance%5C(%5C)&sq=package:chromium&type=cs I don't think I know enough to be a //chrome owner, but I'd be happy to take on //chrome/browser/ui/ash.
I'll take what I can get :-) want to send a cl? On Dec 1, 2016 10:34 PM, <jamescook@chromium.org> wrote: > On 2016/12/02 01:20:07, Nico wrote: > > lgtm > > > > (Mr Cook, wanna be a chrome/ owner?) > > > > > https://codereview.chromium.org/2493973003/diff/120001/ > chrome/browser/lifetime/application_lifetime.h > > File chrome/browser/lifetime/application_lifetime.h (right): > > > > > https://codereview.chromium.org/2493973003/diff/120001/ > chrome/browser/lifetime/application_lifetime.h#newcode81 > > chrome/browser/lifetime/application_lifetime.h:81: // component flash > update > was > > applied. > > Consider using enums instead of bools. Writing > > > > enum RebootStyle { kForceReboot, kNoReboot }; > > void NotifyAndTerminate(bool fast_path, bool always_reboot = kNoReboot); > > > > is not any more work, but at the calling side > > > > foo->NotifyAndTerminal(foo, ApplicationLifetime::kForceReboot); > > > > is way more readable. (Also, style guide discourages default args iirc) > > > > > https://codereview.chromium.org/2493973003/diff/120001/ > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): > > > > > https://codereview.chromium.org/2493973003/diff/120001/ > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc#newcode113 > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:113: > > SystemTrayDelegateChromeOS* g_instance = nullptr; > > If you hadn't seen it, there's also base/memory/singleton.h but I'm > never sure > > when it's preferable over what you have here (so keep this as-is if you > want) > > > > > https://codereview.chromium.org/2493973003/diff/120001/ > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc#newcode237 > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:237: g_instance = > > nullptr; > > oh, i guess in this case Singleton isn't an option anyhow. > > > > > https://codereview.chromium.org/2493973003/diff/120001/ > chrome/browser/ui/ash/system_tray_delegate_chromeos.h > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right): > > > > > https://codereview.chromium.org/2493973003/diff/120001/ > chrome/browser/ui/ash/system_tray_delegate_chromeos.h#newcode74 > > chrome/browser/ui/ash/system_tray_delegate_chromeos.h:74: static > > SystemTrayDelegateChromeOS* Get(); > > I think we usually call these "instance()": > > > https://cs.chromium.org/search/?q=::instance%5C(%5C)& > sq=package:chromium&type=cs > > I don't think I know enough to be a //chrome owner, but I'd be happy to > take on > //chrome/browser/ui/ash. > > > https://codereview.chromium.org/2493973003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/lifetim... File chrome/browser/lifetime/application_lifetime.h (right): https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/lifetim... chrome/browser/lifetime/application_lifetime.h:81: // component flash update was applied. On 2016/12/02 01:20:07, Nico wrote: > Consider using enums instead of bools. Writing > > enum RebootStyle { kForceReboot, kNoReboot }; > void NotifyAndTerminate(bool fast_path, bool always_reboot = kNoReboot); > > is not any more work, but at the calling side > > foo->NotifyAndTerminal(foo, ApplicationLifetime::kForceReboot); > > is way more readable. (Also, style guide discourages default args iirc) I agree about the enumeration. That being said, I read the style guide and it does encourage using a default argument or function overload for a case like this were only one caller cares about that value. That being said, it says when in doubt, prefer function overloads so that's how I will do this. https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:113: SystemTrayDelegateChromeOS* g_instance = nullptr; On 2016/12/02 01:20:07, Nico wrote: > If you hadn't seen it, there's also base/memory/singleton.h but I'm never sure > when it's preferable over what you have here (so keep this as-is if you want) Acknowledged. https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:237: g_instance = nullptr; On 2016/12/02 01:20:07, Nico wrote: > oh, i guess in this case Singleton isn't an option anyhow. Acknowledged. https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right): https://codereview.chromium.org/2493973003/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.h:74: static SystemTrayDelegateChromeOS* Get(); On 2016/12/02 01:20:07, Nico wrote: > I think we usually call these "instance()": > https://cs.chromium.org/search/?q=::instance%5C(%5C)&sq=package:chromium&type=cs Done.
Still LGTM but I have a small naming suggestion https://codereview.chromium.org/2493973003/diff/140001/chrome/browser/lifetim... File chrome/browser/lifetime/application_lifetime.h (right): https://codereview.chromium.org/2493973003/diff/140001/chrome/browser/lifetim... chrome/browser/lifetime/application_lifetime.h:83: enum class ApplicationLifetime { kForceReboot, kOptionalReboot }; nit: I wouldn't call the class itself ApplicationLifetime. It's more like a RebootPolicy or something. Nico might have been thinking this code was in a class ApplicationLifetime based on the filename.
https://codereview.chromium.org/2493973003/diff/140001/chrome/browser/lifetim... File chrome/browser/lifetime/application_lifetime.h (right): https://codereview.chromium.org/2493973003/diff/140001/chrome/browser/lifetim... chrome/browser/lifetime/application_lifetime.h:83: enum class ApplicationLifetime { kForceReboot, kOptionalReboot }; On 2016/12/03 00:18:20, James Cook wrote: > nit: I wouldn't call the class itself ApplicationLifetime. It's more like a > RebootPolicy or something. Nico might have been thinking this code was in a > class ApplicationLifetime based on the filename. Yes, I thought this was in a class and used a plain non-class enum. Using an enum class is fine if you prefer though; James's suggestion sounds good to me.
https://codereview.chromium.org/2493973003/diff/140001/chrome/browser/lifetim... File chrome/browser/lifetime/application_lifetime.h (right): https://codereview.chromium.org/2493973003/diff/140001/chrome/browser/lifetim... chrome/browser/lifetime/application_lifetime.h:83: enum class ApplicationLifetime { kForceReboot, kOptionalReboot }; On 2016/12/03 00:23:13, Nico wrote: > On 2016/12/03 00:18:20, James Cook wrote: > > nit: I wouldn't call the class itself ApplicationLifetime. It's more like a > > RebootPolicy or something. Nico might have been thinking this code was in a > > class ApplicationLifetime based on the filename. > > Yes, I thought this was in a class and used a plain non-class enum. Using an > enum class is fine if you prefer though; James's suggestion sounds good to me. Done.
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, thakis@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2493973003/#ps160001 (title: "Display "Restart to update" dialog to Chrome OS users.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by kerrnel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/component_updater/pepper_flash_component_installer.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/component_updater/pepper_flash_component_installer.cc:46
error: chrome/browser/component_updater/pepper_flash_component_installer.cc:
patch does not apply
Patch:
chrome/browser/component_updater/pepper_flash_component_installer.cc
Index: chrome/browser/component_updater/pepper_flash_component_installer.cc
diff --git
a/chrome/browser/component_updater/pepper_flash_component_installer.cc
b/chrome/browser/component_updater/pepper_flash_component_installer.cc
index
77e261784161be9dd84eaa1a1d06fabb8090c10f..3cb86a3c474ac1a97645c61189288d37ab74c240
100644
--- a/chrome/browser/component_updater/pepper_flash_component_installer.cc
+++ b/chrome/browser/component_updater/pepper_flash_component_installer.cc
@@ -46,6 +46,7 @@
#if defined(OS_CHROMEOS)
#include "base/feature_list.h"
+#include "chrome/browser/ui/ash/system_tray_delegate_chromeos.h"
#include "chromeos/dbus/dbus_method_call_status.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/dbus/image_loader_client.h"
@@ -80,12 +81,20 @@ const uint8_t kSha2Hash[] = {0xc8, 0xce, 0x99, 0xba, 0xce,
0x89, 0xf8, 0x20,
#if defined(OS_CHROMEOS)
void LogRegistrationResult(chromeos::DBusMethodCallStatus call_status,
bool result) {
+ DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (call_status != chromeos::DBUS_METHOD_CALL_SUCCESS) {
LOG(ERROR) << "Call to imageloader service failed.";
return;
}
- if (!result)
+ if (!result) {
LOG(ERROR) << "Component flash registration failed";
+ return;
+ }
+ chromeos::SystemTrayDelegateChromeOS* tray =
+ chromeos::SystemTrayDelegateChromeOS::instance();
+ if (tray) {
+ tray->SetFlashUpdateAvailable();
+ }
}
void ImageLoaderRegistration(const std::string& version,
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, jamescook@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2493973003/#ps180001 (title: "Rebased CL")
The CQ bit was unchecked by kerrnel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, jamescook@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2493973003/#ps200001 (title: "Rebased CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481064176840760,
"parent_rev": "885f3facd006b2cf6c3c9caeec3e39317505a899", "commit_rev":
"ca2b94ce28a13953f0c3723eebdf16703818931a"}
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Display "Restart to update" dialog to Chrome OS users. This displays the "restart to update" message in the system tray to Chrome OS users when a flash update is available. BUG=630420 ========== to ========== Display "Restart to update" dialog to Chrome OS users. This displays the "restart to update" message in the system tray to Chrome OS users when a flash update is available. BUG=630420 Committed: https://crrev.com/5aa7ae3264bd376e48eb283a4977dfc86b98dc11 Cr-Commit-Position: refs/heads/master@{#436782} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5aa7ae3264bd376e48eb283a4977dfc86b98dc11 Cr-Commit-Position: refs/heads/master@{#436782}
Message was sent while issue was closed.
Greg, I'm working in the update code right now and I noticed this. Let me know if you'd like me to fix it in the CL I'm working on. https://codereview.chromium.org/2493973003/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): https://codereview.chromium.org/2493973003/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:334: info->update_required = true; Hey, I just thought of something. If the only update pending is the flash update then the ash::UpdateInfo::severity will be UPDATE_NONE. Luckily the icon still shows in this case, but it's the lowest-priority update color. I think this should explicitly set UPDATE_LOW (or whatever severity you think is appropriate).
Message was sent while issue was closed.
Thanks for bringing that up, and if you don't mind fixing it as long as you're in there, I would really appreciate. Otherwise I'm happy to do it. - Greg On Thu, Dec 8, 2016 at 4:11 PM, <jamescook@chromium.org> wrote: > Greg, I'm working in the update code right now and I noticed this. > > Let me know if you'd like me to fix it in the CL I'm working on. > > > > https://codereview.chromium.org/2493973003/diff/200001/ > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): > > https://codereview.chromium.org/2493973003/diff/200001/ > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc#newcode334 > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:334: > info->update_required = true; > Hey, I just thought of something. If the only update pending is the > flash update then the ash::UpdateInfo::severity will be UPDATE_NONE. > Luckily the icon still shows in this case, but it's the lowest-priority > update color. > > I think this should explicitly set UPDATE_LOW (or whatever severity you > think is appropriate). > > https://codereview.chromium.org/2493973003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/12/09 00:12:30, Greg K wrote: > Thanks for bringing that up, and if you don't mind fixing it as long as > you're in there, I would really appreciate. Otherwise I'm happy to do it. > > - Greg > > On Thu, Dec 8, 2016 at 4:11 PM, <mailto:jamescook@chromium.org> wrote: > > > Greg, I'm working in the update code right now and I noticed this. > > > > Let me know if you'd like me to fix it in the CL I'm working on. > > > > > > > > https://codereview.chromium.org/2493973003/diff/200001/ > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): > > > > https://codereview.chromium.org/2493973003/diff/200001/ > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc#newcode334 > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:334: > > info->update_required = true; > > Hey, I just thought of something. If the only update pending is the > > flash update then the ash::UpdateInfo::severity will be UPDATE_NONE. > > Luckily the icon still shows in this case, but it's the lowest-priority > > update color. > > > > I think this should explicitly set UPDATE_LOW (or whatever severity you > > think is appropriate). > > > > https://codereview.chromium.org/2493973003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hi, This is Chan Li from Findit team. I am looking for possible cause for flaky test AppMenuControllerTest.RecentTabsFavIcon, could you help me and check if this change could cause the flakiness? Thank you so much!
Message was sent while issue was closed.
On 2016/12/10 20:17:19, chanli wrote: > On 2016/12/09 00:12:30, Greg K wrote: > > Thanks for bringing that up, and if you don't mind fixing it as long as > > you're in there, I would really appreciate. Otherwise I'm happy to do it. > > > > - Greg > > > > On Thu, Dec 8, 2016 at 4:11 PM, <mailto:jamescook@chromium.org> wrote: > > > > > Greg, I'm working in the update code right now and I noticed this. > > > > > > Let me know if you'd like me to fix it in the CL I'm working on. > > > > > > > > > > > > https://codereview.chromium.org/2493973003/diff/200001/ > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc > > > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right): > > > > > > https://codereview.chromium.org/2493973003/diff/200001/ > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc#newcode334 > > > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:334: > > > info->update_required = true; > > > Hey, I just thought of something. If the only update pending is the > > > flash update then the ash::UpdateInfo::severity will be UPDATE_NONE. > > > Luckily the icon still shows in this case, but it's the lowest-priority > > > update color. > > > > > > I think this should explicitly set UPDATE_LOW (or whatever severity you > > > think is appropriate). > > > > > > https://codereview.chromium.org/2493973003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Hi, > > This is Chan Li from Findit team. I am looking for possible cause for flaky test > AppMenuControllerTest.RecentTabsFavIcon, could you help me and check if this > change could cause the flakiness? Thank you so much! I don't think that's related. RecentTabsFavIcon seems to be Mac-only, and this code only affects Chrome OS. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
