Stylize PlaceholderImages with icons.

Stylize PlaceholderImages with icons.

This CL stylizes the appearance of PlaceholderImages by adding an icon.
See the spec here:
https://docs.google.com/document/d/1BHeA1azbgCdZgCnr16VN2g7A9MHPQ_dwKn5szh8evMQ/edit

BUG=605351
Review-Url: https://codereview.chromium.org/2915353002
Cr-Commit-Position: refs/heads/master@{#478098}
Committed: https://chromium.googlesource.com/chromium/src/+/aae1e1ce18a9b1923ea7bdfafa5f3afe46050532

[sclittle, 2017-06-02 23:39:35]

sclittle@chromium.org changed reviewers:
+ dglazkov@chromium.org, kinuko@chromium.org

[kinuko, 2017-06-05 06:27:00]

Description was changed from

==========
Stylize PlaceholderImages with icons.

This CL stylizes the appearance of PlaceholderImages by adding an icon.
See the spec here: go/client_lofi_placeholder_spec

BUG=605351
==========

to

==========
Stylize PlaceholderImages with icons.

This CL stylizes the appearance of PlaceholderImages by adding an icon.
See the spec here: go/client_lofi_placeholder_spec

BUG=605351
==========

[kinuko, 2017-06-05 06:27:01]

kinuko@chromium.org changed reviewers:
+ fmalita@chromium.org

[kinuko, 2017-06-05 06:30:14]

The code itself looks reasonable, but I don't have a good idea about the
specific settings / detailed changes in PlaceholderImage.cpp.

Adding fmalita@ for graphics/ review

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:59: 
Can you add a link to the 'spec' that explain these constants?

[f(malita), 2017-06-05 13:04:27]

fmalita@chromium.org changed reviewers:
+ vmpstr@chromium.org

[f(malita), 2017-06-05 13:04:28]

graphics/ code looks good, just a couple of questions/suggestions.

+vmpstr to make sure this plays nice with SP custom display lists.

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:68: constexpr
int kIconPaddingY = 5;
Instead of hard-coding UI constants here, is it feasible to let the resource own
this data?

E.g. make the png exactly 40x34 (including transparent padding), then only use
icon_image->width()/height() in the code here and just draw it at its natural
size.  That should also ensure we're not wasting any cycles scaling at raster
time.

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:88:
icon_image->Draw(canvas, base_flags, icon_dest_rect,
Do we want/need the placeholder to observe RespectImageOrientationEnum?

Depending on the answer, it may be OK to call
canvas->drawImage(icon_image->PaintImageForCurrentFrame(),...) directly here.

[vmpstr, 2017-06-05 18:05:29]

This should work fine with custom display lists. PaintImage currently is just an
SkImage with a couple of extra bools and for this placeholder, I don't think it
really matters what the bools are since they dictate whether we can checker the
image, but here we don't want to do it anyway.

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:62:
flags.setColor(0x80D9D9D9);
nit: You can use SkColorSetARGB or SkColorSetARGBMacro here instead of a magic
hex

[sclittle, 2017-06-05 22:26:08]

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:59: 
On 2017/06/05 06:30:13, kinuko wrote:
> Can you add a link to the 'spec' that explain these constants?

Done.

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:62:
flags.setColor(0x80D9D9D9);
On 2017/06/05 18:05:28, vmpstr wrote:
> nit: You can use SkColorSetARGB or SkColorSetARGBMacro here instead of a magic
> hex

Done.

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:68: constexpr
int kIconPaddingY = 5;
On 2017/06/05 13:04:27, f(malita) wrote:
> Instead of hard-coding UI constants here, is it feasible to let the resource
own
> this data?
> 
> E.g. make the png exactly 40x34 (including transparent padding), then only use
> icon_image->width()/height() in the code here and just draw it at its natural
> size.  That should also ensure we're not wasting any cycles scaling at raster
> time.

Right now, the resource itself is a 4x version of the image, which gets scaled
down when drawn so that it looks nice on high-DPI screens.

I don't think high-DPI PNGs are a thing, and SVGs are heavyweight, so I don't
know of a nice way to do this in the resource itself.

Do you know of any other way to save those cycles? We could maybe hold onto a
static ref of the already-scaled SkImage/PaintImage, but I'm not sure that'll
actually save cycles at raster time.

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:88:
icon_image->Draw(canvas, base_flags, icon_dest_rect,
On 2017/06/05 13:04:27, f(malita) wrote:
> Do we want/need the placeholder to observe RespectImageOrientationEnum?
> 
> Depending on the answer, it may be OK to call
> canvas->drawImage(icon_image->PaintImageForCurrentFrame(),...) directly here.

We probably don't want it to observe the enum, nice catch, thanks. Waiting for
an answer about image scaling before changing it to canvas->drawImage(...),
which doesn't look like it supports scaling.

[sclittle, 2017-06-05 22:26:25]

Description was changed from

==========
Stylize PlaceholderImages with icons.

This CL stylizes the appearance of PlaceholderImages by adding an icon.
See the spec here: go/client_lofi_placeholder_spec

BUG=605351
==========

to

==========
Stylize PlaceholderImages with icons.

This CL stylizes the appearance of PlaceholderImages by adding an icon.
See the spec here:
https://docs.google.com/document/d/1BHeA1azbgCdZgCnr16VN2g7A9MHPQ_dwKn5szh8ev...

BUG=605351
==========

[f(malita), 2017-06-06 14:04:21]

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:68: constexpr
int kIconPaddingY = 5;
On 2017/06/05 22:26:08, sclittle wrote:
> On 2017/06/05 13:04:27, f(malita) wrote:
> > Instead of hard-coding UI constants here, is it feasible to let the resource
> own
> > this data?
> > 
> > E.g. make the png exactly 40x34 (including transparent padding), then only
use
> > icon_image->width()/height() in the code here and just draw it at its
natural
> > size.  That should also ensure we're not wasting any cycles scaling at
raster
> > time.
> 
> Right now, the resource itself is a 4x version of the image, which gets scaled
> down when drawn so that it looks nice on high-DPI screens.

Ah, yes, makes sense to have higher res for hidpi.

> 
> I don't think high-DPI PNGs are a thing, and SVGs are heavyweight, so I don't
> know of a nice way to do this in the resource itself.
> 
> Do you know of any other way to save those cycles? We could maybe hold onto a
> static ref of the already-scaled SkImage/PaintImage, but I'm not sure that'll
> actually save cycles at raster time.

It's not a huge deal, if there's a good reason do scale (hidpi) then I'm fine
with it.

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:88:
icon_image->Draw(canvas, base_flags, icon_dest_rect,
On 2017/06/05 22:26:08, sclittle wrote:
> On 2017/06/05 13:04:27, f(malita) wrote:
> > Do we want/need the placeholder to observe RespectImageOrientationEnum?
> > 
> > Depending on the answer, it may be OK to call
> > canvas->drawImage(icon_image->PaintImageForCurrentFrame(),...) directly
here.
> 
> We probably don't want it to observe the enum, nice catch, thanks. Waiting for
> an answer about image scaling before changing it to canvas->drawImage(...),
> which doesn't look like it supports scaling.

You can use PaintCanvas::drawImageRect() to scale.

https://codereview.chromium.org/2915353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:65:
flags.setColor(SkColorSetARGBMacro(0x80, 0xD9, 0xD9, 0xD9));
nit: SkColorSetARGB()

[sclittle, 2017-06-07 19:43:54]

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:88:
icon_image->Draw(canvas, base_flags, icon_dest_rect,
On 2017/06/06 14:04:21, f(malita) wrote:
> On 2017/06/05 22:26:08, sclittle wrote:
> > On 2017/06/05 13:04:27, f(malita) wrote:
> > > Do we want/need the placeholder to observe RespectImageOrientationEnum?
> > > 
> > > Depending on the answer, it may be OK to call
> > > canvas->drawImage(icon_image->PaintImageForCurrentFrame(),...) directly
> here.
> > 
> > We probably don't want it to observe the enum, nice catch, thanks. Waiting
for
> > an answer about image scaling before changing it to canvas->drawImage(...),
> > which doesn't look like it supports scaling.
> 
> You can use PaintCanvas::drawImageRect() to scale.

Done.

https://codereview.chromium.org/2915353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp (right):

https://codereview.chromium.org/2915353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PlaceholderImage.cpp:65:
flags.setColor(SkColorSetARGBMacro(0x80, 0xD9, 0xD9, 0xD9));
On 2017/06/06 14:04:21, f(malita) wrote:
> nit: SkColorSetARGB()

Done.

[f(malita), 2017-06-07 20:18:47]

Thanks, platform/graphics LGTM.

[kinuko, 2017-06-08 04:56:37]

lgtm/2

[sclittle, 2017-06-08 17:58:26]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2017-06-08 17:58:47]

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-06-08 21:48:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496944706404440,
"parent_rev": "58e1138e4c7edeceff06c91ce16bd984906655da", "commit_rev":
"aae1e1ce18a9b1923ea7bdfafa5f3afe46050532"}

[commit-bot: I haz the power, 2017-06-08 21:48:20]

Description was changed from

==========
Stylize PlaceholderImages with icons.

This CL stylizes the appearance of PlaceholderImages by adding an icon.
See the spec here:
https://docs.google.com/document/d/1BHeA1azbgCdZgCnr16VN2g7A9MHPQ_dwKn5szh8ev...

BUG=605351
==========

to

==========
Stylize PlaceholderImages with icons.

This CL stylizes the appearance of PlaceholderImages by adding an icon.
See the spec here:
https://docs.google.com/document/d/1BHeA1azbgCdZgCnr16VN2g7A9MHPQ_dwKn5szh8ev...

BUG=605351

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

[commit-bot: I haz the power, 2017-06-08 21:48:22]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/aae1e1ce18a9b1923ea7bdfafa5f...