Linux/Windows: Remove NonInteractiveContainer from the profile chooser.

Linux/Windows: Remove NonInteractiveContainer from the profile chooser.

This CL replaces the NonInteractiveContainer class by a View class. The only
goal from NonInteractiveContainer was to return false from
CanProcessEventsWithinSubtree().

This view only contains a Label which has no interaction with the mouse. The
only difference is enabling tool tips.

BUG=674462
Review-Url: https://codereview.chromium.org/2633183002
Cr-Commit-Position: refs/heads/master@{#448228}
Committed: https://chromium.googlesource.com/chromium/src/+/bce01de43097d69f127f2637d9c3398ff780462a

[jlebel, 2017-01-16 18:37:32]

jlebel@chromium.org changed reviewers:
+ msarda@chromium.org

[jlebel, 2017-01-16 18:37:33]

Hello Miha,

Can you review this patch to add back the tooltips in the profile chooser in
Linux?

Thanks,

[msarda, 2017-01-17 13:16:32]

1. The CL description in Chromium have the following structure:
CL Title (less than 72 characters)
CL Body

In your case, this should be something like:
--------------------------------------------------------------------
Title:
Remove NonInteractiveContainer from the profile chooser.

Body:
Remove NonInteractiveContainer from the profile chooser.

This CL removes the NonInteractiveContainer class so the profile name
and the email address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).
--------------------------------------------------------------------

2. Code looks good to me, but it would be good to get a second review from
rpgerta@ as I do not really know why the NonInteractiveContainer was useful in
the first place. 

3. I think you have a problem with autocomplete for my name (s/Miha/Mihai)

[jlebel, 2017-01-17 13:20:39]

Description was changed from

==========
Linux: Removing NonInteractiveContainer class so the profile name and the email
address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).

BUG=674462
==========

to

==========
Remove NonInteractiveContainer from the profile chooser.

This CL removes the NonInteractiveContainer class so the profile name
and the email address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).

BUG=674462
==========

[jlebel, 2017-01-17 13:25:59]

jlebel@chromium.org changed reviewers:
+ rogerta@chromium.org

[jlebel, 2017-01-17 13:25:59]

Hello Roger,

Can you review this patch? The goal is to have tool tip on Linux/Windows in the
profile chooser menu.

Thanks,

[msarda, 2017-01-17 14:04:20]

LGTM.

[jlebel, 2017-01-20 12:27:54]

Description was changed from

==========
Remove NonInteractiveContainer from the profile chooser.

This CL removes the NonInteractiveContainer class so the profile name
and the email address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).

BUG=674462
==========

to

==========
Linux: Remove NonInteractiveContainer from the profile chooser.

This CL removes the NonInteractiveContainer class so the profile name
and the email address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).

BUG=674462
==========

[jlebel, 2017-01-20 12:29:26]

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

[jlebel, 2017-01-20 12:29:26]

Hello Peter,

Can you review this patch about adding tool tips in the profile switching menu
on linux?

Thanks,

[Roger Tawa OOO till Jul 10th, 2017-01-23 15:48:02]

Hi Jerome, I don't recall why the old code disabled all event processing in the
subtree.   Because you are re-enabling all event processing again, what other
side effects are there besides tooltips working again?

[jlebel, 2017-01-23 18:22:34]

On 2017/01/23 15:48:02, Roger Tawa wrote:
> Hi Jerome, I don't recall why the old code disabled all event processing in
the
> subtree.   Because you are re-enabling all event processing again, what other
> side effects are there besides tooltips working again?

I'm not sure what kind of events I should try/test/look for.  I've tried the
keyboard, the right click and the left click+drag. I don't see any side effects.

[Peter Kasting, 2017-01-24 00:48:08]

The CL title says "Linux:", but this change affects all platforms.

There's no explanation here or on the bug about what the
NonInteractiveContainer's primary purpose was and what other side effects there
could be from removing this.

On the associated bug, a finger is pointed at code elsewhere in this file that
disables tooltips; it's not clear to me whether simply removing that would also
have the side effect of fixing the issue here.

In short, I'd like to have a much better understanding of what this code
originally did and why removing it is correct (in general, and the correct fix
for this specific problem).

[jlebel, 2017-01-26 10:29:13]

Description was changed from

==========
Linux: Remove NonInteractiveContainer from the profile chooser.

This CL removes the NonInteractiveContainer class so the profile name
and the email address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).

BUG=674462
==========

to

==========
Linux/Windows: Remove NonInteractiveContainer from the profile chooser.

This CL removes the NonInteractiveContainer class so the profile name
and the email address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).

BUG=674462
==========

[jlebel, 2017-01-26 10:38:30]

