Fix text color of WrenchMenu buttons on Linux Aura.

Fix text color of WrenchMenu buttons on Linux Aura.

The color needs to come from the menu's own native theme.

BUG=347832
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276862

[Evan Stade, 2014-04-21 22:34:19]

this broke chromeos, let me figure out the fix before you review

[Evan Stade, 2014-04-22 00:25:40]

ok, please review

https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc#newcode212
ui/views/view.cc:212: ui::NativeTheme* old_theme = NULL;
this change makes it so that OnNativeThemeChanged is called at least once for
each view

[sky, 2014-04-22 15:30:21]

https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc#newcode212
ui/views/view.cc:212: ui::NativeTheme* old_theme = NULL;
On 2014/04/22 00:25:41, Evan Stade wrote:
> this change makes it so that OnNativeThemeChanged is called at least once for
> each view

Why do you need to do these changes?

[Evan Stade, 2014-04-22 17:12:32]

https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc#newcode212
ui/views/view.cc:212: ui::NativeTheme* old_theme = NULL;
On 2014/04/22 15:30:21, sky wrote:
> On 2014/04/22 00:25:41, Evan Stade wrote:
> > this change makes it so that OnNativeThemeChanged is called at least once
for
> > each view
> 
> Why do you need to do these changes?

The changes on L239-241 just seem more correct to me.

For L212-214, on Chromeos, OnNativeThemeChanged never gets called for the wrench
menu because old_theme == new_theme even when it's first added to the hierarchy.

[sky, 2014-04-22 19:30:14]

On Tue, Apr 22, 2014 at 10:12 AM,  <estade@chromium.org> wrote:
>
> https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc
> File ui/views/view.cc (right):
>
>
https://codereview.chromium.org/245863002/diff/80001/ui/views/view.cc#newcode212
> ui/views/view.cc:212: ui::NativeTheme* old_theme = NULL;
> On 2014/04/22 15:30:21, sky wrote:
>>
>> On 2014/04/22 00:25:41, Evan Stade wrote:
>> > this change makes it so that OnNativeThemeChanged is called at least
>
> once for
>>
>> > each view
>
>
>> Why do you need to do these changes?
>
>
> The changes on L239-241 just seem more correct to me.
>
> For L212-214, on Chromeos, OnNativeThemeChanged never gets called for
> the wrench menu because old_theme == new_theme even when it's first
> added to the hierarchy.

That's expected. The theme hasn't changed. OnNativeThemeChanged should
only be invoked if the theme actually changes.

  -Scott

> https://codereview.chromium.org/245863002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-04-22 19:42:36]

Just to clarify. The reason here is that if in the constructor for your view you
ask for the nativetheme then you should only get OnNativeThemeChanged later on
if the NativeTheme some how changes. Adding a view to a hierarchy does not
necessarily change the theme.

[Evan Stade, 2014-04-22 20:00:00]

On 2014/04/22 19:42:36, sky wrote:
> Just to clarify. The reason here is that if in the constructor for your view
you
> ask for the nativetheme then you should only get OnNativeThemeChanged later on
> if the NativeTheme some how changes. Adding a view to a hierarchy does not
> necessarily change the theme.

So there are two patterns that are possible:

MyView::MyView() {
  // do stuff
  OnNativeThemeChanged(GetNativeTheme());
}

MyView::OnNativeThemeChanged() {
  // do stuff
}

--- or ---

MyView::MyView() {
  // do stuff
}

MyView::OnNativeThemeChanged() {
  // do stuff
}

The advantage of the second pattern (which is enabled by these changes to View)
is that you don't have to manually call OnNativeThemeChanged with a theme that
may not be correct, only to override these changes as soon as the view is
parented. Even in the case where the theme doesn't change, the second pattern
doesn't do any additional work.

[sky, 2014-04-22 22:03:41]

That makes sense. How about some test coverage then, and presumably
you can now update anyone overriding this.

  -Scott

