V1 of material design find in page bar

V1 of material design find in page bar

This patch adds some new vector icons. Because these icons are part of the primary UI, extra effort has been put into achieving a precise appearance. Hence there are new vector directives as well as different definitions for 1x and everything else. The definitions are not translated from the stock MD icons --- instead, they are hand-crafted to match PNGs provided by sgabriel@.

This patch also changes the layout of the FIP bar. This patch does not attempt to achieve pixel perfection for element layout, which will be a goal of later iterations.

You can see the new UI by running Chrome with --top-chrome-md=material-hybrid

BUG=512443
R=jdonnelly@chromium.org, sky@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/cedd7da71f84a8764ea5e9ccadbaed293e0e6dc8

[Evan Stade, 2015-08-04 22:32:29]

estade@chromium.org changed reviewers:
+ jdonnelly@chromium.org, sky@chromium.org

[Evan Stade, 2015-08-04 22:32:52]

+jdonnelly for ui/gfx/vector_icons/aggregate_vector_icons.py
+sky for the rest

[sky, 2015-08-05 00:06:01]

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:81: // Creates a label that mouse click
focus to a given target.
This comment is missing a few words, and I was confused that the description in
no way matches the class name. In context the name makes sense, but the comment
doesn't help.

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:82: class MatchCountLabel : public
views::Label {
Rather than this, and FocusForwarderView, could we make the textfield go all the
way to the right edge, but set an inset so that it never renders over the match
count?

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:361:
size.set_width(find_text_->GetPreferredSize().width());
It's worth a comment as to why the preferred size doesn't include space for
anything else (maybe a pointer to the comment that sets the
set_default_width_in_chars).

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:28: RETURN_IF_IS(NEW_PATH);
Don't you need to update this?

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/fin...
File ui/gfx/vector_icons/find_close.icon (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/fin...
ui/gfx/vector_icons/find_close.icon:1: // A variant of ic_close with rounded
corners.
Random comment on these. Should we be adding copyrights?

https://codereview.chromium.org/1270783005/diff/60001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/views/controls/separ...
ui/views/controls/separator.cc:50: size.Enlarge(insets.width(),
insets.height());
Your change here looks right, but how about landing this separately? I suggest
this mostly because I wouldn't be surprised if there are places that rely on the
old logic, and it might be best to resolve that separately.
As I'm ok with this change feel free to TBR it.

[Justin Donnelly, 2015-08-05 14:32:49]

lgtm

lgtm for aggregate_vector_icons.py

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
File ui/gfx/vector_icons/aggregate_vector_icons.py (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
ui/gfx/vector_icons/aggregate_vector_icons.py:31: output_header.write(line)
How about a continue here to match the new structure of the code below?

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
ui/gfx/vector_icons/aggregate_vector_icons.py:37: os.path.basename(icon_name))
You don't need os.path.basename here, you already got rid of the path above.

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
ui/gfx/vector_icons/aggregate_vector_icons.py:54: (icon_name, scale_factor) =
os.path.splitext(os.path.basename(icon_name))
Ditto

[Evan Stade, 2015-08-05 19:04:26]

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:81: // Creates a label that mouse click
focus to a given target.
On 2015/08/05 00:06:01, sky wrote:
> This comment is missing a few words, and I was confused that the description
in
> no way matches the class name. In context the name makes sense, but the
comment
> doesn't help.

Done.

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:82: class MatchCountLabel : public
views::Label {
On 2015/08/05 00:06:01, sky wrote:
> Rather than this, and FocusForwarderView, could we make the textfield go all
the
> way to the right edge, but set an inset so that it never renders over the
match
> count?

that sounds more difficult because
a) we'd probably have to do explicit layout, instead of using a LayoutManager. I
think this is more error-prone.
b) The match count label changes width depending on its contents ("1 of 1" vs "1
of 1000"), so the inset would have to change as well.

I do think I've found a more comprehensive way to do this forwarding though by
using ViewTargeter[Delegate]. Unfortunately I still have to subclass Label to
give it event handling (note that the code we're commenting on here doesn't
actually work, as it also needs the CanProcessEventsWithinSubtree override). The
other option would be to add a boolean + setter to Label. Let me know if you
prefer that.

https://codereview.chromium.org/1270783005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:361:
size.set_width(find_text_->GetPreferredSize().width());
On 2015/08/05 00:06:01, sky wrote:
> It's worth a comment as to why the preferred size doesn't include space for
> anything else (maybe a pointer to the comment that sets the
> set_default_width_in_chars).

Done.

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:28: RETURN_IF_IS(NEW_PATH);
On 2015/08/05 00:06:01, sky wrote:
> Don't you need to update this?

only if I want it to keep working!

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
File ui/gfx/vector_icons/aggregate_vector_icons.py (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
ui/gfx/vector_icons/aggregate_vector_icons.py:31: output_header.write(line)
On 2015/08/05 14:32:49, Justin Donnelly wrote:
> How about a continue here to match the new structure of the code below?

Done.

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
ui/gfx/vector_icons/aggregate_vector_icons.py:37: os.path.basename(icon_name))
On 2015/08/05 14:32:49, Justin Donnelly wrote:
> You don't need os.path.basename here, you already got rid of the path above.

Done.

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/agg...
ui/gfx/vector_icons/aggregate_vector_icons.py:54: (icon_name, scale_factor) =
os.path.splitext(os.path.basename(icon_name))
On 2015/08/05 14:32:49, Justin Donnelly wrote:
> Ditto

Done.

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/fin...
File ui/gfx/vector_icons/find_close.icon (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/gfx/vector_icons/fin...
ui/gfx/vector_icons/find_close.icon:1: // A variant of ic_close with rounded
corners.
On 2015/08/05 00:06:01, sky wrote:
> Random comment on these. Should we be adding copyrights?

I dunno, this rule seems ambiguous in this case: "Every file containing source
code must include copyright and license information." We don't have any
copyright notice on .png files AFAIK, but I can ping a lawyer.

I added the comments in case people came along and wondered why we weren't just
using ic_close (i.e. skiafying the SVG paths) since these icons closely resemble
those icons.

https://codereview.chromium.org/1270783005/diff/60001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/1270783005/diff/60001/ui/views/controls/separ...
ui/views/controls/separator.cc:50: size.Enlarge(insets.width(),
insets.height());
On 2015/08/05 00:06:01, sky wrote:
> Your change here looks right, but how about landing this separately? I suggest
> this mostly because I wouldn't be surprised if there are places that rely on
the
> old logic, and it might be best to resolve that separately.
> As I'm ok with this change feel free to TBR it.

Done.

[sky, 2015-08-05 21:37:06]

LGTM

[Evan Stade, 2015-08-06 19:03:55]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-08-06 19:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdonnelly@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1270783005/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-06 19:04:45]

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

[commit-bot: I haz the power, 2015-08-06 19:54:03]

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

[commit-bot: I haz the power, 2015-08-06 19:54:04]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Evan Stade, 2015-08-06 19:54:36]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-08-06 19:55:36]

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

[commit-bot: I haz the power, 2015-08-06 20:21:09]

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

[commit-bot: I haz the power, 2015-08-06 20:21:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Evan Stade, 2015-08-06 21:38:58]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-08-06 21:38:58]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
jdonnelly@chromium.org
Link to the patchset: https://codereview.chromium.org/1270783005/#ps120001
(title: "suffixual f")

[commit-bot: I haz the power, 2015-08-06 21:40:40]

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

[Evan Stade, 2015-08-06 23:00:56]

Committed patchset #7 (id:120001) manually as
cedd7da71f84a8764ea5e9ccadbaed293e0e6dc8 (presubmit successful).