On 2017/01/24 00:48:08, Peter Kasting wrote:
> The CL title says "Linux:", but this change affects all platforms.
> 
> There's no explanation here or on the bug about what the
> NonInteractiveContainer's primary purpose was and what other side effects
there
> could be from removing this.
> 
> On the associated bug, a finger is pointed at code elsewhere in this file that
> disables tooltips; it's not clear to me whether simply removing that would
also
> have the side effect of fixing the issue here.
> 
> In short, I'd like to have a much better understanding of what this code
> originally did and why removing it is correct (in general, and the correct fix
> for this specific problem).

NonInteractiveContainer class is only used to return false to the method
CanProcessEventsWithinSubtree().

I was able to find only one use of this method using codesearch. This is used
in:
https://cs.chromium.org/chromium/src/ui/views/view.cc?q=CanProcessEventsWithi...

So as far as I know it exists only avoid tooltips.

[Peter Kasting, 2017-01-26 18:12:59]

On 2017/01/26 10:38:30, jlebel wrote:
> On 2017/01/24 00:48:08, Peter Kasting wrote:
> > The CL title says "Linux:", but this change affects all platforms.
> > 
> > There's no explanation here or on the bug about what the
> > NonInteractiveContainer's primary purpose was and what other side effects
> there
> > could be from removing this.
> > 
> > On the associated bug, a finger is pointed at code elsewhere in this file
that
> > disables tooltips; it's not clear to me whether simply removing that would
> also
> > have the side effect of fixing the issue here.
> > 
> > In short, I'd like to have a much better understanding of what this code
> > originally did and why removing it is correct (in general, and the correct
fix
> > for this specific problem).
> 
> NonInteractiveContainer class is only used to return false to the method
> CanProcessEventsWithinSubtree().
> 
> I was able to find only one use of this method using codesearch. This is used
> in:
>
https://cs.chromium.org/chromium/src/ui/views/view.cc?q=CanProcessEventsWithi...
> 
> So as far as I know it exists only avoid tooltips.

It's also used in TargetForRect():
https://cs.chromium.org/chromium/src/ui/views/view_targeter_delegate.cc?q=Can...

That, in turn, affects event targeting in general.  I would be surprised if the
only practical effect of this change was on tooltips.

[jlebel, 2017-01-26 18:37:51]

Description was changed from

==========
Linux/Windows: Remove NonInteractiveContainer from the profile chooser.

This CL removes the NonInteractiveContainer class so the profile name
and the email address can display tooltips when the strings are truncated
(NonInteractiveContainer prevents events to be processed in the subtree).

BUG=674462
==========

to

==========
Linux/Windows: Remove NonInteractiveContainer from the profile chooser.

This CL replaces the NonInteractiveContainer class by a View class. The only
goal from NonInteractiveContainer was to return false from
CanProcessEventsWithinSubtree().

This view only contains a Label which has no interaction with the mouse. The
only difference is enabling tool tips.

BUG=674462
==========

[jlebel, 2017-01-26 18:38:41]

On 2017/01/26 18:12:59, Peter Kasting wrote:
> On 2017/01/26 10:38:30, jlebel wrote:
> > On 2017/01/24 00:48:08, Peter Kasting wrote:
> > > The CL title says "Linux:", but this change affects all platforms.
> > > 
> > > There's no explanation here or on the bug about what the
> > > NonInteractiveContainer's primary purpose was and what other side effects
> > there
> > > could be from removing this.
> > > 
> > > On the associated bug, a finger is pointed at code elsewhere in this file
> that
> > > disables tooltips; it's not clear to me whether simply removing that would
> > also
> > > have the side effect of fixing the issue here.
> > > 
> > > In short, I'd like to have a much better understanding of what this code
> > > originally did and why removing it is correct (in general, and the correct
> fix
> > > for this specific problem).
> > 
> > NonInteractiveContainer class is only used to return false to the method
> > CanProcessEventsWithinSubtree().
> > 
> > I was able to find only one use of this method using codesearch. This is
used
> > in:
> >
>
https://cs.chromium.org/chromium/src/ui/views/view.cc?q=CanProcessEventsWithi...
> > 
> > So as far as I know it exists only avoid tooltips.
> 
> It's also used in TargetForRect():
>
https://cs.chromium.org/chromium/src/ui/views/view_targeter_delegate.cc?q=Can...
> 
> That, in turn, affects event targeting in general.  I would be surprised if
the
> only practical effect of this change was on tooltips.

This view only contains a Label. I was not able to interact with this Label
using the mouse, the right click
 and the left click.

[Peter Kasting, 2017-01-26 18:43:40]

