Refactor ImageBuffer to make OffscreenCanvas match HTMLCanvasElement

Refactor ImageBuffer to make OffscreenCanvas match HTMLCanvasElement

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2849463005
Cr-Commit-Position: refs/heads/master@{#468787}
Committed: https://chromium.googlesource.com/chromium/src/+/5db093f59a68e29ed89a115da7e0c86b897a67b1

[fserb, 2017-04-27 21:06:50]

Description was changed from

==========
Refactor ImageBuffer to make OffscreenCanvas match HTMLCanvasElement

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

to

==========
Refactor ImageBuffer to make OffscreenCanvas match HTMLCanvasElement

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[fserb, 2017-04-27 21:07:46]

fserb@chromium.org changed reviewers:
+ junov@chromium.org, kbr@chromium.org

[fserb, 2017-04-27 21:07:46]

kbr: third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
junov: everything else.

[fserb, 2017-04-27 21:08:09]

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

[commit-bot: I haz the power, 2017-04-27 21:08:38]

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-04-27 21:41:14]

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

[commit-bot: I haz the power, 2017-04-27 21:41:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fserb, 2017-04-28 17:14:03]

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

[commit-bot: I haz the power, 2017-04-28 17:15:03]

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-04-28 18:34:18]

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

[commit-bot: I haz the power, 2017-04-28 18:34:19]

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

[fserb, 2017-04-28 18:42:12]

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

[commit-bot: I haz the power, 2017-04-28 18:43:21]

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-04-28 19:49:36]

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

[commit-bot: I haz the power, 2017-04-28 19:49:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fserb, 2017-04-28 20:08:01]

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

[commit-bot: I haz the power, 2017-04-28 20:09:06]

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-04-29 00:11:03]

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

[commit-bot: I haz the power, 2017-04-29 00:11:04]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-04-29 00:21:17]

WebGLRenderingContextBase.cpp lgtm, but please refer to some bug ID. Thanks.

[Justin Novosad, 2017-05-01 15:18:57]

lgtm with suggestions.

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1115: ImageBuffer*
HTMLCanvasElement::GetImageBuffer() const {
Nit (existed before this CL): This function does not deserve "const" IMHO,
especially once its semantics are clarified by renaming it "GetOrCreate". I also
think "const" is overused by the methods that call this function.  const_casts
like the one below are hypocritical and should be avoided with only a few rare
exceptions (e.g. caches, stats needed for validations or consistency checks).

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:166: ImageBuffer*
GetImageBuffer() const override;
This should be called GetOrCreateImageBuffer() IMHO, to make it clear that it
may return non-null even when HasImageBuffer() returns false.

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:263: // argh.
Comment not useful.

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:264:
OffscreenCanvas* non_const_this = const_cast<OffscreenCanvas*>(this);
Another reason to remove "const" from the function signature.

[fserb, 2017-05-02 18:49:30]

un-consted it.
PTAL

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1115: ImageBuffer*
HTMLCanvasElement::GetImageBuffer() const {
On 2017/05/01 at 15:18:57, Justin Novosad wrote:
> Nit (existed before this CL): This function does not deserve "const" IMHO,
especially once its semantics are clarified by renaming it "GetOrCreate". I also
think "const" is overused by the methods that call this function.  const_casts
like the one below are hypocritical and should be avoided with only a few rare
exceptions (e.g. caches, stats needed for validations or consistency checks).

done

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:166: ImageBuffer*
GetImageBuffer() const override;
On 2017/05/01 at 15:18:57, Justin Novosad wrote:
> This should be called GetOrCreateImageBuffer() IMHO, to make it clear that it
may return non-null even when HasImageBuffer() returns false.

done

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:263: // argh.
On 2017/05/01 at 15:18:57, Justin Novosad wrote:
> Comment not useful.

done

https://codereview.chromium.org/2849463005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:264:
OffscreenCanvas* non_const_this = const_cast<OffscreenCanvas*>(this);
On 2017/05/01 at 15:18:57, Justin Novosad wrote:
> Another reason to remove "const" from the function signature.

done done

[Justin Novosad, 2017-05-02 19:23:04]

lgtm

[fserb, 2017-05-02 19:44:49]

The CQ bit was checked by fserb@chromium.org

[fserb, 2017-05-02 19:44:50]

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

[commit-bot: I haz the power, 2017-05-02 19:45:14]

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-05-02 22:04:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493754289946480,
"parent_rev": "33dd9a282e6918138ab3c6f76ca16bc0a3ae333b", "commit_rev":
"5db093f59a68e29ed89a115da7e0c86b897a67b1"}

[commit-bot: I haz the power, 2017-05-02 22:04:19]

Description was changed from

==========
Refactor ImageBuffer to make OffscreenCanvas match HTMLCanvasElement

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor ImageBuffer to make OffscreenCanvas match HTMLCanvasElement

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2849463005
Cr-Commit-Position: refs/heads/master@{#468787}
Committed:
https://chromium.googlesource.com/chromium/src/+/5db093f59a68e29ed89a115da7e0...
==========

[commit-bot: I haz the power, 2017-05-02 22:04:22]

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