Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1420163003:
Fixed Windows system tray icon. (Closed)
|
Created:
5 years, 1 month ago by Matt Giuca Modified:
5 years ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@profile-icon-imagefamily Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed Windows system tray icon.
Previously was using a 22x22 image, then Windows was scaling it to the
desired size (usually 16x16, but not necessarily). Now, selects the
appropriate icon from the Chrome ICO file and scales it using a
high-quality scaling algorithm to the desired size.
This should also mean that on Chrome Canary, the system tray icon is the
golden Canary logo.
NOTE: This new algorithm may add several milliseconds to startup time.
If this causes perf issues, revert and we can work out a way to do this
without blocking the startup path.
BUG=124141, 351874
Committed: https://crrev.com/8367e724a57ac3300262d39b4ec15f2d410cc80d
Cr-Commit-Position: refs/heads/master@{#361597}
Patch Set 1 #Patch Set 2 : Post a delayed task to avoid increasing startup time. #Patch Set 3 : Rebase (had to inline GetAppIconForSize because it got deleted upstream). #Patch Set 4 : Remove IDR_STATUS_TRAY_ICON on Win, and update tests to account for this. #
Total comments: 20
Patch Set 5 : Address msw comments. #Patch Set 6 : Rebase. #Patch Set 7 : Avoid posting after-startup task in test. #Patch Set 8 : Fix crash (avoid async call from DisplayClientInstalledNotification). #
Total comments: 2
Patch Set 9 : Revert changes to start in a delayed task. #
Messages
Total messages: 37 (12 generated)
mgiuca@chromium.org changed reviewers: + atwilson@chromium.org, msw@chromium.org, oshima@chromium.org
atwilson: All please. oshima: chrome_unscaled_resources.grd msw: status_tray_win_unittest.cc (aside: shouldn't atwilson have OWNERS in chrome/browser/ui/views/status_icons?) I changed the creation of the sys tray icon to a delayed task because of the potential additional startup time. In my experiments this can cause the icon to appear 3--5 seconds after starting the browser. Is that undesirable? The alternative is adding 3--6 ms of extra time on the UI thread during startup. See benchmarks here: https://codereview.chromium.org/1408063012#msg10
https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... chrome/browser/background/background_mode_manager.cc:921: if (!family) Maybe invert the check to conditionally return the scaled image, and then change the #else to #endif to let the code fall through and return the static image on errors. (then you'll want to keep the IDR_STATUS_TRAY_ICON definition on Win) https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... File chrome/browser/status_icons/status_tray_unittest.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:6: #include "base/strings/string_util.h" Remove; not used. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:40: // Note: IDR_STATUS_TRAY_ICON is not available on Windows, so just create a nit: consider "Just create a dummy icon image; the actual image is irrelevant here." https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:42: gfx::ImageSkia image = gfx::test::CreateImageSkia(16, 16); nit: inline below? https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:44: StatusTray::OTHER_ICON, image, base::ASCIIToUTF16("tool tip")); nit: use a blank string16() and remove utf_string_conversions.h https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:9: #include "base/strings/string_util.h" Remove https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:60: gfx::ImageSkia image = gfx::test::CreateImageSkia(16, 16); ditto: inline https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:62: StatusTray::OTHER_ICON, image, base::ASCIIToUTF16("tool tip")); ditto: string16(), remove utf_string_conversions.h https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:87: StatusIconWin* icon = static_cast<StatusIconWin*>(CreateStatusIcon(&tray)); nit: maybe include the static_cast in the helper and return StatusIconWin*?
https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... chrome/browser/background/background_mode_manager.cc:921: if (!family) On 2015/11/12 00:43:29, msw wrote: > Maybe invert the check to conditionally return the scaled image, and then change > the #else to #endif to let the code fall through and return the static image on > errors. (then you'll want to keep the IDR_STATUS_TRAY_ICON definition on Win) There should never be an error here (in fact, I'll add a DCHECK here). Falling back to a secondary (and buggy) icon which is otherwise never needed on Windows is a bad idea. You wouldn't suggest adding this code and resource if it didn't already exist (on other platforms), so why add it as a fallback? https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... File chrome/browser/status_icons/status_tray_unittest.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:6: #include "base/strings/string_util.h" On 2015/11/12 00:43:29, msw wrote: > Remove; not used. Done. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:40: // Note: IDR_STATUS_TRAY_ICON is not available on Windows, so just create a On 2015/11/12 00:43:29, msw wrote: > nit: consider "Just create a dummy icon image; the actual image is irrelevant > here." Done. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:42: gfx::ImageSkia image = gfx::test::CreateImageSkia(16, 16); On 2015/11/12 00:43:29, msw wrote: > nit: inline below? Done. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/status_i... chrome/browser/status_icons/status_tray_unittest.cc:44: StatusTray::OTHER_ICON, image, base::ASCIIToUTF16("tool tip")); On 2015/11/12 00:43:29, msw wrote: > nit: use a blank string16() and remove utf_string_conversions.h Done. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:9: #include "base/strings/string_util.h" On 2015/11/12 00:43:29, msw wrote: > Remove Done. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:60: gfx::ImageSkia image = gfx::test::CreateImageSkia(16, 16); On 2015/11/12 00:43:29, msw wrote: > ditto: inline Done. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:62: StatusTray::OTHER_ICON, image, base::ASCIIToUTF16("tool tip")); On 2015/11/12 00:43:29, msw wrote: > ditto: string16(), remove utf_string_conversions.h Done. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/status_icons/status_tray_win_unittest.cc:87: StatusIconWin* icon = static_cast<StatusIconWin*>(CreateStatusIcon(&tray)); On 2015/11/12 00:43:29, msw wrote: > nit: maybe include the static_cast in the helper and return StatusIconWin*? Done.
lgtm with a nit and a q. https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... chrome/browser/background/background_mode_manager.cc:921: if (!family) On 2015/11/12 04:44:39, Matt Giuca wrote: > On 2015/11/12 00:43:29, msw wrote: > > Maybe invert the check to conditionally return the scaled image, and then > change > > the #else to #endif to let the code fall through and return the static image > on > > errors. (then you'll want to keep the IDR_STATUS_TRAY_ICON definition on Win) > > There should never be an error here (in fact, I'll add a DCHECK here). > > Falling back to a secondary (and buggy) icon which is otherwise never needed on > Windows is a bad idea. You wouldn't suggest adding this code and resource if it > didn't already exist (on other platforms), so why add it as a fallback? Fair enough... Where is that image used? Can it be removed? nit: ternary here return family ? family->CreateExact(size).AsImageSkia() : gfx::ImageSkia();
c/a/theme lgtm
https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/1420163003/diff/60001/chrome/browser/backgrou... chrome/browser/background/background_mode_manager.cc:921: if (!family) On 2015/11/12 18:27:23, msw wrote: > Fair enough... Where is that image used? Can it be removed? It is used for the status tray on Mac and Linux (because they don't use Windows ICO loading code). It can't be removed there. But it is being removed (in this CL) on Windows, so the resource no longer exists. > nit: ternary here > return family ? family->CreateExact(size).AsImageSkia() : gfx::ImageSkia(); I prefer not to use that style because "if (!family)" is an error case. I think it's preferable to write: if (error happened) report error and exit normal code Instead of return (!error happened) ? normal code : error code
lgtm
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1420163003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420163003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Fixed Windows system tray icon. Previously was using a 22x22 image, then Windows was scaling it to the desired size (usually 16x16, but not necessarily). Now, selects the appropriate icon from the Chrome ICO file and scales it using a high-quality scaling algorithm to the desired size. Also changes loading the status tray icon to take place in a delayed task, so it does not block startup. (This is because the new high-quality icon loading code may be somewhat slower than the old code --- on the order of milliseconds.) BUG=124141 ========== to ========== Fixed Windows system tray icon. Previously was using a 22x22 image, then Windows was scaling it to the desired size (usually 16x16, but not necessarily). Now, selects the appropriate icon from the Chrome ICO file and scales it using a high-quality scaling algorithm to the desired size. Also changes loading the status tray icon to take place in a delayed task, so it does not block startup. (This is because the new high-quality icon loading code may be somewhat slower than the old code --- on the order of milliseconds.) This should also mean that on Chrome Canary, the system tray icon is the golden Canary logo. BUG=124141,351874 ==========
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1420163003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420163003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1420163003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420163003/140001
atwilson: Could you take another look at PS7? The issue that came up is that we had one caller of CreateStatusTrayIcon() that was expecting |status_icon_| to be available right away (not expecting async). I fixed that by calling the sync method CreateStatusTrayIconTask(). It's a little scary though, because we could have other calls like it --- anywhere in the call hierarchy --- that expect it to be available right away. I did an audit and didn't find any other problems, but I want to make sure you're happy to go ahead with making this async. It is a pretty big semantic change for BackgroundModeManager.
On 2015/11/23 04:05:09, Matt Giuca wrote: > atwilson: Could you take another look at PS7? Sorry, I mean PS8.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
See my suggestion in the comments (have CreateStatusTrayIcon always create a static icon synchronously, but then update the icon via a task, if you are concerned about scaling being slow). Have you measured the time it takes to scale the icon? Is it significant given the overall work we have to do to create a status tray icon at all? https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... chrome/browser/background/background_mode_manager.cc:896: content::BrowserThread::PostAfterStartupTask( This seems incorrect, because it's possible that someone may have called RemoveStatusTrayIcon() while this task was queued (for example, if we are no longer in background mode because an extension has been uninstalled, etc). If we can't take the time during startup to scale an icon, can we instead just use a static resource image immediately, then queue up a task to replace that image with a scaled one later via StatusIcon::SetImage()?
On 2015/11/23 11:28:11, Andrew T Wilson (Slow) wrote: > See my suggestion in the comments (have CreateStatusTrayIcon always create a > static icon synchronously, but then update the icon via a task, if you are > concerned about scaling being slow). > > Have you measured the time it takes to scale the icon? Is it significant given > the overall work we have to do to create a status tray icon at all? Yeah, kinda. The time to load the icons using the new code versus the old code is here: https://codereview.chromium.org/1408063012/#msg10 It adds about 3--6 ms to startup time. I'm not sure if that's significant enough to care about, but I considered it enough to do a delayed task. If you disagree, we can go back to just doing it at startup. But note that those measurements were based on *having the exact size already present* so there is no actual scaling occurring. I haven't tested with scaling. I'll have to set up the benchmark environment again to do this (I can do that tomorrow). I also haven't measured the overall time to create the status tray icon. But note that I am moving that whole process to a delayed task. Arguably, that should be happening in a delayed task anyway if it takes more than a few ms, because it isn't critical to startup. So I think this is on the right track. https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... File chrome/browser/background/background_mode_manager.cc (right): https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... chrome/browser/background/background_mode_manager.cc:896: content::BrowserThread::PostAfterStartupTask( On 2015/11/23 11:28:11, Andrew T Wilson (Slow) wrote: > This seems incorrect, because it's possible that someone may have called > RemoveStatusTrayIcon() while this task was queued (for example, if we are no > longer in background mode because an extension has been uninstalled, etc). That's a good point. But we can work around it by having a "pending" Boolean which is set to true by calling CreateStatusTrayIcon and false by RemoveStatusTrayIcon, then CreateStatusTrayIconTask checks that the Boolean is still set to true. > If we can't take the time during startup to scale an icon, can we instead just > use a static resource image immediately, then queue up a task to replace that > image with a scaled one later via StatusIcon::SetImage()? Hmm, the reason I am doing this is because the static resource is extremely ugly (because Windows scales it to a size that we can't control for). So I think that will be a weird experience to see an ugly icon, then have it snap to a nice one. I'd rather not.
On 2015/11/24 07:07:46, Matt Giuca wrote: > On 2015/11/23 11:28:11, Andrew T Wilson (Slow) wrote: > > See my suggestion in the comments (have CreateStatusTrayIcon always create a > > static icon synchronously, but then update the icon via a task, if you are > > concerned about scaling being slow). > > > > Have you measured the time it takes to scale the icon? Is it significant given > > the overall work we have to do to create a status tray icon at all? > > Yeah, kinda. The time to load the icons using the new code versus the old code > is here: > https://codereview.chromium.org/1408063012/#msg10 > > It adds about 3--6 ms to startup time. I'm not sure if that's significant enough > to care about, but I considered it enough to do a delayed task. If you disagree, > we can go back to just doing it at startup. > > But note that those measurements were based on *having the exact size already > present* so there is no actual scaling occurring. I haven't tested with scaling. > I'll have to set up the benchmark environment again to do this (I can do that > tomorrow). > > I also haven't measured the overall time to create the status tray icon. But > note that I am moving that whole process to a delayed task. Arguably, that > should be happening in a delayed task anyway if it takes more than a few ms, > because it isn't critical to startup. So I think this is on the right track. > > https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... > File chrome/browser/background/background_mode_manager.cc (right): > > https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... > chrome/browser/background/background_mode_manager.cc:896: > content::BrowserThread::PostAfterStartupTask( > On 2015/11/23 11:28:11, Andrew T Wilson (Slow) wrote: > > This seems incorrect, because it's possible that someone may have called > > RemoveStatusTrayIcon() while this task was queued (for example, if we are no > > longer in background mode because an extension has been uninstalled, etc). > > That's a good point. But we can work around it by having a "pending" Boolean > which is set to true by calling CreateStatusTrayIcon and false by > RemoveStatusTrayIcon, then CreateStatusTrayIconTask checks that the Boolean is > still set to true. My preference would be to just check in this change without the background task - typically, if Chrome is running in background mode, it's being launched at startup with no open windows, so the couple of milliseconds of additional work would not be visible to the user anyway. That said, if you feel strongly about avoiding this latency, I'd also be OK with the pending boolean + new tests to cover this case. But I don't think it's worth the work. > > > If we can't take the time during startup to scale an icon, can we instead just > > use a static resource image immediately, then queue up a task to replace that > > image with a scaled one later via StatusIcon::SetImage()? > > Hmm, the reason I am doing this is because the static resource is extremely ugly > (because Windows scales it to a size that we can't control for). So I think that > will be a weird experience to see an ugly icon, then have it snap to a nice one. > I'd rather not. That's fine, although I suspect the icon wouldn't be visible long enough for people to notice. Anyhow, I'm pretty flexible with however you want to go here - do nothing (just create the icon at startup), add a pending boolean and create the icon via a task, or use an ugly icon and then prettify-it. I appreciate you taking the time to work on this - wherever we end up will be way better than the current ugly-icon world.
On 2015/11/24 15:55:35, Andrew T Wilson (Slow) wrote: > On 2015/11/24 07:07:46, Matt Giuca wrote: > > On 2015/11/23 11:28:11, Andrew T Wilson (Slow) wrote: > > > See my suggestion in the comments (have CreateStatusTrayIcon always create a > > > static icon synchronously, but then update the icon via a task, if you are > > > concerned about scaling being slow). > > > > > > Have you measured the time it takes to scale the icon? Is it significant > given > > > the overall work we have to do to create a status tray icon at all? > > > > Yeah, kinda. The time to load the icons using the new code versus the old code > > is here: > > https://codereview.chromium.org/1408063012/#msg10 > > > > It adds about 3--6 ms to startup time. I'm not sure if that's significant > enough > > to care about, but I considered it enough to do a delayed task. If you > disagree, > > we can go back to just doing it at startup. > > > > But note that those measurements were based on *having the exact size already > > present* so there is no actual scaling occurring. I haven't tested with > scaling. > > I'll have to set up the benchmark environment again to do this (I can do that > > tomorrow). > > > > I also haven't measured the overall time to create the status tray icon. But > > note that I am moving that whole process to a delayed task. Arguably, that > > should be happening in a delayed task anyway if it takes more than a few ms, > > because it isn't critical to startup. So I think this is on the right track. > > > > > https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... > > File chrome/browser/background/background_mode_manager.cc (right): > > > > > https://codereview.chromium.org/1420163003/diff/140001/chrome/browser/backgro... > > chrome/browser/background/background_mode_manager.cc:896: > > content::BrowserThread::PostAfterStartupTask( > > On 2015/11/23 11:28:11, Andrew T Wilson (Slow) wrote: > > > This seems incorrect, because it's possible that someone may have called > > > RemoveStatusTrayIcon() while this task was queued (for example, if we are no > > > longer in background mode because an extension has been uninstalled, etc). > > > > That's a good point. But we can work around it by having a "pending" Boolean > > which is set to true by calling CreateStatusTrayIcon and false by > > RemoveStatusTrayIcon, then CreateStatusTrayIconTask checks that the Boolean is > > still set to true. > > My preference would be to just check in this change without the background task > - typically, if Chrome is running in background mode, it's being launched at > startup with no open windows, so the couple of milliseconds of additional work > would not be visible to the user anyway. OK, I'm fine with that (and I definitely think it helps to split up the icon change from the backgrounding anyway). I'll go ahead and remove the backgrounding part of this CL, then we can check the perf dashboards and see if there is enough of a regression to care about. "Premature optimization is the root of all evil." etc. > That said, if you feel strongly about avoiding this latency, I'd also be OK with > the pending boolean + new tests to cover this case. But I don't think it's worth > the work. I don't feel particularly strongly. I just expected backlash if I introduced latency to startup ;) But let's wait and see.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1420163003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420163003/160001
Removed the delayed task code and updated CL description. Now this is *just* fixing the icon and not changing the way the status tray is created.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, atwilson@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1420163003/#ps160001 (title: "Revert changes to start in a delayed task.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1420163003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1420163003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8367e724a57ac3300262d39b4ec15f2d410cc80d Cr-Commit-Position: refs/heads/master@{#361597} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
