remove legacy Skia flags

remove legacy Skia flags

BUG=
TBR=asvitkine, dcheng

Committed: https://crrev.com/111c379025c22713d8e404c28a46e8cd06eca620
Cr-Commit-Position: refs/heads/master@{#429331}

[reed1, 2016-11-01 20:56:33]

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

[commit-bot: I haz the power, 2016-11-01 20:57:09]

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

[reed1, 2016-11-01 20:57:11]

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

[f(malita), 2016-11-01 21:01:02]

lgtm

[Stephen White, 2016-11-01 21:11:48]

https://codereview.chromium.org/2472543002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2472543002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:458:
compositePaint.setImageFilter(sk_ref_sp(filter));
Rather than doing an explicit ref here, I think we should be returning an
sk_sp<SkImageFilter> from stateGetFilter() (and transitively, from
CanvasRenderingContext*::getFilter()). Internally, they're just doing a get() on
an sk_sp<> member variable anyway.

[reed1, 2016-11-01 21:28:14]

That is a much bigger change, as there are multiple subclasses that will need to
be updated. I also see that loopers are handled similarly (ref'd only when
needed, rather than every time a getter is called). Can such a change be
separate from this one (which removes a deprecated calling pattern) or do they
all need to happen as part of this CL?

[Stephen White, 2016-11-01 21:36:33]

On 2016/11/01 21:28:14, reed1 wrote:
> That is a much bigger change, as there are multiple subclasses that will need
to
> be updated. I also see that loopers are handled similarly (ref'd only when
> needed, rather than every time a getter is called). Can such a change be
> separate from this one (which removes a deprecated calling pattern) or do they
> all need to happen as part of this CL?

If my search is correct, there are only 13 files with calls to sk_ref_sp() in 
WebKit. I'd really prefer to avoid adding more, since they're error-prone.

I think we have adequate telemetry testing coverage in case this is a
performance 
hotspot (which I doubt).

[commit-bot: I haz the power, 2016-11-01 23:32:53]

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

[commit-bot: I haz the power, 2016-11-01 23:32:54]

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

[reed1, 2016-11-02 13:50:11]

I will make this change. However, I don't see sk_ref_sp as a bug. SkPaint's
getter returns a bare pointer on purpose. Many callers don't want to pay for the
overhead of a ref() call which happens each time we copy a sk_sp(), so I don't
view this instances as a bug.

[reed1, 2016-11-02 14:23:51]

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

[commit-bot: I haz the power, 2016-11-02 14:24:08]

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

[f(malita), 2016-11-02 14:24:50]

https://codereview.chromium.org/2472543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2472543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:458:
compositePaint.setImageFilter(filter);
nit: should be safe to std::move(filter) here.

[commit-bot: I haz the power, 2016-11-02 14:26:47]

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

[commit-bot: I haz the power, 2016-11-02 14:26:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[reed1, 2016-11-02 15:27:09]

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

[commit-bot: I haz the power, 2016-11-02 15:27:25]

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

[reed1, 2016-11-02 15:30:37]

https://codereview.chromium.org/2472543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
(right):

https://codereview.chromium.org/2472543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h:458:
compositePaint.setImageFilter(filter);
On 2016/11/02 14:24:50, f(malita) wrote:
> nit: should be safe to std::move(filter) here.

Done.

[Stephen White, 2016-11-02 15:33:43]

senorblanco@chromium.org changed reviewers:
+ junov@chromium.org

[Stephen White, 2016-11-02 15:33:44]

LGTM, but will leave for Justin.

[Justin Novosad, 2016-11-02 15:39:21]

LGTM with nit

https://codereview.chromium.org/2472543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2472543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:379:
setColorSpaceAndComputeTransform(colorSpace);
Not new to this CL, but missing std::move here.

[reed1, 2016-11-02 15:43:25]

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

[commit-bot: I haz the power, 2016-11-02 15:43:45]

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

[reed1, 2016-11-02 15:54:45]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-11-02 15:54:52]

The CQ bit was checked by reed@google.com

[reed1, 2016-11-02 15:54:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, senorblanco@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2472543002/#ps80001
(title: "address nit")

[commit-bot: I haz the power, 2016-11-02 15:54:59]

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, 2016-11-02 16:01:03]

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

[commit-bot: I haz the power, 2016-11-02 16:01:04]

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, 2016-11-02 16:30:11]

Description was changed from

==========
remove legacy Skia flags

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

to

==========
remove legacy Skia flags

BUG=
TBR=asvitkine, dcheng
==========

[reed1, 2016-11-02 16:30:23]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-11-02 16:30:43]

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, 2016-11-02 17:53:28]

Description was changed from

==========
remove legacy Skia flags

BUG=
TBR=asvitkine, dcheng
==========

to

==========
remove legacy Skia flags

BUG=
TBR=asvitkine, dcheng
==========

[commit-bot: I haz the power, 2016-11-02 17:53:31]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-02 18:23:19]

Description was changed from

==========
remove legacy Skia flags

BUG=
TBR=asvitkine, dcheng
==========

to

==========
remove legacy Skia flags

BUG=
TBR=asvitkine, dcheng

Committed: https://crrev.com/111c379025c22713d8e404c28a46e8cd06eca620
Cr-Commit-Position: refs/heads/master@{#429331}
==========

[commit-bot: I haz the power, 2016-11-02 18:23:21]

Patchset 5 (id:??) landed as
https://crrev.com/111c379025c22713d8e404c28a46e8cd06eca620
Cr-Commit-Position: refs/heads/master@{#429331}