On 2017/01/26 18:38:41, jlebel wrote:
> On 2017/01/26 18:12:59, Peter Kasting wrote:
> > On 2017/01/26 10:38:30, jlebel wrote:
> > > On 2017/01/24 00:48:08, Peter Kasting wrote:
> > > > The CL title says "Linux:", but this change affects all platforms.
> > > > 
> > > > There's no explanation here or on the bug about what the
> > > > NonInteractiveContainer's primary purpose was and what other side
effects
> > > there
> > > > could be from removing this.
> > > > 
> > > > On the associated bug, a finger is pointed at code elsewhere in this
file
> > that
> > > > disables tooltips; it's not clear to me whether simply removing that
would
> > > also
> > > > have the side effect of fixing the issue here.
> > > > 
> > > > In short, I'd like to have a much better understanding of what this code
> > > > originally did and why removing it is correct (in general, and the
correct
> > fix
> > > > for this specific problem).
> > > 
> > > NonInteractiveContainer class is only used to return false to the method
> > > CanProcessEventsWithinSubtree().
> > > 
> > > I was able to find only one use of this method using codesearch. This is
> used
> > > in:
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/view.cc?q=CanProcessEventsWithi...
> > > 
> > > So as far as I know it exists only avoid tooltips.
> > 
> > It's also used in TargetForRect():
> >
>
https://cs.chromium.org/chromium/src/ui/views/view_targeter_delegate.cc?q=Can...
> > 
> > That, in turn, affects event targeting in general.  I would be surprised if
> the
> > only practical effect of this change was on tooltips.
> 
> This view only contains a Label. I was not able to interact with this Label
> using the mouse, the right click
>  and the left click.

Well, two labels (per the code).  But since Label is not selectable by default,
and I assume not focusable when not selected (did you try tabbing through this
stuff?), maybe it can't handle any events.

One question I asked that you did not answer was around the code elsewhere in
the file that disables tooltips, and how that interacts with this code.

I'm beginning to believe that removing this is correct, but I'm still feeling
cautious.  (I wish when this was originally added that the author/reviewer had
justified it better in comments.)

[jlebel, 2017-01-27 14:32:12]

On 2017/01/26 18:43:40, Peter Kasting wrote:
> On 2017/01/26 18:38:41, jlebel wrote:
> > On 2017/01/26 18:12:59, Peter Kasting wrote:
> > > On 2017/01/26 10:38:30, jlebel wrote:
> > > > On 2017/01/24 00:48:08, Peter Kasting wrote:
> > > > > The CL title says "Linux:", but this change affects all platforms.
> > > > > 
> > > > > There's no explanation here or on the bug about what the
> > > > > NonInteractiveContainer's primary purpose was and what other side
> effects
> > > > there
> > > > > could be from removing this.
> > > > > 
> > > > > On the associated bug, a finger is pointed at code elsewhere in this
> file
> > > that
> > > > > disables tooltips; it's not clear to me whether simply removing that
> would
> > > > also
> > > > > have the side effect of fixing the issue here.
> > > > > 
> > > > > In short, I'd like to have a much better understanding of what this
code
> > > > > originally did and why removing it is correct (in general, and the
> correct
> > > fix
> > > > > for this specific problem).
> > > > 
> > > > NonInteractiveContainer class is only used to return false to the method
> > > > CanProcessEventsWithinSubtree().
> > > > 
> > > > I was able to find only one use of this method using codesearch. This is
> > used
> > > > in:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/view.cc?q=CanProcessEventsWithi...
> > > > 
> > > > So as far as I know it exists only avoid tooltips.
> > > 
> > > It's also used in TargetForRect():
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/view_targeter_delegate.cc?q=Can...
> > > 
> > > That, in turn, affects event targeting in general.  I would be surprised
if
> > the
> > > only practical effect of this change was on tooltips.
> > 
> > This view only contains a Label. I was not able to interact with this Label
> > using the mouse, the right click
> >  and the left click.
> 
> Well, two labels (per the code).  But since Label is not selectable by
default,
> and I assume not focusable when not selected (did you try tabbing through this
> stuff?), maybe it can't handle any events.
> 
> One question I asked that you did not answer was around the code elsewhere in
> the file that disables tooltips, and how that interacts with this code.
> 
> I'm beginning to believe that removing this is correct, but I'm still feeling
> cautious.  (I wish when this was originally added that the author/reviewer had
> justified it better in comments.)


I've also tried the scroll wheel, keyboard events (like tab, arrows, escape,
enter and
others). It always behave as expected.

I'm not sure to understand your second question. I don't see any interaction
between
those labels with the rest of the code.

[Peter Kasting, 2017-01-27 20:04:34]

