Reuse submenu arrow images for trees (like the edit bookmark tree).

Reuse submenu arrow images for trees (like the edit bookmark tree).

This makes our UI more consistent and makes the arrows look
significantly better at 1.25x scale.

BUG=692270
Review-Url: https://codereview.chromium.org/2694323003
Cr-Commit-Position: refs/heads/master@{#450822}
Committed: https://chromium.googlesource.com/chromium/src/+/81217a22a1dc79485433f211e35eaca71167a369

[Evan Stade, 2017-02-15 00:34:37]

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

[Evan Stade, 2017-02-15 00:45:33]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 00:46:37]

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

[Peter Kasting, 2017-02-15 01:05:31]

LGTM

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
ui/views/controls/tree/tree_view.cc:833:
GetNativeTheme()->GetSystemColor(text_color_id(false, false))));
Any particular reason to move away from kColorId_TreeArrow?  If this is
equivalent or better, I'm fine with it, but in that case, this was the only user
of that color, so we should rip it out entirely.

If we keep this derivation, should we be matching the conditions on line 816
above here?

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
ui/views/controls/tree/tree_view.cc:845: arrow_bounds.y());
Nit: I think this is equivalent, and seems shorter and more readable:

  gfx::Rect arrow_bounds(kArrowRegionSize, node_bounds.height());
  arrow_bounds.ClampToCenteredSize(arrow.size());
  canvas->DrawImageInt(arrow, node_bounds.GetMirroredXForRect(arrow_bounds),
                       node_bounds.y() + arrow_bounds.y());

[Evan Stade, 2017-02-15 01:28:58]

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
ui/views/controls/tree/tree_view.cc:833:
GetNativeTheme()->GetSystemColor(text_color_id(false, false))));
On 2017/02/15 01:05:31, Peter Kasting wrote:
> Any particular reason to move away from kColorId_TreeArrow?  If this is
> equivalent or better, I'm fine with it, but in that case, this was the only
user
> of that color, so we should rip it out entirely.

Ah, forgot about that color. I think we should remove it. The derived version is
better because it will be more consistent with other ui (it will come out to
5a5a5a instead of 7a7a7a).

> 
> If we keep this derivation, should we be matching the conditions on line 816
> above here?

the selection highlight doesn't extend to the arrow, so I don't think we should
be changing its color ever. I'm ambivalent about using text_color_id() vs just
kColorId_TreeText

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
ui/views/controls/tree/tree_view.cc:845: arrow_bounds.y());
On 2017/02/15 01:05:31, Peter Kasting wrote:
> Nit: I think this is equivalent, and seems shorter and more readable:
> 
>   gfx::Rect arrow_bounds(kArrowRegionSize, node_bounds.height());
>   arrow_bounds.ClampToCenteredSize(arrow.size());
>   canvas->DrawImageInt(arrow, node_bounds.GetMirroredXForRect(arrow_bounds),
>                        node_bounds.y() + arrow_bounds.y());

the arrow square isn't centered in the node bounds.

[Peter Kasting, 2017-02-15 02:05:28]

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
ui/views/controls/tree/tree_view.cc:845: arrow_bounds.y());
On 2017/02/15 01:28:58, Evan Stade wrote:
> On 2017/02/15 01:05:31, Peter Kasting wrote:
> > Nit: I think this is equivalent, and seems shorter and more readable:
> > 
> >   gfx::Rect arrow_bounds(kArrowRegionSize, node_bounds.height());
> >   arrow_bounds.ClampToCenteredSize(arrow.size());
> >   canvas->DrawImageInt(arrow, node_bounds.GetMirroredXForRect(arrow_bounds),
> >                        node_bounds.y() + arrow_bounds.y());
> 
> the arrow square isn't centered in the node bounds.

I know.  The code I wrote won't center it in the node bounds.  It will center it
vertically within the node height, and horizontally within a
kArrowRegionSize-width space pinned to the leading edge of the control (mirrored
for RTL).

[commit-bot: I haz the power, 2017-02-15 03:16:01]

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

[commit-bot: I haz the power, 2017-02-15 03:16:02]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-02-15 15:58:11]

On 2017/02/15 02:05:28, Peter Kasting wrote:
>
https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
> File ui/views/controls/tree/tree_view.cc (right):
> 
>
https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
> ui/views/controls/tree/tree_view.cc:845: arrow_bounds.y());
> On 2017/02/15 01:28:58, Evan Stade wrote:
> > On 2017/02/15 01:05:31, Peter Kasting wrote:
> > > Nit: I think this is equivalent, and seems shorter and more readable:
> > > 
> > >   gfx::Rect arrow_bounds(kArrowRegionSize, node_bounds.height());
> > >   arrow_bounds.ClampToCenteredSize(arrow.size());
> > >   canvas->DrawImageInt(arrow,
node_bounds.GetMirroredXForRect(arrow_bounds),
> > >                        node_bounds.y() + arrow_bounds.y());
> > 
> > the arrow square isn't centered in the node bounds.
> 
> I know.  The code I wrote won't center it in the node bounds.  It will center
it
> vertically within the node height, and horizontally within a
> kArrowRegionSize-width space pinned to the leading edge of the control
(mirrored
> for RTL).

I tried this code (assuming you meant GetMirroredXForRect() and not
node_bounds.GetMirroredXForRect()). The arrow isn't pinned to the leading edge
of node_bounds (what we want) but to the leading edge of the table. When I was
writing this code I did try to use GetMirroredXForRect but I don't think it
helps us here, in part because node_bounds is already mirrored.

[Evan Stade, 2017-02-15 16:06:25]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 16:06:42]

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

