ui: Remove a bunch of uses of gfx::Canvas::ExtractImageRep()

ui: Remove a bunch of uses of gfx::Canvas::ExtractImageRep()

This method calls PaintCanvas::getBaseLayerSize() which should not
exist. Since these callers are just trying to generate a bitmap, close
the loop and have them make a bitmap, then draw/raster into it.

I have manually tested each ui case to verify that I see it in the
browser and that it is the same after (this was not easy).

BUG=671433
Review-Url: https://codereview.chromium.org/2760133002
Cr-Commit-Position: refs/heads/master@{#458447}
Committed: https://chromium.googlesource.com/chromium/src/+/4c5cfacb5d7d63b4b264c2783dfb1cbf32aacda5

[danakj, 2017-03-20 22:32:56]

Description was changed from

==========
ui: Remove a bunch of uses of gfx::Canvas::ExtractImageRep()

This method calls PaintCanvas::getBaseLayerSize() which should not
exist. Since these callers are just trying to generate a bitmap, close
the loop and have them make a bitmap, then draw/raster into it.

I have manually tested each ui case to verify that I see it in the
browser and that it is the same after (this was not easy).

R=enne@chromium.org, pkasting@chromium.org
BUG=671433
==========

to

==========
ui: Remove a bunch of uses of gfx::Canvas::ExtractImageRep()

This method calls PaintCanvas::getBaseLayerSize() which should not
exist. Since these callers are just trying to generate a bitmap, close
the loop and have them make a bitmap, then draw/raster into it.

I have manually tested each ui case to verify that I see it in the
browser and that it is the same after (this was not easy).

BUG=671433
==========

[danakj, 2017-03-20 22:32:56]

danakj@chromium.org changed reviewers:
+ blundell@chromium.org, oshima@chromium.org, sadrul@chromium.org

[danakj, 2017-03-20 22:34:11]

enne: overall or just fyi
pkasting: chrome
blundell: favicon
sadrul: ui
oshima: ash

[danakj, 2017-03-20 22:34:14]

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

[danakj, 2017-03-20 22:34:28]

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

[commit-bot: I haz the power, 2017-03-20 22:34:59]

Dry run: 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-03-20 22:45:45]

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

[commit-bot: I haz the power, 2017-03-20 22:45:46]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Peter Kasting, 2017-03-20 22:49:21]