On 2017/01/27 14:32:12, jlebel wrote:
> On 2017/01/26 18:43:40, Peter Kasting wrote:
> > On 2017/01/26 18:38:41, jlebel wrote:
> > > On 2017/01/26 18:12:59, Peter Kasting wrote:
> > > > On 2017/01/26 10:38:30, jlebel wrote:
> > > > > On 2017/01/24 00:48:08, Peter Kasting wrote:
> > > > > > The CL title says "Linux:", but this change affects all platforms.
> > > > > > 
> > > > > > There's no explanation here or on the bug about what the
> > > > > > NonInteractiveContainer's primary purpose was and what other side
> > effects
> > > > > there
> > > > > > could be from removing this.
> > > > > > 
> > > > > > On the associated bug, a finger is pointed at code elsewhere in this
> > file
> > > > that
> > > > > > disables tooltips; it's not clear to me whether simply removing that
> > would
> > > > > also
> > > > > > have the side effect of fixing the issue here.
> > > > > > 
> > > > > > In short, I'd like to have a much better understanding of what this
> code
> > > > > > originally did and why removing it is correct (in general, and the
> > correct
> > > > fix
> > > > > > for this specific problem).
> > > > > 
> > > > > NonInteractiveContainer class is only used to return false to the
method
> > > > > CanProcessEventsWithinSubtree().
> > > > > 
> > > > > I was able to find only one use of this method using codesearch. This
is
> > > used
> > > > > in:
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/view.cc?q=CanProcessEventsWithi...
> > > > > 
> > > > > So as far as I know it exists only avoid tooltips.
> > > > 
> > > > It's also used in TargetForRect():
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/views/view_targeter_delegate.cc?q=Can...
> > > > 
> > > > That, in turn, affects event targeting in general.  I would be surprised
> if
> > > the
> > > > only practical effect of this change was on tooltips.
> > > 
> > > This view only contains a Label. I was not able to interact with this
Label
> > > using the mouse, the right click
> > >  and the left click.
> > 
> > Well, two labels (per the code).  But since Label is not selectable by
> default,
> > and I assume not focusable when not selected (did you try tabbing through
this
> > stuff?), maybe it can't handle any events.
> > 
> > One question I asked that you did not answer was around the code elsewhere
in
> > the file that disables tooltips, and how that interacts with this code.
> > 
> > I'm beginning to believe that removing this is correct, but I'm still
feeling
> > cautious.  (I wish when this was originally added that the author/reviewer
had
> > justified it better in comments.)
> 
> 
> I've also tried the scroll wheel, keyboard events (like tab, arrows, escape,
> enter and
> others). It always behave as expected.
> 
> I'm not sure to understand your second question. I don't see any interaction
> between
> those labels with the rest of the code.

BackgroundColorHoverButton() disables tooltips on its label. 
|profile_name_container| is added as a child of a BackgroundColorHoverButton. 
Reading it more, I don't think that can matter -- this child will not be what
label() returns, so it won't be affected -- but this connection is what I was
asking about.

BTW, I think it's wrong that |current_profile_card_| is a
BackgroundColorHoverButton to begin with.  It makes no sense to me to use a
custom subclass of a LabelButton in a place where you're going to change its
layout manager and add a bunch of children.  This should probably be a
CustomButton, though I'm not 100% sure.

LGTM on this change, but this code needs more help.

[jlebel, 2017-02-06 10:29:56]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2017-02-06 10:30:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 11:33:21]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486376996212850, "parent_rev":
"8fabb1af2315f0b6fc685118b7c25b726bf3f94e", "commit_rev":
"bce01de43097d69f127f2637d9c3398ff780462a"}

[commit-bot: I haz the power, 2017-02-06 11:33:55]

Description was changed from

==========
Linux/Windows: Remove NonInteractiveContainer from the profile chooser.

This CL replaces the NonInteractiveContainer class by a View class. The only
goal from NonInteractiveContainer was to return false from
CanProcessEventsWithinSubtree().

This view only contains a Label which has no interaction with the mouse. The
only difference is enabling tool tips.

BUG=674462
==========

to

==========
Linux/Windows: Remove NonInteractiveContainer from the profile chooser.

This CL replaces the NonInteractiveContainer class by a View class. The only
goal from NonInteractiveContainer was to return false from
CanProcessEventsWithinSubtree().

This view only contains a Label which has no interaction with the mouse. The
only difference is enabling tool tips.

BUG=674462

Review-Url: https://codereview.chromium.org/2633183002
Cr-Commit-Position: refs/heads/master@{#448228}
Committed:
https://chromium.googlesource.com/chromium/src/+/bce01de43097d69f127f2637d9c3...
==========

[commit-bot: I haz the power, 2017-02-06 11:33:56]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/bce01de43097d69f127f2637d9c3...

[Tom Anderson, 2017-02-16 21:51:23]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2701693003/ by thomasanderson@chromium.org.

The reason for reverting is: Broke button highlight when hovering over name or
email labels.  See crbug.com/693222.