Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2021443003:
Remove WebGL CHROMIUM image fallback logic. (Closed)
|
Created:
4 years, 6 months ago by erikchen Modified:
4 years, 6 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, Rik, f(malita), blink-reviews, piman+watch_chromium.org, Stephen Chennney, ajuma+watch_chromium.org, danakj+watch_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove WebGL CHROMIUM image fallback logic.
A failure to allocate a CHROMIUM image should be treated the same as a failure
in texImage2D. This removes some unnecessary, poorly tested logic from WebGL.
BUG=581777
Committed: https://crrev.com/a7d7f50f3a02bcf5ec17908749959af8890090c8
Cr-Commit-Position: refs/heads/master@{#396982}
Patch Set 1 #Patch Set 2 : Fix test. #
Total comments: 7
Patch Set 3 : Comments from kbr. #Patch Set 4 : Comments from danakj. #
Messages
Total messages: 25 (12 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021443003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021443003/1
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Description was changed from ========== Remove WebGL CHROMIUM image fallback logic. A failure to allocate a CHROMIUM image should be treated the same as a failure in texImage2D. This removes some untested, unnecessary logic from WebGL. BUG=581777 ========== to ========== Remove WebGL CHROMIUM image fallback logic. A failure to allocate a CHROMIUM image should be treated the same as a failure in texImage2D. This removes some unnecessary, poorly tested logic from WebGL. BUG=581777 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021443003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021443003/20001
erikchen@chromium.org changed reviewers: + kbr@chromium.org
kbr: Please review.
LGTM. Minor naming suggestions (to add *ForTesting). https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:196: bool createImageChromiumShouldFail() const { return m_createImageChromiumFail; } createImageChromiumShouldFailForTesting? shouldCreateImageChromiumFailForTesting? https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:198: void setCreateImageChromiumFail(bool fail) { m_createImageChromiumFail = fail; } setCreateImageChromiumShouldFailForTesting? setCreateImageChromiumFailForTesting?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:196: bool createImageChromiumShouldFail() const { return m_createImageChromiumFail; } On 2016/05/28 01:41:58, Ken Russell wrote: > createImageChromiumShouldFailForTesting? > shouldCreateImageChromiumFailForTesting? Removed, as it's unused. https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:198: void setCreateImageChromiumFail(bool fail) { m_createImageChromiumFail = fail; } On 2016/05/28 01:41:58, Ken Russell wrote: > setCreateImageChromiumShouldFailForTesting? > setCreateImageChromiumFailForTesting? Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2021443003/#ps40001 (title: "Comments from kbr.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021443003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021443003/40001
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:198: void setCreateImageChromiumFail(bool fail) { m_createImageChromiumFail = fail; } On 2016/05/31 19:01:28, erikchen wrote: > On 2016/05/28 01:41:58, Ken Russell wrote: > > setCreateImageChromiumShouldFailForTesting? > > setCreateImageChromiumFailForTesting? > > Done. Since this is already inside a test file, the ForTesting would be redundant here.
https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:198: void setCreateImageChromiumFail(bool fail) { m_createImageChromiumFail = fail; } On 2016/05/31 20:34:46, danakj wrote: > On 2016/05/31 19:01:28, erikchen wrote: > > On 2016/05/28 01:41:58, Ken Russell wrote: > > > setCreateImageChromiumShouldFailForTesting? > > > setCreateImageChromiumFailForTesting? > > > > Done. > > Since this is already inside a test file, the ForTesting would be redundant > here. Thanks for pointing that out. I'd not realized this was inside DrawingBufferTest.cpp and not DrawingBuffer.cpp.
The CQ bit was unchecked by erikchen@chromium.org
https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/2021443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:198: void setCreateImageChromiumFail(bool fail) { m_createImageChromiumFail = fail; } On 2016/05/31 20:57:18, Ken Russell wrote: > On 2016/05/31 20:34:46, danakj wrote: > > On 2016/05/31 19:01:28, erikchen wrote: > > > On 2016/05/28 01:41:58, Ken Russell wrote: > > > > setCreateImageChromiumShouldFailForTesting? > > > > setCreateImageChromiumFailForTesting? > > > > > > Done. > > > > Since this is already inside a test file, the ForTesting would be redundant > > here. > > Thanks for pointing that out. I'd not realized this was inside > DrawingBufferTest.cpp and not DrawingBuffer.cpp. Removed the ForTesting suffix.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2021443003/#ps60001 (title: "Comments from danakj.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021443003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2021443003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove WebGL CHROMIUM image fallback logic. A failure to allocate a CHROMIUM image should be treated the same as a failure in texImage2D. This removes some unnecessary, poorly tested logic from WebGL. BUG=581777 ========== to ========== Remove WebGL CHROMIUM image fallback logic. A failure to allocate a CHROMIUM image should be treated the same as a failure in texImage2D. This removes some unnecessary, poorly tested logic from WebGL. BUG=581777 Committed: https://crrev.com/a7d7f50f3a02bcf5ec17908749959af8890090c8 Cr-Commit-Position: refs/heads/master@{#396982} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a7d7f50f3a02bcf5ec17908749959af8890090c8 Cr-Commit-Position: refs/heads/master@{#396982} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