LGTM

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_item.cc:85: SkBitmap mask_bitmap;
Nit: Trivial, but I liked having this block below the DrawImageInt() call
because it minimized the lifetimes of the temps in it.

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/app_launcher_login_handler.cc (right):

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/app_launcher_login_handler.cc:182: if (b) {
Why this change?  |b| doesn't add any clarity, and we don't read |entry| if the
conditional fails.

If you wanted to do this, I'd pick a more descriptive name than |b|, but I'd
probably just leave it as it was.

[sadrul, 2017-03-21 00:15:20]

lgtm

https://codereview.chromium.org/2760133002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/blue_button_unittest.cc (right):

https://codereview.chromium.org/2760133002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:42: true);
true /* is_opaque */?

(I see the old code did not have this too :( )

[blundell, 2017-03-21 08:21:01]

It looks like the trybots are not happy due to real problems?

[danakj, 2017-03-21 15:25:57]

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_item.cc:85: SkBitmap mask_bitmap;
On 2017/03/20 22:49:21, Peter Kasting wrote:
> Nit: Trivial, but I liked having this block below the DrawImageInt() call
> because it minimized the lifetimes of the temps in it.

Sure, done. I liked having all the calls to |canvas| together fwiw.

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/app_launcher_login_handler.cc (right):

https://codereview.chromium.org/2760133002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/app_launcher_login_handler.cc:182: if (b) {
On 2017/03/20 22:49:21, Peter Kasting wrote:
> Why this change?  |b| doesn't add any clarity, and we don't read |entry| if
the
> conditional fails.
> 
> If you wanted to do this, I'd pick a more descriptive name than |b|, but I'd
> probably just leave it as it was.

Oops this was me debug printing stuff, thanks.

https://codereview.chromium.org/2760133002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/blue_button_unittest.cc (right):

https://codereview.chromium.org/2760133002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/blue_button_unittest.cc:42: true);
On 2017/03/21 00:15:20, sadrul wrote:
> true /* is_opaque */?
> 
> (I see the old code did not have this too :( )

Done.

[danakj, 2017-03-21 15:26:07]

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

[commit-bot: I haz the power, 2017-03-21 15:26:31]

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

[blundell, 2017-03-21 15:26:45]

On 2017/03/21 08:21:01, blundell wrote:
> It looks like the trybots are not happy due to real problems?

Sorry, to be more specific it looks like the iOS trybots are not happy due to
real problems?

[danakj, 2017-03-21 15:27:46]

On Tue, Mar 21, 2017 at 11:25 AM, <danakj+owner@chromium.org> wrote:

> On Tue, Mar 21, 2017 at 4:21 AM, <blundell@chromium.org> wrote:
>
>> It looks like the trybots are not happy due to real problems?
>>
>
> I think unrelated to my changes, mash_unittests passes for me locally.
>

Oh there's something with ios GN rules.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-21 15:32:35]

On Tue, Mar 21, 2017 at 4:21 AM, <blundell@chromium.org> wrote:

> It looks like the trybots are not happy due to real problems?
>

I think unrelated to my changes, mash_unittests passes for me locally.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-03-21 15:34:23]

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

[commit-bot: I haz the power, 2017-03-21 15:34:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[danakj, 2017-03-21 15:37:37]

On 2017/03/21 15:27:46, danakj wrote:
> On Tue, Mar 21, 2017 at 11:25 AM, <mailto:danakj+owner@chromium.org> wrote:
> 
> > On Tue, Mar 21, 2017 at 4:21 AM, <mailto:blundell@chromium.org> wrote:
> >
> >> It looks like the trybots are not happy due to real problems?
> >>
> >
> > I think unrelated to my changes, mash_unittests passes for me locally.
> >
> 
> Oh there's something with ios GN rules.

So the error is:

ERROR at //build/config/ios/rules.gni:427:3: assert_no_deps failed.
  source_set(_arch_executable_source) {
  ^------------------------------------
//ios/chrome/test:ios_chrome_perftests_arch_executable_sources has an
assert_no_deps entry:
  //cc/*
which fails for the dependency path:
  //ios/chrome/test:ios_chrome_perftests_arch_executable_sources ->
  //ios/chrome/test:run_all_unittests ->
  //ios/chrome/test:test_support ->
  //ios/chrome/browser:browser ->
  //components/ntp_tiles:ntp_tiles ->
  //components/favicon/core:core ->
  //cc/paint:paint

This is saying that
//ios/chrome/test:ios_chrome_perftests_arch_executable_sources is now depending
on something from //cc which is not allowed.

This reminds me of https://codereview.chromium.org/2640983002 where we learnt
that ios was depending on some ui/gfx stuff but *not* on painting things: 

   "ios depends on skia and ui/gfx but includes a limited set of
    functionality from skia.  This is handled by deliberately not including
    paint related files for ios builds."

The fallback_icon_service.cc however is using gfx::Canvas which is part of the
painting code. You can't use gfx::Canvas without using //cc/paint now, it
already holds a cc::PaintCanvas.

In fact, gfx::Canvas is not included in //ui/gfx on ios:
https://cs.chromium.org/chromium/src/ui/gfx/BUILD.gn?rcl=926bcd0f17209ee4f86d...

  if (!is_ios) {
    sources += [
      "blit.cc",
      "blit.h",
      "canvas.cc",
      "canvas.h",
  ...

So uh.. it's not clear to me how this was using gfx::Canvas and compiling on ios
before.

[danakj, 2017-03-21 15:40:44]

On 2017/03/21 15:37:37, danakj wrote:
> On 2017/03/21 15:27:46, danakj wrote:
> > On Tue, Mar 21, 2017 at 11:25 AM, <mailto:danakj+owner@chromium.org> wrote:
> > 
> > > On Tue, Mar 21, 2017 at 4:21 AM, <mailto:blundell@chromium.org> wrote:
> > >
> > >> It looks like the trybots are not happy due to real problems?
> > >>
> > >
> > > I think unrelated to my changes, mash_unittests passes for me locally.
> > >
> > 
> > Oh there's something with ios GN rules.
> 
> So the error is:
> 
> ERROR at //build/config/ios/rules.gni:427:3: assert_no_deps failed.
>   source_set(_arch_executable_source) {
>   ^------------------------------------
> //ios/chrome/test:ios_chrome_perftests_arch_executable_sources has an
> assert_no_deps entry:
>   //cc/*
> which fails for the dependency path:
>   //ios/chrome/test:ios_chrome_perftests_arch_executable_sources ->
>   //ios/chrome/test:run_all_unittests ->
>   //ios/chrome/test:test_support ->
>   //ios/chrome/browser:browser ->
>   //components/ntp_tiles:ntp_tiles ->
>   //components/favicon/core:core ->
>   //cc/paint:paint
> 
> This is saying that
> //ios/chrome/test:ios_chrome_perftests_arch_executable_sources is now
depending
> on something from //cc which is not allowed.
> 
> This reminds me of https://codereview.chromium.org/2640983002 where we learnt
> that ios was depending on some ui/gfx stuff but *not* on painting things: 
> 
>    "ios depends on skia and ui/gfx but includes a limited set of
>     functionality from skia.  This is handled by deliberately not including
>     paint related files for ios builds."
> 
> The fallback_icon_service.cc however is using gfx::Canvas which is part of the
> painting code. You can't use gfx::Canvas without using //cc/paint now, it
> already holds a cc::PaintCanvas.
> 
> In fact, gfx::Canvas is not included in //ui/gfx on ios:
>
https://cs.chromium.org/chromium/src/ui/gfx/BUILD.gn?rcl=926bcd0f17209ee4f86d...
> 
>   if (!is_ios) {
>     sources += [
>       "blit.cc",
>       "blit.h",
>       "canvas.cc",
>       "canvas.h",
>   ...
> 
> So uh.. it's not clear to me how this was using gfx::Canvas and compiling on
ios
> before.

Oh!

  if (!is_ios) {
    sources += [
      "fallback_icon_client.h",
      "fallback_icon_service.cc",
      "fallback_icon_service.h",
    ]
  }

fallback_icon_service.cc is not part of ios, so I will not include //cc/paint
except on !ios also to match.

[danakj, 2017-03-21 15:42:08]

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

[commit-bot: I haz the power, 2017-03-21 15:42:26]

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

[oshima, 2017-03-21 15:47:24]

ash/ lgtm with a nit

https://codereview.chromium.org/2760133002/diff/40001/ash/common/frame/captio...
File ash/common/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/2760133002/diff/40001/ash/common/frame/captio...
ash/common/frame/caption_buttons/frame_caption_button.cc:128: int centered_y =
(height() - icon_image_.height()) / 2;
The name centered_ is misleading. It's origin of the centered rect right
(= bounds().ClampToCenteredSize(image.size()).origin()) ?

centered_origin_x/y?

[danakj, 2017-03-21 15:55:26]

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

[danakj, 2017-03-21 15:55:32]

https://codereview.chromium.org/2760133002/diff/40001/ash/common/frame/captio...
File ash/common/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/2760133002/diff/40001/ash/common/frame/captio...
ash/common/frame/caption_buttons/frame_caption_button.cc:128: int centered_y =
(height() - icon_image_.height()) / 2;
On 2017/03/21 15:47:24, oshima wrote:
> The name centered_ is misleading. It's origin of the centered rect right
> (= bounds().ClampToCenteredSize(image.size()).origin()) ?
> 
> centered_origin_x/y?

Done.

[commit-bot: I haz the power, 2017-03-21 15:55:33]

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

[danakj, 2017-03-21 15:55:52]

On 2017/03/21 15:40:44, danakj wrote:
> On 2017/03/21 15:37:37, danakj wrote:
> > On 2017/03/21 15:27:46, danakj wrote:
> > > On Tue, Mar 21, 2017 at 11:25 AM, <mailto:danakj+owner@chromium.org>
wrote:
> > > 
> > > > On Tue, Mar 21, 2017 at 4:21 AM, <mailto:blundell@chromium.org> wrote:
> > > >
> > > >> It looks like the trybots are not happy due to real problems?
> > > >>
> > > >
> > > > I think unrelated to my changes, mash_unittests passes for me locally.
> > > >
> > > 
> > > Oh there's something with ios GN rules.
> > 
> > So the error is:
> > 
> > ERROR at //build/config/ios/rules.gni:427:3: assert_no_deps failed.
> >   source_set(_arch_executable_source) {
> >   ^------------------------------------
> > //ios/chrome/test:ios_chrome_perftests_arch_executable_sources has an
> > assert_no_deps entry:
> >   //cc/*
> > which fails for the dependency path:
> >   //ios/chrome/test:ios_chrome_perftests_arch_executable_sources ->
> >   //ios/chrome/test:run_all_unittests ->
> >   //ios/chrome/test:test_support ->
> >   //ios/chrome/browser:browser ->
> >   //components/ntp_tiles:ntp_tiles ->
> >   //components/favicon/core:core ->
> >   //cc/paint:paint
> > 
> > This is saying that
> > //ios/chrome/test:ios_chrome_perftests_arch_executable_sources is now
> depending
> > on something from //cc which is not allowed.
> > 
> > This reminds me of https://codereview.chromium.org/2640983002 where we
learnt
> > that ios was depending on some ui/gfx stuff but *not* on painting things: 
> > 
> >    "ios depends on skia and ui/gfx but includes a limited set of
> >     functionality from skia.  This is handled by deliberately not including
> >     paint related files for ios builds."
> > 
> > The fallback_icon_service.cc however is using gfx::Canvas which is part of
the
> > painting code. You can't use gfx::Canvas without using //cc/paint now, it
> > already holds a cc::PaintCanvas.
> > 
> > In fact, gfx::Canvas is not included in //ui/gfx on ios:
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/BUILD.gn?rcl=926bcd0f17209ee4f86d...
> > 
> >   if (!is_ios) {
> >     sources += [
> >       "blit.cc",
> >       "blit.h",
> >       "canvas.cc",
> >       "canvas.h",
> >   ...
> > 
> > So uh.. it's not clear to me how this was using gfx::Canvas and compiling on
> ios
> > before.
> 
> Oh!
> 
>   if (!is_ios) {
>     sources += [
>       "fallback_icon_client.h",
>       "fallback_icon_service.cc",
>       "fallback_icon_service.h",
>     ]
>   }
> 
> fallback_icon_service.cc is not part of ios, so I will not include //cc/paint
> except on !ios also to match.

@blundell: ios is happy now :) PTAL

[danakj, 2017-03-21 15:56:22]

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

[commit-bot: I haz the power, 2017-03-21 15:56:39]

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

[blundell, 2017-03-21 16:04:46]

//components lgtm, thanks

that class shouldn't be in .../core at all, it should be in .../content instead.
then you wouldn't have had this issue in the first place. i'll follow up there.

[danakj, 2017-03-21 16:07:17]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2017-03-21 16:07:21]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-03-21 16:07:22]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
pkasting@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2760133002/#ps80001
(title: "canvasbounds: ashnit")

[danakj, 2017-03-21 16:07:30]

Thanks everyone!

[commit-bot: I haz the power, 2017-03-21 16:07:40]

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-03-21 17:10:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490112441822790,
"parent_rev": "bf846a94da5c2b4f8ff93a7942d3d50799381675", "commit_rev":
"4c5cfacb5d7d63b4b264c2783dfb1cbf32aacda5"}

[commit-bot: I haz the power, 2017-03-21 17:11:07]

Description was changed from

==========
ui: Remove a bunch of uses of gfx::Canvas::ExtractImageRep()

This method calls PaintCanvas::getBaseLayerSize() which should not
exist. Since these callers are just trying to generate a bitmap, close
the loop and have them make a bitmap, then draw/raster into it.

I have manually tested each ui case to verify that I see it in the
browser and that it is the same after (this was not easy).

BUG=671433
==========

to

==========
ui: Remove a bunch of uses of gfx::Canvas::ExtractImageRep()

This method calls PaintCanvas::getBaseLayerSize() which should not
exist. Since these callers are just trying to generate a bitmap, close
the loop and have them make a bitmap, then draw/raster into it.

I have manually tested each ui case to verify that I see it in the
browser and that it is the same after (this was not easy).

BUG=671433

Review-Url: https://codereview.chromium.org/2760133002
Cr-Commit-Position: refs/heads/master@{#458447}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c5cfacb5d7d63b4b264c2783dfb...
==========

[commit-bot: I haz the power, 2017-03-21 17:11:09]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4c5cfacb5d7d63b4b264c2783dfb...