Toggle avatar color when window activation changes.

Toggle avatar color when window activation changes.

On Windows 10 in particular, active and inactive windows may have very different frame colors, and the incognito icon for one may be invisible on the other.

We should probably also consider toggling the TOOLBAR_TOP_SEPARATOR_COLOR.  That also makes me wonder if this is the right way to do this; perhaps both these should have _INACTIVE colors and whatever draws the frame in the correct color should draw these as well.

It's also not clear to me whether the nonclient view really needs the widget destruction overload (as noted in a comment).

BUG=none
TEST=Run Chrome on Windows 10 with window titlebars set to a dark color.  Open an incognito window.  Toggle the window between active and inactive and check that the incognito icon toggles between white and grey.

Committed: https://crrev.com/8aecb326e01ed668003bcc72bda50ca64c768d16
Cr-Commit-Position: refs/heads/master@{#381868}

[Peter Kasting, 2016-02-25 11:14:24]

Description was changed from

==========
WIP - dynamic coloration

BUG=
==========

to

==========
WIP - dynamic coloration

BUG=
==========

[Peter Kasting, 2016-02-25 11:14:24]

pkasting@chromium.org changed reviewers:
+ sky@chromium.org

[Peter Kasting, 2016-02-25 11:35:41]

Scott, I could use some advice on parts of this CL.

This CL has parts of two different changes in it, but the questionable parts all
have to do with my attempt to dynamically update the color of the incognito icon
on Win 10 as the window activation state changes, since when doing this the
window frame toggles between white and (whatever color the user has set for
their windows, which may be dark and thus require the opposite incognito icon
color to be visible).

The basic idea here is that BrowserView::GetOTRAvatarIcon() gets the frame color
from the BrowserNonClientFrameView and uses it to determine the icon color. 
BrowserView's widget activation state change observer gets an added bit to force
the nonclient frame view to update the avatar icon when the activation state
changes.

There are several problems here.

* BrowserView::GetOTRAvatarIcon() has to null-check the
BrowserNonClientFrameView.  This is because the various frame views call
UpdateAvatar() in their constructors, which means this code is reached while the
nonclient frame view is still being constructed and thus isn't yet accessible
from the BrowserFrame.  I initially manually plumbed the right nonclient frame
view object directly to this call, but that still doesn't work because calling
GetFrameColor() results in calling GetWidget()->IsActive(), and the nonclient
frame view isn't yet in a widget hierarchy.  Rather than try to also plumb/fake
that, it seems like the real issue here is that the nonclient frame view
implementations should call UpdateAvatar() after they're placed in a widget
hierarchy instead of during construction.  I think.  (I'm worried that the
current mechanism could set the wrong icon color until the widget's activation
state is first changed.)  I suppose the way to do that would be to add
ViewHierarchyChanged() overrides and check for GetWidget() being non-null there?
 Not quite sure the right idiom for this.

* BrowserView::OnWidgetActivationChanged() doesn't call UpdateAvatar() directly;
instead it posts a task to do so.  This is because apparently at the time this
function is called during widget deactivation, the code in
HWNDMessageHandler::IsActive() that calls GetActiveWindow() still gets back the
current HWND, so the code that computes the icon color by checking the
activation state always sees the widget as active.  I'm not sure how the message
processing order works here.  Unless somehow this activation state checking is
fixable, it seems like the only options are PostTask() as I've done, or plumbing
the |active| parameter from OnWidgetActivationChanged() through into enough
places that we can use that instead of GetWidget()->IsActive().

* The PostTask() call uses Unretained() on the BrowserNonClientFrameView, which
is certainly wrong.  I don't know what the right way to do this is, though... is
there a weak factory on some relevant object?  Of course this is moot if I kill
the PostTask() call.

How would you do all this?

[sky, 2016-02-25 18:04:09]

On 2016/02/25 11:35:41, Peter Kasting wrote:
> Scott, I could use some advice on parts of this CL.
> 
> This CL has parts of two different changes in it, but the questionable parts
all
> have to do with my attempt to dynamically update the color of the incognito
icon
> on Win 10 as the window activation state changes, since when doing this the
> window frame toggles between white and (whatever color the user has set for
> their windows, which may be dark and thus require the opposite incognito icon
> color to be visible).
> 
> The basic idea here is that BrowserView::GetOTRAvatarIcon() gets the frame
color
> from the BrowserNonClientFrameView and uses it to determine the icon color. 
> BrowserView's widget activation state change observer gets an added bit to
force
> the nonclient frame view to update the avatar icon when the activation state
> changes.
> 
> There are several problems here.
> 
> * BrowserView::GetOTRAvatarIcon() has to null-check the
> BrowserNonClientFrameView.  This is because the various frame views call
> UpdateAvatar() in their constructors, which means this code is reached while
the
> nonclient frame view is still being constructed and thus isn't yet accessible
> from the BrowserFrame.  I initially manually plumbed the right nonclient frame
> view object directly to this call, but that still doesn't work because calling
> GetFrameColor() results in calling GetWidget()->IsActive(), and the nonclient
> frame view isn't yet in a widget hierarchy.  Rather than try to also
plumb/fake
> that, it seems like the real issue here is that the nonclient frame view
> implementations should call UpdateAvatar() after they're placed in a widget
> hierarchy instead of during construction.  I think.  (I'm worried that the
> current mechanism could set the wrong icon color until the widget's activation
> state is first changed.)  I suppose the way to do that would be to add
> ViewHierarchyChanged() overrides and check for GetWidget() being non-null
there?
>  Not quite sure the right idiom for this.

BrowserView itself doesn't make use of GetOTRAvatarIcon. It's implemented there
so the implementation can be shared among the frame types. Would it make more
sense to remove from BrowserView and implement as appropriate in the
BrowserNonClientFrameViews? Seems like that would get rid of the timing issues
you're referring to. Glass/Opaque would want to share an implementation, but I
would imagine there is a way to do share between the two.

As you say, waiting until ViewHierarchyChanged and then updating the avatar icon
seems like it will address timing problems as well.

> * BrowserView::OnWidgetActivationChanged() doesn't call UpdateAvatar()
directly;
> instead it posts a task to do so.  This is because apparently at the time this
> function is called during widget deactivation, the code in
> HWNDMessageHandler::IsActive() that calls GetActiveWindow() still gets back
the
> current HWND, so the code that computes the icon color by checking the
> activation state always sees the widget as active.  I'm not sure how the
message
> processing order works here.  Unless somehow this activation state checking is
> fixable, it seems like the only options are PostTask() as I've done, or
plumbing
> the |active| parameter from OnWidgetActivationChanged() through into enough
> places that we can use that instead of GetWidget()->IsActive().

I'm not familiar with the particulars of WM_ACTIVATE, but I wonder if the
behavior of ::GetActiveWindow() depends upon whether the window gaining
activation is in the same process or not?

One (icky) thing you could is add a variable to HWNDMessageHandler to force
IsActive() to return false when processing a WM_ACTIVATE for inactive, eg:

void HWNDMessageHandler::PostProcessActivateMessage(
    int activation_state,
    bool minimized,
    HWND window_gaining_or_losing_activation) {
  if (activation_state == WA_INACTIVE)
    force_inactive_ = true;
  ...
  ...
  // Handle case of this being destroyed and early out if before changing this.
  force_inactive_ = false;
}

bool HWNDMessageHandler::IsActive() const {
  return force_inactive_ ? false : GetActiveWindow() == hwnd();
}



> * The PostTask() call uses Unretained() on the BrowserNonClientFrameView,
which
> is certainly wrong.  I don't know what the right way to do this is, though...
is
> there a weak factory on some relevant object?  Of course this is moot if I
kill
> the PostTask() call.
> 
> How would you do all this?

[Peter Kasting, 2016-03-17 09:05:59]

Description was changed from

==========
WIP - dynamic coloration

BUG=
==========

to

==========
Toggle avatar color when window activation changes.

On Windows 10 in particular, active and inactive windows may have very different
frame colors, and the incognito icon for one may be invisible on the other.

We should probably also consider toggling the TOOLBAR_TOP_SEPARATOR_COLOR.  That
also makes me wonder if this is the right way to do this; perhaps both these
should have _INACTIVE colors and whatever draws the frame in the correct color
should draw these as well.

It's also not clear to me whether the nonclient view really needs the widget
destruction overload (as noted in a comment).

BUG=none
TEST=Run Chrome on Windows 10 with window titlebars set to a dark color.  Open
an incognito window.  Toggle the window between active and inactive and check
that the incognito icon toggles between white and grey.
==========

[Peter Kasting, 2016-03-17 09:06:21]

Scott, this is now ready for a closer look.  See the CL description for some
questions.

[sky, 2016-03-17 21:00:31]

https://codereview.chromium.org/1733293002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1733293002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:38:
observer_(this) {
Feels wrong to have an observer on Widget when there is such a close
relationship between this class and Widget already. I would rather you add a
function to NonClientFrameView that OnNativeWidgetActivationChanged calls.

[Peter Kasting, 2016-03-17 23:14:10]

PTAL

https://codereview.chromium.org/1733293002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1733293002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:38:
observer_(this) {
On 2016/03/17 21:00:31, sky wrote:
> Feels wrong to have an observer on Widget when there is such a close
> relationship between this class and Widget already. I would rather you add a
> function to NonClientFrameView that OnNativeWidgetActivationChanged calls.

Done.

[sky, 2016-03-17 23:58:12]

LGTM

[Peter Kasting, 2016-03-18 00:22:11]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-03-18 00:22:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1733293002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1733293002/180001

[commit-bot: I haz the power, 2016-03-18 01:43:09]

Description was changed from

==========
Toggle avatar color when window activation changes.

On Windows 10 in particular, active and inactive windows may have very different
frame colors, and the incognito icon for one may be invisible on the other.

We should probably also consider toggling the TOOLBAR_TOP_SEPARATOR_COLOR.  That
also makes me wonder if this is the right way to do this; perhaps both these
should have _INACTIVE colors and whatever draws the frame in the correct color
should draw these as well.

It's also not clear to me whether the nonclient view really needs the widget
destruction overload (as noted in a comment).

BUG=none
TEST=Run Chrome on Windows 10 with window titlebars set to a dark color.  Open
an incognito window.  Toggle the window between active and inactive and check
that the incognito icon toggles between white and grey.
==========

to

==========
Toggle avatar color when window activation changes.

On Windows 10 in particular, active and inactive windows may have very different
frame colors, and the incognito icon for one may be invisible on the other.

We should probably also consider toggling the TOOLBAR_TOP_SEPARATOR_COLOR.  That
also makes me wonder if this is the right way to do this; perhaps both these
should have _INACTIVE colors and whatever draws the frame in the correct color
should draw these as well.

It's also not clear to me whether the nonclient view really needs the widget
destruction overload (as noted in a comment).

BUG=none
TEST=Run Chrome on Windows 10 with window titlebars set to a dark color.  Open
an incognito window.  Toggle the window between active and inactive and check
that the incognito icon toggles between white and grey.
==========

[commit-bot: I haz the power, 2016-03-18 01:43:11]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-18 01:44:27]

Description was changed from

==========
Toggle avatar color when window activation changes.

On Windows 10 in particular, active and inactive windows may have very different
frame colors, and the incognito icon for one may be invisible on the other.

We should probably also consider toggling the TOOLBAR_TOP_SEPARATOR_COLOR.  That
also makes me wonder if this is the right way to do this; perhaps both these
should have _INACTIVE colors and whatever draws the frame in the correct color
should draw these as well.

It's also not clear to me whether the nonclient view really needs the widget
destruction overload (as noted in a comment).

BUG=none
TEST=Run Chrome on Windows 10 with window titlebars set to a dark color.  Open
an incognito window.  Toggle the window between active and inactive and check
that the incognito icon toggles between white and grey.
==========

to

==========
Toggle avatar color when window activation changes.

On Windows 10 in particular, active and inactive windows may have very different
frame colors, and the incognito icon for one may be invisible on the other.

We should probably also consider toggling the TOOLBAR_TOP_SEPARATOR_COLOR.  That
also makes me wonder if this is the right way to do this; perhaps both these
should have _INACTIVE colors and whatever draws the frame in the correct color
should draw these as well.

It's also not clear to me whether the nonclient view really needs the widget
destruction overload (as noted in a comment).

BUG=none
TEST=Run Chrome on Windows 10 with window titlebars set to a dark color.  Open
an incognito window.  Toggle the window between active and inactive and check
that the incognito icon toggles between white and grey.

Committed: https://crrev.com/8aecb326e01ed668003bcc72bda50ca64c768d16
Cr-Commit-Position: refs/heads/master@{#381868}
==========

[commit-bot: I haz the power, 2016-03-18 01:44:29]

Patchset 10 (id:??) landed as
https://crrev.com/8aecb326e01ed668003bcc72bda50ca64c768d16
Cr-Commit-Position: refs/heads/master@{#381868}