Moves the notification icon out of the status area overflow.

Moves the notification icon out of the status area overflow.

This only happens the first time it is created per data dir,
so that the user can change back their setting if desired.

This approach uses COM to talk with Windows Explorer to
change the user's notification area preferences.

R=dimich@chromium.org
BUG=347693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265807

[dewittj, 2013-12-19 01:10:12]

This is just a preliminary review, not ready for shipping.

TODOs are:
* Possibly move the contents of itray_notify.h into
web_notification_tray_win.cc's anonymous namespace.
* Only ensure the icon if we have never done so before (using prefs) and if the
user hasn't explicitly set it to be hidden.
* Report failures via UMA so we can get a clear picture of what versions don't
work.

[Dmitry Titov, 2014-01-03 23:49:51]

Great TODOs, love to see them done, although perhaps not a single patch.

Some relatively small nitpicks:

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:115: } else
if (interface_version_ == INTERFACE_VERSION_LEGACY) {
It rather looks like a switch (interface_version) {}...

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:152: if
(notify_item->hWnd == window_ && notify_item->uID == icon_id_) {
does this check repeats most of the previous? Why not the whole thing or
removed?

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:157: if (hr
== S_OK)
in some places you check FAILED(hr), in some hr == S_OK. Either shoudl use
FAILED/SUCCEEDED, or == S_OK/!= S_OK...

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:179:
tray_notify_win8->RegisterCallback(this, &callback_id);
this sequence of calls will need a good comment. It looks scary :-)

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:191:
tray_notify->RegisterCallback(this);
same as above.

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:250: void
WebNotificationTray::EnforceStatusIconVisible(StatusIcon* icon) {
This method does not need an argument.

[dewittj, 2014-02-14 17:44:31]

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:115: } else
if (interface_version_ == INTERFACE_VERSION_LEGACY) {
On 2014/01/03 23:49:51, Dmitry Titov wrote:
> It rather looks like a switch (interface_version) {}...

Done.

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:152: if
(notify_item->hWnd == window_ && notify_item->uID == icon_id_) {
On 2014/01/03 23:49:51, Dmitry Titov wrote:
> does this check repeats most of the previous? Why not the whole thing or
> removed?

Done.

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:157: if (hr
== S_OK)
On 2014/01/03 23:49:51, Dmitry Titov wrote:
> in some places you check FAILED(hr), in some hr == S_OK. Either shoudl use
> FAILED/SUCCEEDED, or == S_OK/!= S_OK...

Done.

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:179:
tray_notify_win8->RegisterCallback(this, &callback_id);
On 2014/01/03 23:49:51, Dmitry Titov wrote:
> this sequence of calls will need a good comment. It looks scary :-)

Done.

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:191:
tray_notify->RegisterCallback(this);
On 2014/01/03 23:49:51, Dmitry Titov wrote:
> same as above.

Done.

https://codereview.chromium.org/110693004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:250: void
WebNotificationTray::EnforceStatusIconVisible(StatusIcon* icon) {
On 2014/01/03 23:49:51, Dmitry Titov wrote:
> This method does not need an argument.

Done.

[dewittj, 2014-03-10 20:27:25]

Would you mind giving this one more look before I broaden the review?

[Dmitry Titov, 2014-03-18 22:34:13]

Looks good, some comments:

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.cc (right):

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:101: // Explorer.exe
maybe add "which happens synchronously once callback is registered"? It is not
readily obvious from the code, although I think this is what happens, right?

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.h (right):

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:19:
PREFERENCE_SHOW_WHEN_ACTIVE = 0x00,
that leading 0 is mysterious...

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:29: // This structure
outlives the process that creates the status item.
This comment is unclear. This structure or the registry information passed into
Explorer using this structure outlives the process?

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:51: explicit
TrayWatcherWin(UINT icon_id, HWND window);
no need to have 'explicit' with 2 params.

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:53: // Creates an
instance of TrayNotify, and ensures that it supports either
The comment and the name of method seem to talk about different things.

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:100: void
WebNotificationTray::JoinWorkerThread() { worker_thread_.reset(); }
I think Thread wants its Stop() to be called explicitly before destruction on
subclasses. Even though you don't' subclass, it still can be a good idea, to
blend in.

[dewittj, 2014-03-19 18:36:14]

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.cc (right):

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:101: // Explorer.exe
On 2014/03/18 22:34:13, Dmitry Titov wrote:
> maybe add "which happens synchronously once callback is registered"? It is not
> readily obvious from the code, although I think this is what happens, right?

Done.

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.h (right):

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:19:
PREFERENCE_SHOW_WHEN_ACTIVE = 0x00,
On 2014/03/18 22:34:13, Dmitry Titov wrote:
> that leading 0 is mysterious...

Done.

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:29: // This structure
outlives the process that creates the status item.
On 2014/03/18 22:34:13, Dmitry Titov wrote:
> This comment is unclear. This structure or the registry information passed
into
> Explorer using this structure outlives the process?

Done.

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:51: explicit
TrayWatcherWin(UINT icon_id, HWND window);
On 2014/03/18 22:34:13, Dmitry Titov wrote:
> no need to have 'explicit' with 2 params.

Done.

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:53: // Creates an
instance of TrayNotify, and ensures that it supports either
On 2014/03/18 22:34:13, Dmitry Titov wrote:
> The comment and the name of method seem to talk about different things.

Done.

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:100: void
WebNotificationTray::JoinWorkerThread() { worker_thread_.reset(); }
On 2014/03/18 22:34:13, Dmitry Titov wrote:
> I think Thread wants its Stop() to be called explicitly before destruction on
> subclasses. Even though you don't' subclass, it still can be a good idea, to
> blend in.

I have added Stop for clarity even though I don't think that it's required if
I'm not subclassing Thread.

[Dmitry Titov, 2014-03-19 18:38:04]

lgtm

[dewittj, 2014-03-19 18:38:17]

atwilson: Please review status_icons changes
sky: Please review all, since you own c/b/ui/views/status_icons and were on the
conversation about COM initialization on other threads with cpu.

[sky, 2014-03-19 22:26:38]

+cpu as he knows COM more than I.

Is there a particular reason this is private?

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:70: StatusIconWin* AsStatusIconWin();
This means the parent class knows about a UI specific implementation. In general
we don't want the base classes knowing about specific implementations. You need
to find another way to go about this.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.cc (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:45:
interface_version_(INTERFACE_VERSION_UNKNOWN) {
Order of member list should match that of declaration.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:52: bool
TrayWatcherWin::CreateTrayNotify() {
Order of definition/declaration should still match.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:45: class
TrayWatcherWin : public INotificationCB,
Add a description.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:69: ~TrayWatcherWin();
destructor should be beneath constructor and virtual (since you're subclassing
other classes). Actually, shouldn't this be protected since you're subclassing
IUknownImpl?

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:105: UINT icon_id_;
descriptions for these, and make const if you can.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:108: };
DISALLOW...

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc
(right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc:18: class
TrayWatcherWinTest : public InProcessBrowserTest {
Is there a reason these tests need to be browser tests? And separately, if these
get sharded is there a problem if the same tests are running at the same time?

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc:80: };
DISALLOW...

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray.h:108:
BooleanPrefMember did_force_tray_visible_;
nit: newline 107/108.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_icon_win.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_icon_win.h:43: HWND hwnd() const {
return window_; }
style guide says accessor and member should match names. So, either window() or
rename to member to hwnd_.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.h:15: // Returns true if
the browser is currently running in a mode that disallows
nit: members after constructor/destructor.

[cpu_(ooo_6.6-7.5), 2014-03-19 23:14:27]

are you guys aware of these limitations here
http://www.geoffchappell.com/studies/windows/shell/explorer/interfaces/itrayn...

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.cc (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:17: // ItrayNotify
GUID
can you not use attributes and instead use

class __declspec(uuid("FB852B2C-6BAD-4605-9551-F15F87830935"))

[dewittj, 2014-03-20 19:31:08]

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:70: StatusIconWin* AsStatusIconWin();
On 2014/03/19 22:26:39, sky wrote:
> This means the parent class knows about a UI specific implementation. In
general
> we don't want the base classes knowing about specific implementations. You
need
> to find another way to go about this.

Perhaps atwilson can suggest a better way.  In earlier iterations of this code
I'd used a static_cast<StatusIconWin*>(status_icon) but it seemed safer to avoid
static casts and just return NULL in scenarios where it's unsafe.  The trivial
change is for this to return a bool, and use that to inform whether a
static_cast is accepatble but I doubt that would actually satisfy your concern.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.cc (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:17: // ItrayNotify
GUID
On 2014/03/19 23:14:27, cpu wrote:
> can you not use attributes and instead use
> 
> class __declspec(uuid("FB852B2C-6BAD-4605-9551-F15F87830935"))
> 
> 

Done.

For my education, can you explain why you prefer one over the other?

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:45:
interface_version_(INTERFACE_VERSION_UNKNOWN) {
On 2014/03/19 22:26:39, sky wrote:
> Order of member list should match that of declaration.

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:52: bool
TrayWatcherWin::CreateTrayNotify() {
On 2014/03/19 22:26:39, sky wrote:
> Order of definition/declaration should still match.

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:45: class
TrayWatcherWin : public INotificationCB,
On 2014/03/19 22:26:39, sky wrote:
> Add a description.

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:69: ~TrayWatcherWin();
On 2014/03/19 22:26:39, sky wrote:
> destructor should be beneath constructor and virtual (since you're subclassing
> other classes). Actually, shouldn't this be protected since you're subclassing
> IUknownImpl?

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:105: UINT icon_id_;
On 2014/03/19 22:26:39, sky wrote:
> descriptions for these, and make const if you can.

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:108: };
On 2014/03/19 22:26:39, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc
(right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc:18: class
TrayWatcherWinTest : public InProcessBrowserTest {
On 2014/03/19 22:26:39, sky wrote:
> Is there a reason these tests need to be browser tests? And separately, if
these
> get sharded is there a problem if the same tests are running at the same time?

I have put it into the interactive_ui_tests target so that it's only running one
at a time.  It certainly runs well as a unit test but I didn't know if they were
allowed in interactive_ui_tests.  Any thoughts?

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc:80: };
On 2014/03/19 22:26:39, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray.h:108:
BooleanPrefMember did_force_tray_visible_;
On 2014/03/19 22:26:39, sky wrote:
> nit: newline 107/108.

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_icon_win.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_icon_win.h:43: HWND hwnd() const {
return window_; }
On 2014/03/19 22:26:39, sky wrote:
> style guide says accessor and member should match names. So, either window()
or
> rename to member to hwnd_.

Done.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.h:15: // Returns true if
the browser is currently running in a mode that disallows
On 2014/03/19 22:26:39, sky wrote:
> nit: members after constructor/destructor.

Even static functions?  I was under the impression that they came first.

[sky, 2014-03-20 20:59:38]

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:70: StatusIconWin* AsStatusIconWin();
On 2014/03/20 19:31:08, dewittj wrote:
> On 2014/03/19 22:26:39, sky wrote:
> > This means the parent class knows about a UI specific implementation. In
> general
> > we don't want the base classes knowing about specific implementations. You
> need
> > to find another way to go about this.
> 
> Perhaps atwilson can suggest a better way.  In earlier iterations of this code
> I'd used a static_cast<StatusIconWin*>(status_icon) but it seemed safer to
avoid
> static casts and just return NULL in scenarios where it's unsafe.  The trivial
> change is for this to return a bool, and use that to inform whether a
> static_cast is accepatble but I doubt that would actually satisfy your
concern.

If you can't actually have your code take a StatusIconWin, then yes, a
static_cast is what you should do.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc
(right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win_browsertest.cc:18: class
TrayWatcherWinTest : public InProcessBrowserTest {
On 2014/03/20 19:31:08, dewittj wrote:
> On 2014/03/19 22:26:39, sky wrote:
> > Is there a reason these tests need to be browser tests? And separately, if
> these
> > get sharded is there a problem if the same tests are running at the same
time?
> 
> I have put it into the interactive_ui_tests target so that it's only running
one
> at a time.  It certainly runs well as a unit test but I didn't know if they
were
> allowed in interactive_ui_tests.  Any thoughts?

I'm not sure if there is a way to say your test can't run in parallel. Pawel
could likely tell you for sure.

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.h (right):

https://codereview.chromium.org/110693004/diff/310001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.h:15: // Returns true if
the browser is currently running in a mode that disallows
On 2014/03/20 19:31:08, dewittj wrote:
> On 2014/03/19 22:26:39, sky wrote:
> > nit: members after constructor/destructor.
> 
> Even static functions?  I was under the impression that they came first.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
, and yes, we get this wrong in many places.

[Andrew T Wilson (Slow), 2014-03-21 09:15:12]

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:21: class StatusIconWin;
Yeah, I don't like this, even though I understand why you don't like static
casts. It's OK for code running on OS_WIN to assume that all StatusIcons are
actually StatusIconWins, and just do a static cast.

Taking a step back, it seems like you only need StatusIconWin for two reasons:

1) In WebNotificationTray::EnforceStatusIconVisible() to make a status icon
visible.
2) In a browser test to grab out some window internals.

Instead of putting the functionality to make a status icon visible into a
separate class, consider exposing a virtual ForceVisible() API on this class.
This would be a no-op on most platforms, but StatusIconWin() could basically do
what WebNotificationTray::EnforceStatusIconVisible() does now, and
WebNotificationTray::EnforceStatusIconVisible() could just call
status_icon_->ForceVisible().

I think that fits in better with the overall class design. And for the browser
test you can do whatever (static casts, etc - it's just a test).

[sky, 2014-03-21 16:11:21]

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:21: class StatusIconWin;
On 2014/03/21 09:15:13, Andrew T Wilson wrote:
> Yeah, I don't like this, even though I understand why you don't like static
> casts. It's OK for code running on OS_WIN to assume that all StatusIcons are
> actually StatusIconWins, and just do a static cast.
> 
> Taking a step back, it seems like you only need StatusIconWin for two reasons:
> 
> 1) In WebNotificationTray::EnforceStatusIconVisible() to make a status icon
> visible.
> 2) In a browser test to grab out some window internals.
> 
> Instead of putting the functionality to make a status icon visible into a
> separate class, consider exposing a virtual ForceVisible() API on this class.
> This would be a no-op on most platforms, but StatusIconWin() could basically
do
> what WebNotificationTray::EnforceStatusIconVisible() does now, and
> WebNotificationTray::EnforceStatusIconVisible() could just call
> status_icon_->ForceVisible().
> 
> I think that fits in better with the overall class design. And for the browser
> test you can do whatever (static casts, etc - it's just a test).

+1

[cpu_(ooo_6.6-7.5), 2014-03-21 20:41:42]

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.cc (right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:19: // notifications.
to answer your previous question, the class _declspec(uuid()) pattern is less
magical than the [attributes] tags, might one day another toolchain be able to
digest windows cpp files it most certainly would know about declspecs.

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.cc:102: 
same here replace for HRESULT STDMETHODCALLTYPE and same with STDMETHODIMP_

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.h (right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:66:
STDMETHOD(QueryInterface)(REFIID, PVOID*)OVERRIDE;
you mind changing this to virtual HRESULT STDMETHODCALLTYPE QueryInterface and
the above to 
ULONG STDMETHODCALLTYPE etc.

I understand we have both in the codebase but my suggestion is more readable.

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:88: 
dcheck here that worker_thread_ is null?

[dewittj, 2014-03-27 23:12:34]

On 2014/03/21 20:41:42, cpu wrote:
>
https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
> File chrome/browser/ui/views/message_center/tray_watcher_win.cc (right):
> 
>
https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
> chrome/browser/ui/views/message_center/tray_watcher_win.cc:19: //
notifications.
> to answer your previous question, the class _declspec(uuid()) pattern is less
> magical than the [attributes] tags, might one day another toolchain be able to
> digest windows cpp files it most certainly would know about declspecs.
> 
>
https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
> chrome/browser/ui/views/message_center/tray_watcher_win.cc:102: 
> same here replace for HRESULT STDMETHODCALLTYPE and same with STDMETHODIMP_
> 
>
https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
> File chrome/browser/ui/views/message_center/tray_watcher_win.h (right):
> 
>
https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
> chrome/browser/ui/views/message_center/tray_watcher_win.h:66:
> STDMETHOD(QueryInterface)(REFIID, PVOID*)OVERRIDE;
> you mind changing this to virtual HRESULT STDMETHODCALLTYPE QueryInterface and
> the above to 
> ULONG STDMETHODCALLTYPE etc.
> 
> I understand we have both in the codebase but my suggestion is more readable.
> 
>
https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
> File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
> (right):
> 
>
https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
> chrome/browser/ui/views/message_center/web_notification_tray_win.cc:88: 
> dcheck here that worker_thread_ is null?

Working on moving this to status tray. Stay tuned...

[dewittj, 2014-04-01 17:58:09]

Sorry for the long delay.  I moved the functionality into StatusTrayWin - since
allowing arbitrary statusicons to change their icons at any time, I created a
class to manage multiple requests to the worker thread instead of just starting
a new thread for each request.  Please lmk if you prefer a different idiom.

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/status_i...
File chrome/browser/status_icons/status_icon.h (right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/status_i...
chrome/browser/status_icons/status_icon.h:21: class StatusIconWin;
On 2014/03/21 16:11:22, sky wrote:
> On 2014/03/21 09:15:13, Andrew T Wilson wrote:
> > Yeah, I don't like this, even though I understand why you don't like static
> > casts. It's OK for code running on OS_WIN to assume that all StatusIcons are
> > actually StatusIconWins, and just do a static cast.
> > 
> > Taking a step back, it seems like you only need StatusIconWin for two
reasons:
> > 
> > 1) In WebNotificationTray::EnforceStatusIconVisible() to make a status icon
> > visible.
> > 2) In a browser test to grab out some window internals.
> > 
> > Instead of putting the functionality to make a status icon visible into a
> > separate class, consider exposing a virtual ForceVisible() API on this
class.
> > This would be a no-op on most platforms, but StatusIconWin() could basically
> do
> > what WebNotificationTray::EnforceStatusIconVisible() does now, and
> > WebNotificationTray::EnforceStatusIconVisible() could just call
> > status_icon_->ForceVisible().
> > 
> > I think that fits in better with the overall class design. And for the
browser
> > test you can do whatever (static casts, etc - it's just a test).
> 
> +1

Done.

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/tray_watcher_win.h (right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/tray_watcher_win.h:66:
STDMETHOD(QueryInterface)(REFIID, PVOID*)OVERRIDE;
On 2014/03/21 20:41:43, cpu wrote:
> you mind changing this to virtual HRESULT STDMETHODCALLTYPE QueryInterface and
> the above to 
> ULONG STDMETHODCALLTYPE etc.
> 
> I understand we have both in the codebase but my suggestion is more readable.

Done.

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:88: 
On 2014/03/21 20:41:43, cpu wrote:
> dcheck here that worker_thread_ is null?

I rewrote this part of the code; now the browser thread keeps a count of pending
requests and calls Stop() on the thread when that count reaches zero.  I expect
that it will not grow above 1 any time soon.

[dewittj, 2014-04-04 16:59:12]

ping?

[sky, 2014-04-04 17:12:23]

I'm waiting for you to make cpu happy.

Also, how come chromium-reviews isn't cc'd here?

[dewittj, 2014-04-04 17:13:47]

I had marked it private a long time ago before I was ready for review and never
unmarked it.  Should be corrected now.

[cpu_(ooo_6.6-7.5), 2014-04-04 18:04:15]

lgtm

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:59: // It appears that
IUnknowns are coincidentally compatible with
please add a CHECK() to make sure you are in the correct thread.

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:73:
worker_thread_.Stop();
remind me, does Stop() kill the actual native thread?

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:79: 
remove the spaces 79, 81 unless this is a common style under /status_icons/

[dewittj, 2014-04-04 21:05:51]

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:59: // It appears that
IUnknowns are coincidentally compatible with
On 2014/04/04 18:04:16, cpu wrote:
> please add a CHECK() to make sure you are in the correct thread.

How can you do that in a static method?  I can't find any examples of that.

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:73:
worker_thread_.Stop();
On 2014/04/04 18:04:16, cpu wrote:
> remind me, does Stop() kill the actual native thread?

Stop() calls QuitWhenIdle on the worker thread, and PlatformThread::Join()
(pthread_join on POSIX) on the main thread.

https://codereview.chromium.org/110693004/diff/370001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:79: 
On 2014/04/04 18:04:16, cpu wrote:
> remove the spaces 79, 81 unless this is a common style under /status_icons/

Done.

[sky, 2014-04-04 21:13:25]

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:8: #include
<objbase.h>
nit: Are all of these new includes really necessary?

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_state_changer_win.h
(right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_state_changer_win.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_STATUS_ICONS_STATUS_TRAY_STATE_CHANGER_WIN_H_
I'm assuming cpu reviewed all this code.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_state_changer_win.h:27: PWSTR
pszExeName;  // The file name of the creating executable.
Even though I'm assuming cpu reviewed all this, how come these members don't
follow style guide convension, eg psz_exe_name?

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:30: class
StatusTrayStateChangerProxyImpl : public StatusTrayStateChangerProxy {
Description?

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:39: virtual void
EnqueueChange(UINT icon_id, HWND window) {
OVERRIDE

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:40: if
(pending_requests_ == 0) {
nit: no {}

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:72: if
(--pending_requests_ == 0) {
nit: no {}

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:127: if
(!state_changer_proxy_.get()) {
nit: no {}

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.h (right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.h:15: class
StatusTrayStateChangerProxy {
Description?

[dewittj, 2014-04-04 22:35:57]

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray_win.cc
(right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray_win.cc:8: #include
<objbase.h>
On 2014/04/04 21:13:25, sky wrote:
> nit: Are all of these new includes really necessary?

No, they are leftovers from previous versions of this patch.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_state_changer_win.h
(right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_state_changer_win.h:27: PWSTR
pszExeName;  // The file name of the creating executable.
On 2014/04/04 21:13:25, sky wrote:
> Even though I'm assuming cpu reviewed all this, how come these members don't
> follow style guide convension, eg psz_exe_name?

I guess I thought that in windows data structures we'd use windows-style names. 
I changed them.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.cc (right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:30: class
StatusTrayStateChangerProxyImpl : public StatusTrayStateChangerProxy {
On 2014/04/04 21:13:25, sky wrote:
> Description?

Done.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:39: virtual void
EnqueueChange(UINT icon_id, HWND window) {
On 2014/04/04 21:13:25, sky wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:40: if
(pending_requests_ == 0) {
On 2014/04/04 21:13:25, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:72: if
(--pending_requests_ == 0) {
On 2014/04/04 21:13:25, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.cc:127: if
(!state_changer_proxy_.get()) {
On 2014/04/04 21:13:25, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_win.h (right):

https://codereview.chromium.org/110693004/diff/390001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_win.h:15: class
StatusTrayStateChangerProxy {
On 2014/04/04 21:13:25, sky wrote:
> Description?

Done.

[sky, 2014-04-04 23:12:54]

LGTM

[dewittj, 2014-04-07 16:13:43]

atwilson: ping

[Andrew T Wilson (Slow), 2014-04-08 16:06:19]

StatusTray/Icon stuff LGTM. I couldn't follow the Win/COM code, but I'm assuming
cpu/sky covered that.

https://codereview.chromium.org/110693004/diff/410001/chrome/browser/ui/views...
File chrome/browser/ui/views/message_center/web_notification_tray.cc (right):

https://codereview.chromium.org/110693004/diff/410001/chrome/browser/ui/views...
chrome/browser/ui/views/message_center/web_notification_tray.cc:355: if
(!*did_force_tray_visible_) {
Hmmm, is it safe to do this if Init() was never called? I thought that
BooleanPrefMember.GetValue() DCHECKs in this case?

[dewittj, 2014-04-09 20:21:54]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-04-09 20:22:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/110693004/410001

[dewittj, 2014-04-09 20:22:32]

The CQ bit was unchecked by dewittj@chromium.org

[dewittj, 2014-04-18 17:34:23]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-04-18 17:34:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/110693004/430001

[commit-bot: I haz the power, 2014-04-18 17:34:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 17:34:57]

Failed to apply patch for
chrome/browser/notifications/notification_prefs_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/browser/notifications/notification_prefs_manager.cc
  |diff --git a/chrome/browser/notifications/notification_prefs_manager.cc
b/chrome/browser/notifications/notification_prefs_manager.cc
  |index
a77eb63f2ee0c7c61cb1510da5672dddc3d1c0b8..93db523a7599650963ac842dfe445c5b7a28632a
100644
  |--- a/chrome/browser/notifications/notification_prefs_manager.cc
  |+++ b/chrome/browser/notifications/notification_prefs_manager.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       chrome/browser/notifications/notification_prefs_manager.cc
Index: chrome/browser/notifications/notification_prefs_manager.cc
diff --git a/chrome/browser/notifications/notification_prefs_manager.cc
b/chrome/browser/notifications/notification_prefs_manager.cc
index
a77eb63f2ee0c7c61cb1510da5672dddc3d1c0b8..93db523a7599650963ac842dfe445c5b7a28632a
100644
--- a/chrome/browser/notifications/notification_prefs_manager.cc
+++ b/chrome/browser/notifications/notification_prefs_manager.cc
@@ -1,30 +1,31 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/notifications/notification_prefs_manager.h"
-
-#include "base/prefs/pref_registry_simple.h"
-#include "base/prefs/pref_service.h"
-#include "chrome/common/pref_names.h"
-
-NotificationPrefsManager::NotificationPrefsManager(PrefService* prefs) {
-#if defined(OS_CHROMEOS)
-  static bool have_cleared = false;
-
-  if (!have_cleared) {
-    // Option menu for changing desktop notification position on ChromeOS is
-    // disabled. Force preference to default.
-    prefs->ClearPref(prefs::kDesktopNotificationPosition);
-    have_cleared = true;
-  }
-#endif
-}
-
-// static
-void NotificationPrefsManager::RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterIntegerPref(prefs::kDesktopNotificationPosition,
-                                BalloonCollection::DEFAULT_POSITION);
-  registry->RegisterBooleanPref(prefs::kMessageCenterShowedFirstRunBalloon,
-                                false);
-}
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/notifications/notification_prefs_manager.h"
+
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+#include "chrome/common/pref_names.h"
+
+NotificationPrefsManager::NotificationPrefsManager(PrefService* prefs) {
+#if defined(OS_CHROMEOS)
+  static bool have_cleared = false;
+
+  if (!have_cleared) {
+    // Option menu for changing desktop notification position on ChromeOS is
+    // disabled. Force preference to default.
+    prefs->ClearPref(prefs::kDesktopNotificationPosition);
+    have_cleared = true;
+  }
+#endif
+}
+
+// static
+void NotificationPrefsManager::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterIntegerPref(prefs::kDesktopNotificationPosition,
+                                BalloonCollection::DEFAULT_POSITION);
+  registry->RegisterBooleanPref(prefs::kMessageCenterShowedFirstRunBalloon,
+                                false);
+  registry->RegisterBooleanPref(prefs::kMessageCenterForcedOnTaskbar, false);
+}

[dewittj, 2014-04-18 19:44:54]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-04-18 19:45:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/110693004/450001

[commit-bot: I haz the power, 2014-04-19 00:43:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-19 00:43:57]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[dewittj, 2014-04-23 21:22:24]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-04-23 21:22:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/110693004/490001

[commit-bot: I haz the power, 2014-04-24 01:16:13]

Change committed as 265807

[Lei Zhang, 2014-04-24 03:16:51]

A revert of this CL has been created in
https://codereview.chromium.org/256443004/ by thestig@chromium.org.

The reason for reverting is: From Vista Tests 1:

StatusTrayStateChangerWinTest.TraySizeApiTest (run #1):
[ RUN      ] StatusTrayStateChangerWinTest.TraySizeApiTest
c:\b\build\slave\cr-win-rel\build\src\chrome\browser\ui\views\status_icons\status_tray_state_changer_interactive_uitest_win.cc(149):
error: Expected: (new_width) > (width), actual: 241 vs 241
[  FAILED  ] StatusTrayStateChangerWinTest.TraySizeApiTest (94 ms)
.