On Tue, Apr 22, 2014 at 1:00 PM,  <estade@chromium.org> wrote:
> On 2014/04/22 19:42:36, sky wrote:
>>
>> Just to clarify. The reason here is that if in the constructor for your
>> view
>
> you
>>
>> ask for the nativetheme then you should only get OnNativeThemeChanged
>> later on
>> if the NativeTheme some how changes. Adding a view to a hierarchy does not
>> necessarily change the theme.
>
>
> So there are two patterns that are possible:
>
> MyView::MyView() {
>   // do stuff
>   OnNativeThemeChanged(GetNativeTheme());
> }
>
> MyView::OnNativeThemeChanged() {
>   // do stuff
> }
>
> --- or ---
>
> MyView::MyView() {
>   // do stuff
> }
>
> MyView::OnNativeThemeChanged() {
>   // do stuff
> }
>
> The advantage of the second pattern (which is enabled by these changes to
> View)
> is that you don't have to manually call OnNativeThemeChanged with a theme
> that
> may not be correct, only to override these changes as soon as the view is
> parented. Even in the case where the theme doesn't change, the second
> pattern
> doesn't do any additional work.
>
> https://codereview.chromium.org/245863002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2014-04-22 23:25:44]

found a problem when I was updating AutofillDialogViews. Some of the views in
there are getting multiple OnNativeThemeChanged notifications. The reason is
because ClientView::ViewHierarchyChanged() and
NonClientView::ViewHierarchyChanged() both add children. So it creates a
situation where nested calls to AddChildViewAt can trigger
PropagateNativeThemeChanged to children, then later their parents, meaning the
children get OnNativeThemeChanged more than once.

There are two ways to fix this, one is what I've uploaded. The other is to move
the PropagateNativeThemeChange() call above the ViewHierarchyChangedImpl() call.
The latter is more elegant and has fewer changed LOC, but is also more subtle
and seems less robust. Let me know what you think about the two solutions.

[Evan Stade, 2014-04-22 23:29:29]

patchset 7 is option (a), patchset 8 is option (b).

[sky, 2014-04-22 23:30:42]

I don't want to add a member for this.
Why is it necessarily a problem if invoked multiple times?

[Evan Stade, 2014-04-22 23:31:42]

On 2014/04/22 23:30:42, sky wrote:
> I don't want to add a member for this.
> Why is it necessarily a problem if invoked multiple times?

it's just extra work

[sky, 2014-04-22 23:50:09]

It doesn't seem like it'll be invoked that many times to care.

  -Scott

On Tue, Apr 22, 2014 at 4:31 PM,  <estade@chromium.org> wrote:
> On 2014/04/22 23:30:42, sky wrote:
>>
>> I don't want to add a member for this.
>> Why is it necessarily a problem if invoked multiple times?
>
>
> it's just extra work
>
> https://codereview.chromium.org/245863002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2014-04-23 22:42:05]

I suppose you don't mind the subtle solution then, because it won't be a big
deal if it breaks. Or do you just want things to remain unchanged?

On 2014/04/22 23:50:09, sky wrote:
> It doesn't seem like it'll be invoked that many times to care.
> 
>   -Scott
> 
> On Tue, Apr 22, 2014 at 4:31 PM,  <mailto:estade@chromium.org> wrote:
> > On 2014/04/22 23:30:42, sky wrote:
> >>
> >> I don't want to add a member for this.
> >> Why is it necessarily a problem if invoked multiple times?
> >
> >
> > it's just extra work
> >
> > https://codereview.chromium.org/245863002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-04-24 03:07:47]

How about subtle solution with test coverage? Assuming it doesn't
involve a new member.

  -Scott

On Wed, Apr 23, 2014 at 3:42 PM,  <estade@chromium.org> wrote:
> I suppose you don't mind the subtle solution then, because it won't be a big
> deal if it breaks. Or do you just want things to remain unchanged?
>
>
> On 2014/04/22 23:50:09, sky wrote:
>>
>> It doesn't seem like it'll be invoked that many times to care.
>
>
>>    -Scott
>
>
>> On Tue, Apr 22, 2014 at 4:31 PM,  <mailto:estade@chromium.org> wrote:
>> > On 2014/04/22 23:30:42, sky wrote:
>> >>
>> >> I don't want to add a member for this.
>> >> Why is it necessarily a problem if invoked multiple times?
>> >
>> >
>> > it's just extra work
>> >
>> > https://codereview.chromium.org/245863002/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/245863002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2014-06-12 01:09:11]

after shaving other yaks and getting distracted with other tasks, I've come back
to this patch. PTAL.

[sky, 2014-06-12 15:41:22]

Are all the yaks in LA? LGTM

[Evan Stade, 2014-06-12 19:39:50]

On 2014/06/12 15:41:22, sky wrote:
> Are all the yaks in LA? LGTM

They're in Tibet, which is why it took me so long.

thx for review

[Evan Stade, 2014-06-12 19:39:59]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-12 19:42:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/245863002/200001

[commit-bot: I haz the power, 2014-06-13 00:19:29]

Change committed as 276862