don't use deprecated helper methods on SkCanvas

don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=avi

Review-Url: https://codereview.chromium.org/2716013002
Cr-Commit-Position: refs/heads/master@{#453089}
Committed: https://chromium.googlesource.com/chromium/src/+/04167b8238edebc1e53fd19058fcce510dd2d767

[reed1, 2017-02-24 18:38:14]

Description was changed from

==========
don't use deprecated helper methods on SkCanvas

BUG=
==========

to

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reed1, 2017-02-24 18:38:32]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2017-02-24 18:38:41]

reed@google.com changed reviewers:
+ fmalita@chromium.org

[commit-bot: I haz the power, 2017-02-24 18:39:12]

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-02-24 21:19:58]

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

[commit-bot: I haz the power, 2017-02-24 21:19:59]

Dry run: This issue passed the CQ dry run.

[reed1, 2017-02-24 21:32:10]

reed@google.com changed reviewers:
+ avi@chromium.org, enne@chromium.org, msw@chromium.org, pfeldman@chromium.org,
sky@chromium.org, vmpstr@chromium.org

[reed1, 2017-02-24 21:32:10]

need OWNERS for
cc/
content/
ui/views

[msw, 2017-02-24 21:34:35]

ui/views/painter.cc lgtm

[enne (OOO), 2017-02-24 21:35:32]

cc lgtm

[f(malita), 2017-02-24 21:37:19]

https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
File cc/layers/picture_image_layer_unittest.cc (right):

https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
cc/layers/picture_image_layer_unittest.cc:40: image_canvas->drawRect({0.f, 0.f,
100.f, 100.f}, blue_paint);
I find initializer lists fragile.  Is that LTRB or XYWH?  What if the internal
structure of SkRect changes?

MakeWH/MakeLTRB/etc. would be a clearer choice, no?

[reed1, 2017-02-24 21:39:02]

https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
File cc/layers/picture_image_layer_unittest.cc (right):

https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
cc/layers/picture_image_layer_unittest.cc:40: image_canvas->drawRect({0.f, 0.f,
100.f, 100.f}, blue_paint);
On 2017/02/24 21:37:19, f(malita) wrote:
> I find initializer lists fragile.  Is that LTRB or XYWH?  What if the internal
> structure of SkRect changes?
> 
> MakeWH/MakeLTRB/etc. would be a clearer choice, no?

We are using these internally now as well. There is no way we could change it.

[f(malita), 2017-02-24 21:42:54]

On 2017/02/24 21:39:02, reed1 wrote:
>
https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
> File cc/layers/picture_image_layer_unittest.cc (right):
> 
>
https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
> cc/layers/picture_image_layer_unittest.cc:40: image_canvas->drawRect({0.f,
0.f,
> 100.f, 100.f}, blue_paint);
> On 2017/02/24 21:37:19, f(malita) wrote:
> > I find initializer lists fragile.  Is that LTRB or XYWH?  What if the
internal
> > structure of SkRect changes?
> > 
> > MakeWH/MakeLTRB/etc. would be a clearer choice, no?
> 
> We are using these internally now as well. There is no way we could change it.

Fair enough, but these are still harder to read for anyone not familiar with
SkRect internals (e.g. blink rects store xywh, not ltrb).  Just nitpicking
though, LGTM.

[reed1, 2017-02-24 21:44:25]

On 2017/02/24 21:42:54, f(malita) wrote:
> On 2017/02/24 21:39:02, reed1 wrote:
> >
>
https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
> > File cc/layers/picture_image_layer_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2716013002/diff/1/cc/layers/picture_image_lay...
> > cc/layers/picture_image_layer_unittest.cc:40: image_canvas->drawRect({0.f,
> 0.f,
> > 100.f, 100.f}, blue_paint);
> > On 2017/02/24 21:37:19, f(malita) wrote:
> > > I find initializer lists fragile.  Is that LTRB or XYWH?  What if the
> internal
> > > structure of SkRect changes?
> > > 
> > > MakeWH/MakeLTRB/etc. would be a clearer choice, no?
> > 
> > We are using these internally now as well. There is no way we could change
it.
> 
> Fair enough, but these are still harder to read for anyone not familiar with
> SkRect internals (e.g. blink rects store xywh, not ltrb).  Just nitpicking
> though, LGTM.

I will change, but note that the prev call sites was identical ... 4 numbers in
a row, with no labels in the method name for left, top, etc.

[reed1, 2017-02-24 21:54:32]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2017-02-24 21:54:40]

done

[commit-bot: I haz the power, 2017-02-24 21:55:18]

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

[f(malita), 2017-02-24 21:56:15]

On 2017/02/24 21:54:40, reed1 wrote:
> done

thanks, lgtm++

[reed1, 2017-02-24 23:14:50]

The CQ bit was unchecked by reed@google.com

[reed1, 2017-02-24 23:14:56]

The CQ bit was checked by reed@google.com

[reed1, 2017-02-24 23:14:57]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2716013002/#ps20001
(title: "use named makers for rects")

[commit-bot: I haz the power, 2017-02-24 23:15:19]

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-24 23:24:31]

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

[commit-bot: I haz the power, 2017-02-24 23:24:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sky, 2017-02-25 00:31:30]

views LGTM
One comment, unless you're steeped in skia MakeIWH is rather obscure. It would
be clearer if MakeIWH was named something like MakeFromIntWidthAndHeight().

[reed1, 2017-02-25 02:39:22]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2017-02-25 02:39:38]

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-25 02:46:35]

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

[commit-bot: I haz the power, 2017-02-25 02:46:36]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[maganako4, 2017-02-25 08:28:31]

maganako4@gmail.com changed reviewers:
+ maganako4@gmail.com

[reed1, 2017-02-25 10:35:42]

Description was changed from

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TRB=avi
==========

[reed1, 2017-02-25 10:35:54]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2017-02-25 10:35:58]

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-25 10:41:07]

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

[commit-bot: I haz the power, 2017-02-25 10:41:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[reed1, 2017-02-25 12:27:30]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-25 12:27:34]

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

[reed1, 2017-02-25 12:54:26]

The CQ bit was unchecked by reed@google.com

[reed1, 2017-02-25 12:54:32]

The CQ bit was checked by reed@google.com

[reed1, 2017-02-25 12:54:33]

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

[commit-bot: I haz the power, 2017-02-25 12:54:44]

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-25 13:01:47]

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

[commit-bot: I haz the power, 2017-02-25 13:01:49]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[reed1, 2017-02-25 13:10:11]

Description was changed from

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TRB=avi
==========

to

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TRB=esprehn
==========

[reed1, 2017-02-25 13:10:27]

Description was changed from

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TRB=esprehn
==========

to

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=avi
==========

[reed1, 2017-02-25 13:10:41]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2017-02-25 13:10:51]

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-25 13:55:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488028241599020,
"parent_rev": "af8599b05922a8f2b945afe6b7d8468ed3565130", "commit_rev":
"04167b8238edebc1e53fd19058fcce510dd2d767"}

[commit-bot: I haz the power, 2017-02-25 13:56:14]

Description was changed from

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=avi
==========

to

==========
don't use deprecated helper methods on SkCanvas

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=avi

Review-Url: https://codereview.chromium.org/2716013002
Cr-Commit-Position: refs/heads/master@{#453089}
Committed:
https://chromium.googlesource.com/chromium/src/+/04167b8238edebc1e53fd19058fc...
==========

[commit-bot: I haz the power, 2017-02-25 13:56:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/04167b8238edebc1e53fd19058fc...