[Evan Stade, 2017-02-15 16:07:36]

estade@chromium.org changed reviewers:
+ msw@chromium.org

[Evan Stade, 2017-02-15 16:07:37]

+msw for owners

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
ui/views/controls/tree/tree_view.cc:833:
GetNativeTheme()->GetSystemColor(text_color_id(false, false))));
On 2017/02/15 01:28:58, Evan Stade wrote:
> On 2017/02/15 01:05:31, Peter Kasting wrote:
> > Any particular reason to move away from kColorId_TreeArrow?  If this is
> > equivalent or better, I'm fine with it, but in that case, this was the only
> user
> > of that color, so we should rip it out entirely.
> 
> Ah, forgot about that color. I think we should remove it. The derived version
is
> better because it will be more consistent with other ui (it will come out to
> 5a5a5a instead of 7a7a7a).
> 
> > 
> > If we keep this derivation, should we be matching the conditions on line 816
> > above here?
> 
> the selection highlight doesn't extend to the arrow, so I don't think we
should
> be changing its color ever. I'm ambivalent about using text_color_id() vs just
> kColorId_TreeText

done

[commit-bot: I haz the power, 2017-02-15 18:03:46]

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

[commit-bot: I haz the power, 2017-02-15 18:03:46]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-02-15 19:23:35]

On 2017/02/15 15:58:11, Evan Stade wrote:
> On 2017/02/15 02:05:28, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
> > File ui/views/controls/tree/tree_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/2694323003/diff/1/ui/views/controls/tree/tree...
> > ui/views/controls/tree/tree_view.cc:845: arrow_bounds.y());
> > On 2017/02/15 01:28:58, Evan Stade wrote:
> > > On 2017/02/15 01:05:31, Peter Kasting wrote:
> > > > Nit: I think this is equivalent, and seems shorter and more readable:
> > > > 
> > > >   gfx::Rect arrow_bounds(kArrowRegionSize, node_bounds.height());
> > > >   arrow_bounds.ClampToCenteredSize(arrow.size());
> > > >   canvas->DrawImageInt(arrow,
> node_bounds.GetMirroredXForRect(arrow_bounds),
> > > >                        node_bounds.y() + arrow_bounds.y());
> > > 
> > > the arrow square isn't centered in the node bounds.
> > 
> > I know.  The code I wrote won't center it in the node bounds.  It will
center
> it
> > vertically within the node height, and horizontally within a
> > kArrowRegionSize-width space pinned to the leading edge of the control
> (mirrored
> > for RTL).
> 
> I tried this code (assuming you meant GetMirroredXForRect() and not
> node_bounds.GetMirroredXForRect()).

That's actually the crucial difference.  I did mean
node_bounds.GetMirroredXForRect(), which doesn't compile since I had "View" and
"Rect" crossed up in my head.  An unqualified GetMirroredXForRect() definitely
won't work conceptually.  There's no way to do what I wanted today, we'd need
something like a "Rect::GetMirroredXInRect" method instead (and have Views
implement all its existing GetMirroredX...() functions by calling through to
methods on its bounds()).  I'm not sure whether the gfx people would like this
-- it probably makes more sense as a non-member helper somewhere.  I'm not sure
whether it's worth pursuing.

Not for this CL, I guess, but I'm wondering why this class goes to all this
trouble to manually paint things in the right spots instead of just using more
child views.  Why is this thing not an ImageView that would then get
auto-mirrored by its parent?  Having to do a lot of manual mirroring calls is
usually a bad sign...

[msw, 2017-02-15 20:13:37]

lgtm modulo Peter's concern (for followup?)

[Evan Stade, 2017-02-15 22:02:23]

> Not for this CL, I guess, but I'm wondering why this class goes to all this
> trouble to manually paint things in the right spots instead of just using more
> child views.  Why is this thing not an ImageView that would then get
> auto-mirrored by its parent?  Having to do a lot of manual mirroring calls is
> usually a bad sign...

I don't think this code has been touched in many years (hence the authorless
TODO). There may not have been as many other view classes or useful
functionality like automatic rtl mirroring when this was initially written, and
it's never been significantly overhauled.

[Evan Stade, 2017-02-15 22:02:42]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-02-15 22:02:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2694323003/#ps20001
(title: "remove TreeArrow color")

[commit-bot: I haz the power, 2017-02-15 22:04:39]

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-15 22:48:23]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487196162195840,
"parent_rev": "d81f57c5700ed617cc183847ad3e0f35df1c8221", "commit_rev":
"81217a22a1dc79485433f211e35eaca71167a369"}

[commit-bot: I haz the power, 2017-02-15 22:50:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487196162195840,
"parent_rev": "d81f57c5700ed617cc183847ad3e0f35df1c8221", "commit_rev":
"81217a22a1dc79485433f211e35eaca71167a369"}

[commit-bot: I haz the power, 2017-02-15 22:51:34]

Description was changed from

==========
Reuse submenu arrow images for trees (like the edit bookmark tree).

This makes our UI more consistent and makes the arrows look
significantly better at 1.25x scale.

BUG=692270
==========

to

==========
Reuse submenu arrow images for trees (like the edit bookmark tree).

This makes our UI more consistent and makes the arrows look
significantly better at 1.25x scale.

BUG=692270

Review-Url: https://codereview.chromium.org/2694323003
Cr-Commit-Position: refs/heads/master@{#450822}
Committed:
https://chromium.googlesource.com/chromium/src/+/81217a22a1dc79485433f211e35e...
==========

[commit-bot: I haz the power, 2017-02-15 22:51:41]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/81217a22a1dc79485433f211e35e...