Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 1407393002: ImageBitmap: Change two enum uses. (Closed)

Created:
2 years, 8 months ago by jbroman
Modified:
2 years, 8 months ago
CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, vmpstr+blinkwatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ImageBitmap: Change two enum uses. An earlier CL (https://codereview.chromium.org/1382883002/) made two changes which seem to have unintended effects on behaviour. 1. The ImageBitmap constructor asserts that its data is opaque (whereas previously it took the default NonOpaque opacity mode), but if this is so it should have a comment justifying it. 2. UnacceleratedImageBufferSurface is clearings the SkSurface if it was told _not_ to initialize image pixels, which seems like an inversion of the intended condition. BUG=543515, 544691 Committed: https://crrev.com/1998d1cc5f2985cdeac94d3e669b0e7901a77bbb Cr-Commit-Position: refs/heads/master@{#354880}

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -2 lines) Patch
M third_party/WebKit/Source/core/frame/ImageBitmap.cpp View 1 chunk +1 line, -1 line 6 comments Download
M third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp View 1 chunk +1 line, -1 line 1 comment Download

Messages

Total messages: 18 (3 generated)
jbroman
This looks like a couple of simple mistakes. What do you think? I tried to ...
2 years, 8 months ago (2015-10-17 02:59:54 UTC) #2
xidachen
https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode101 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); This part is ...
2 years, 8 months ago (2015-10-18 14:08:48 UTC) #3
jbroman
https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode101 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); On 2015/10/18 at ...
2 years, 8 months ago (2015-10-18 15:44:14 UTC) #4
xidachen
I think it may need a bit more time to debug this issue. https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File ...
2 years, 8 months ago (2015-10-18 15:44:53 UTC) #5
jbroman
https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode101 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); On 2015/10/18 at ...
2 years, 8 months ago (2015-10-18 15:48:00 UTC) #6
xidachen
We can spend more time on the ImageData API, but I think we may need ...
2 years, 8 months ago (2015-10-18 15:55:56 UTC) #7
jbroman
Added references to bugs xidachen believes to be related.
2 years, 8 months ago (2015-10-19 18:55:24 UTC) #9
xidachen
lgtm Thanks for fixing it.
2 years, 8 months ago (2015-10-19 18:57:50 UTC) #10
Justin Novosad
lgtm https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode101 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:101: OwnPtr<ImageBuffer> buffer = ImageBuffer::create(data->size(), NonOpaque, DoNotInitializeImagePixels); I agree ...
2 years, 8 months ago (2015-10-19 19:13:20 UTC) #11
Justin Novosad
On 2015/10/18 14:08:48, xidachen wrote: > This part is tricky. It was set to be ...
2 years, 8 months ago (2015-10-19 19:15:36 UTC) #12
Justin Novosad
On 2015/10/18 15:44:14, jbroman wrote: > I'd like Justin's opinion, but it seems to me ...
2 years, 8 months ago (2015-10-19 19:16:28 UTC) #13
Justin Novosad
On 2015/10/18 15:48:00, jbroman wrote: > https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp > File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): > > https://codereview.chromium.org/1407393002/diff/1/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode101 > ...
2 years, 8 months ago (2015-10-19 19:17:15 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407393002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407393002/1
2 years, 8 months ago (2015-10-19 19:36:56 UTC) #16
commit-bot: I haz the power
Committed patchset #1 (id:1)
2 years, 8 months ago (2015-10-19 21:59:28 UTC) #17
commit-bot: I haz the power
2 years, 8 months ago (2015-10-19 22:00:34 UTC) #18
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/1998d1cc5f2985cdeac94d3e669b0e7901a77bbb
Cr-Commit-Position: refs/heads/master@{#354880}

Powered by Google App Engine
This is Rietveld 408576698