Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
758493004:
canvas: make a temporary buffer when a context doesn't exist. (Closed)
|
Created:
6 years ago by dshwang Modified:
6 years ago CC:
blink-reviews, blink-reviews-css, blink-reviews-html_chromium.org, ed+blinkwatch_opera.com, dglazkov+blink, Rik, apavlov+blink_chromium.org, groby+blinkspell_chromium.org, darktears, aandrey+blink_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Descriptioncanvas: make a temporary buffer when a context doesn't exist.
This CL doesn't make a m_imageBuffer when a context doesn't exist because
we don't know which context will be created.
Furthermore, this CL makes code more understandable, because no context means
no buffer from now on.
BUG=331181, 438240
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186402
Patch Set 1 #
Total comments: 10
Patch Set 2 : buffer() creates noContextImageBuffer when no context #
Total comments: 12
Patch Set 3 : Don't create temp imageBuffer again #
Total comments: 8
Patch Set 4 : rename to isPaintable() #
Total comments: 1
Messages
Total messages: 24 (4 generated)
dongseong.hwang@intel.com changed reviewers: + junov@chromium.org
dongseong.hwang@intel.com changed reviewers: + kbr@chromium.org
could you review? It's follow-up CL of https://codereview.chromium.org/750273003
https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:676: ASSERT(m_context); It means that we can move imageBuffer to CanvasRenderingContext after this CL.
https://codereview.chromium.org/758493004/diff/1/Source/core/editing/Editor.cpp File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/758493004/diff/1/Source/core/editing/Editor.c... Source/core/editing/Editor.cpp:424: static PassRefPtr<Image> imageFromNode(const Node& node) Can you explain why this needs to be a RefPtr now? https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:359: if (!canCreateImageBuffer()) { I disagree with this part. We should not create an image buffer if we don't need to. Having no image buffer happens in cases where a context has not been drawn to since it was created or since it was last resized. We also want to prevent the immediate creation of an image buffer in cases where Chrome is in the process of recovering from a lost GPU context (GPU process crash, GPU driver crash). Also, canCreateImageBuffer() does not guarantee that buffer() will return a valid image buffer. https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:778: return buffer()->copyImage(DontCopyBackingStore, Unscaled); Here: not safe to assume buffer is non-null https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:781: RefPtr<SkImage> image = buffer()->newImageSnapshot(); here too https://codereview.chromium.org/758493004/diff/1/Source/core/imagebitmap/Imag... File Source/core/imagebitmap/ImageBitmapFactories.cpp (right): https://codereview.chromium.org/758493004/diff/1/Source/core/imagebitmap/Imag... Source/core/imagebitmap/ImageBitmapFactories.cpp:181: RefPtr<Image> canvasImage = canvas->copiedImage(BackBuffer); This seems like a step in the wrong direction. We want to move toward making this asynchronous (see comment above). Requesting a resolved Image here is the opposite of that. Let me explain. If the canvas is stored as a display list or use deferred rendering (via Canvas2DLayerBridge), the objective would be to return immediately with an unfulfilled Promise object.
Thank you for good review with great explanation. In the 1st patch set, I changed all API not use buffer() if context don't exist. In the 2nd patch set, I change buffer() itself. Now buffer() returns ImageBuffer as-is no matter what context. However, buffer() creates temporary imagebuffer if context don't exist. I didn't reuse existing m_imageBuffer for temporary imagebuffer because it makes code very difficult to read. https://codereview.chromium.org/758493004/diff/1/Source/core/editing/Editor.cpp File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/758493004/diff/1/Source/core/editing/Editor.c... Source/core/editing/Editor.cpp:424: static PassRefPtr<Image> imageFromNode(const Node& node) On 2014/11/27 00:49:30, junov wrote: > Can you explain why this needs to be a RefPtr now? Next patch removes it. I changed HTMLCanvasElement::copiedImage return RefPtr, so it was needed not to remove the object in this scope. https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:359: if (!canCreateImageBuffer()) { On 2014/11/27 00:49:31, junov wrote: > I disagree with this part. We should not create an image buffer if we don't need > to. Having no image buffer happens in cases where a context has not been drawn > to since it was created or since it was last resized. We also want to prevent > the immediate creation of an image buffer in cases where Chrome is in the > process of recovering from a lost GPU context (GPU process crash, GPU driver > crash). Also, canCreateImageBuffer() does not guarantee that buffer() will > return a valid image buffer. Thank you for explaining. I rollback this code in next patch set https://codereview.chromium.org/758493004/diff/1/Source/core/html/HTMLCanvasE... Source/core/html/HTMLCanvasElement.cpp:778: return buffer()->copyImage(DontCopyBackingStore, Unscaled); On 2014/11/27 00:49:31, junov wrote: > Here: not safe to assume buffer is non-null Yes, true In next patch set, above "if statement" for buffer() is preserved. https://codereview.chromium.org/758493004/diff/1/Source/core/imagebitmap/Imag... File Source/core/imagebitmap/ImageBitmapFactories.cpp (right): https://codereview.chromium.org/758493004/diff/1/Source/core/imagebitmap/Imag... Source/core/imagebitmap/ImageBitmapFactories.cpp:181: RefPtr<Image> canvasImage = canvas->copiedImage(BackBuffer); On 2014/11/27 00:49:31, junov wrote: > This seems like a step in the wrong direction. We want to move toward making > this asynchronous (see comment above). Requesting a resolved Image here is the > opposite of that. Let me explain. If the canvas is stored as a display list or > use deferred rendering (via Canvas2DLayerBridge), the objective would be to > return immediately with an unfulfilled Promise object. Thank you for explaining. I rollback this change. https://codereview.chromium.org/758493004/diff/20001/Source/core/css/CSSCanva... File Source/core/css/CSSCanvasValue.cpp (right): https://codereview.chromium.org/758493004/diff/20001/Source/core/css/CSSCanva... Source/core/css/CSSCanvasValue.cpp:92: if (!elt) I remove all buffer() usages from other modules Now HTMLCanvasElement::buffer() is called by only canvas code. It makes refactorying code about imagebuffer easier.
https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:666: m_noContextImageBuffer = ImageBuffer::create(size(), NonOpaque); Please explain how this code helps. It seems wasteful to me to allocate a persistent buffer in cases where we know the content is blank. Why is this better than just returning null? The calling code already needs to be able to handle a null return value since createImageBuffer() is not guaranteed to succeed. https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:691: if (!m_copiedImage && buffer()) { Not new in this patch, but there is something wrong here: if m_copiedImage is non-null, how do we know that is came from the right sourceBuffer? I think this is a mistake that was not caught in the code review https://codereview.chromium.org/749653002. Could you fix that in a separate patch? https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:764: return buffer()->copyImage(DontCopyBackingStore, Unscaled); I think this CL is a good opportunity to fix something else that bugs me. I find the function name "buffer" could be improved. It looks like an accessor but it is actually more than that. In my opinion, buffer() and createImageBuffer() should be merged into a single function called getOrCreateImageBuffer(), and there should be a plain accessor called getImageBuffer() which does not attempts to create.
https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:666: m_noContextImageBuffer = ImageBuffer::create(size(), NonOpaque); Ironically, the title of this change is "Don't make a buffer when a context doesn't exist", which is a good idea, but you are doing the opposite right here. ;-)
Thank you for great review! https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:666: m_noContextImageBuffer = ImageBuffer::create(size(), NonOpaque); On 2014/11/27 18:58:35, junov wrote: > Please explain how this code helps. It seems wasteful to me to allocate a > persistent buffer in cases where we know the content is blank. Why is this > better than just returning null? The calling code already needs to be able to > handle a null return value since createImageBuffer() is not guaranteed to > succeed. That's good question. Two reason: 1. this method return pointer. ImageBuffer cannot be wrapped by RefPtr 2. All callers except for CanvasRenderingContext expects this method to return valid buffer if size is acceptable. For example, HTMLCanvasElement::paint, HTMLCanvasElement::toDataURL, HTMLCanvasElement::copiedImage, HTMLCanvasElement::getSourceImageForCanvas, WebGLRenderingContextBase::texImage2D, WebGLRenderingContextBase::texSubImage2D Those API want to use transparent imageBuffer when context don't exist. So if returning nullptr here, so many layout tests fail. The first patch set fix it as those API don't call buffer(), but the problem is that we don't know if buffer() is really null until we actually call buffer() as you mentioned. https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:691: if (!m_copiedImage && buffer()) { On 2014/11/27 18:58:35, junov wrote: > Not new in this patch, but there is something wrong here: if m_copiedImage is > non-null, how do we know that is came from the right sourceBuffer? I think this > is a mistake that was not caught in the code review > https://codereview.chromium.org/749653002. Could you fix that in a separate > patch? Good point. I'll fix it in the separate CL. Thank you. It needs to create new member, m_copiedBackBufferImage, which is not related to canvas 2d. It's why I want to move imageBuffer to each CanvasRenderingContext. https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:764: return buffer()->copyImage(DontCopyBackingStore, Unscaled); On 2014/11/27 18:58:35, junov wrote: > I think this CL is a good opportunity to fix something else that bugs me. I find > the function name "buffer" could be improved. It looks like an accessor but it > is actually more than that. In my opinion, buffer() and createImageBuffer() > should be merged into a single function called getOrCreateImageBuffer(), and > there should be a plain accessor called getImageBuffer() which does not attempts > to create. That's good idea, but many const methods call buffer() currently. I want to do it in the next CL.
https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:666: m_noContextImageBuffer = ImageBuffer::create(size(), NonOpaque); On 2014/11/27 19:34:28, dshwang wrote: > 2. All callers except for CanvasRenderingContext expects this method to return > valid buffer if size is acceptable. That is a problem. We cannot guarantee that this method will return a non-null pointer. The call to ImageBuffer::create will return a null pointer if it is unable to allocate a buffer of the requested size. https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:764: return buffer()->copyImage(DontCopyBackingStore, Unscaled); On 2014/11/27 19:34:28, dshwang wrote: > On 2014/11/27 18:58:35, junov wrote: > > I think this CL is a good opportunity to fix something else that bugs me. I > find > > the function name "buffer" could be improved. It looks like an accessor but it > > is actually more than that. In my opinion, buffer() and createImageBuffer() > > should be merged into a single function called getOrCreateImageBuffer(), and > > there should be a plain accessor called getImageBuffer() which does not > attempts > > to create. > > That's good idea, but many const methods call buffer() currently. > I want to do it in the next CL. Acknowledged.
https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:666: m_noContextImageBuffer = ImageBuffer::create(size(), NonOpaque); On 2014/11/27 20:18:15, junov wrote: > On 2014/11/27 19:34:28, dshwang wrote: > > > 2. All callers except for CanvasRenderingContext expects this method to return > > valid buffer if size is acceptable. > > That is a problem. We cannot guarantee that this method will return a non-null > pointer. The call to ImageBuffer::create will return a null pointer if it is > unable to allocate a buffer of the requested size. That's true, but in that sense, most of layout tests fail anytime. Let me explain. All layout tests expects that ImageBuffer::create success with normal size. For example, var canvas = doc.getElementById("canvas"); var context = canvas.getContext("2d"); var canvas2 = doc.getElementById("canvas2"); canvas2.width = 10; canvas2.height = 10; // Following 3 lines expect that canvas2 success to create image buffer, no matter whether canvas2 has a context or not. context.drawImage(canvas2); canvas2.toDataURL(); webGL.texImage2D(..., canvas2); If ImageBuffer::Create of canvas2 fails anytime, we cannot rely on layout test. If we return nullptr when no context, above kind of tests fail. The failure of buffer() with normal size is very exceptional case, but we must handle as-is for release chrome browser.
https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:666: m_noContextImageBuffer = ImageBuffer::create(size(), NonOpaque); On 2014/11/27 20:42:17, dshwang wrote: > On 2014/11/27 20:18:15, junov wrote: > > On 2014/11/27 19:34:28, dshwang wrote: > > > > > 2. All callers except for CanvasRenderingContext expects this method to > return > > > valid buffer if size is acceptable. > > > > That is a problem. We cannot guarantee that this method will return a non-null > > pointer. The call to ImageBuffer::create will return a null pointer if it is > > unable to allocate a buffer of the requested size. > > That's true, but in that sense, most of layout tests fail anytime. > > Let me explain. > All layout tests expects that ImageBuffer::create success with normal size. > > For example, > var canvas = doc.getElementById("canvas"); > var context = canvas.getContext("2d"); > > var canvas2 = doc.getElementById("canvas2"); > canvas2.width = 10; > canvas2.height = 10; > // Following 3 lines expect that canvas2 success to create image buffer, no > matter whether canvas2 has a context or not. > context.drawImage(canvas2); > canvas2.toDataURL(); > webGL.texImage2D(..., canvas2); > > If ImageBuffer::Create of canvas2 fails anytime, we cannot rely on layout test. > If we return nullptr when no context, above kind of tests fail. > > The failure of buffer() with normal size is very exceptional case, but we must > handle as-is for release chrome browser. Let me explain more in the case of context.drawImage(canvas2) HTMLCanvasElement::getSourceImageForCanvas() is called for context.drawImage(canvas2). The patch set 1 deal with it as follows: if (!canCreateImageBuffer()) { *status = InvalidSourceImageStatus; return nullptr; } if (!m_context) { *status = NormalSourceImageStatus; return createTransparentImage(size()); } RefPtr<SkImage> image = buffer()->newImageSnapshot(); Above code is wrong as you mentioned "not safe to assume buffer is non-null" Patch set 2 deal with it as follows: if (!buffer()) { *status = InvalidSourceImageStatus; return nullptr; } RefPtr<SkImage> image = buffer()->newImageSnapshot(); If we want buffer() to return nullptr when no context, code need to bloat like if (!canCreateImageBuffer()) { *status = InvalidSourceImageStatus; return nullptr; } if (!m_context) { *status = NormalSourceImageStatus; return createTransparentImage(size()); } if (!buffer()) { *status = InvalidSourceImageStatus; return nullptr; } RefPtr<SkImage> image = buffer()->newImageSnapshot(); It's possible but all callers need double check for canCreateImageBuffer() and buffer(). What do you think?
On 2014/11/27 20:42:17, dshwang wrote: > https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... > File Source/core/html/HTMLCanvasElement.cpp (right): > > https://codereview.chromium.org/758493004/diff/20001/Source/core/html/HTMLCan... > Source/core/html/HTMLCanvasElement.cpp:666: m_noContextImageBuffer = > ImageBuffer::create(size(), NonOpaque); > On 2014/11/27 20:18:15, junov wrote: > > On 2014/11/27 19:34:28, dshwang wrote: > > > > > 2. All callers except for CanvasRenderingContext expects this method to > return > > > valid buffer if size is acceptable. > > > > That is a problem. We cannot guarantee that this method will return a non-null > > pointer. The call to ImageBuffer::create will return a null pointer if it is > > unable to allocate a buffer of the requested size. > > That's true, but in that sense, most of layout tests fail anytime. The concern is not about layout tests, it is about overall product stability, and security. In general, Chrome crashes with an out-of-memory exception when it runs out of RAM. By crashing immediately, Chrome is safe from from having its processes compromized by attackers looking to exploit security vulnerability introduced by unhandled allocation failures. However, because large canvas allocations are a frequent source of memory exhaustion, it was decided several years ago that canvas allocation failures should be handled more gracefully in order to improve product stability. So now, what happens when a pixel buffer allocation fails is that instead of crashing, the affected content is rendered as blank. This may or may not make the current page unusable, but at least it does not crash your other tabs that share the same render process. Allowing allocations to fail without crashing the entire process means that we have to be extremely careful about handling the failure correctly. Not doing so may create serious security vulnerabilities because user code can purposely cause the browser to use a bad pointer, which is something that attackers can potentially exploit for all sort of purposes like accessing arbitrary memory locations, and whatever that may allow a hacker to do.
On 2014/11/28 07:49:44, dshwang wrote:
> If we want buffer() to return nullptr when no context, code need to bloat like
> if (!canCreateImageBuffer()) {
> *status = InvalidSourceImageStatus;
> return nullptr;
> }
> if (!m_context) {
> *status = NormalSourceImageStatus;
> return createTransparentImage(size());
> }
> if (!buffer()) {
> *status = InvalidSourceImageStatus;
> return nullptr;
> }
> RefPtr<SkImage> image = buffer()->newImageSnapshot();
I think this would be fine:
if (!m_context) {
*status = NormalSourceImageStatus;
return createTransparentImage(size());
}
if (!buffer()) {
*status = InvalidSourceImageStatus;
return nullptr;
}
This is actually preferable because the transparent image, which contains no
useful information, will be released from memory as soon as the calling code is
done with it. Having large blank buffers persistently allocated in memory
should be avoided if possible.
On 2014/11/28 16:31:31, junov wrote:
> On 2014/11/28 07:49:44, dshwang wrote:
>
> > If we want buffer() to return nullptr when no context, code need to bloat
like
> > if (!canCreateImageBuffer()) {
> > *status = InvalidSourceImageStatus;
> > return nullptr;
> > }
> > if (!m_context) {
> > *status = NormalSourceImageStatus;
> > return createTransparentImage(size());
> > }
> > if (!buffer()) {
> > *status = InvalidSourceImageStatus;
> > return nullptr;
> > }
> > RefPtr<SkImage> image = buffer()->newImageSnapshot();
>
> I think this would be fine:
>
> if (!m_context) {
> *status = NormalSourceImageStatus;
> return createTransparentImage(size());
> }
> if (!buffer()) {
> *status = InvalidSourceImageStatus;
> return nullptr;
> }
>
> This is actually preferable because the transparent image, which contains no
> useful information, will be released from memory as soon as the calling code
is
> done with it. Having large blank buffers persistently allocated in memory
> should be avoided if possible.
I got it. Patch set 3 complied this comments. I introduce
HTMLCanvasElement::isDrawable to replace buffer() and reduce the code.
https://codereview.chromium.org/758493004/diff/40001/Source/core/editing/Edit...
File Source/core/editing/Editor.cpp (right):
https://codereview.chromium.org/758493004/diff/40001/Source/core/editing/Edit...
Source/core/editing/Editor.cpp:432: return
toHTMLCanvasElement(node).copiedImage(FrontBuffer);
This line return PassRefPtr<Image> so this function must return PassRefPtr not
to return dangling pointer.
In addition, below cachedImage->imageForRenderer() returns PassRefPtr also.
https://codereview.chromium.org/758493004/diff/40001/Source/core/frame/ImageB...
File Source/core/frame/ImageBitmap.cpp (left):
https://codereview.chromium.org/758493004/diff/40001/Source/core/frame/ImageB...
Source/core/frame/ImageBitmap.cpp:89: m_bitmap =
cropImage(canvas->buffer()->copyImage(CopyBackingStore).get(), cropRect);
After this CL, other classes don't use buffer() except for
CanvasRenderingContext
https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):
https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:225: }
Introduce this class to replace buffer() in some code.
This method doesn't create imageBuffer if a context doesn't exist.
https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:771: if (!isDrawable()) {
for example of using isDrawable()
https://codereview.chromium.org/758493004/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):
https://codereview.chromium.org/758493004/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:3610: ImageBuffer* buffer
= canvas->buffer();
Above change is needed because this line calls buffer().
lgtm, but I would like kbr to also take a look. In particular the parts that touch WebGL. https://codereview.chromium.org/758493004/diff/40001/Source/core/editing/Edit... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/758493004/diff/40001/Source/core/editing/Edit... Source/core/editing/Editor.cpp:432: return toHTMLCanvasElement(node).copiedImage(FrontBuffer); On 2014/12/01 14:31:12, dshwang wrote: > This line return PassRefPtr<Image> so this function must return PassRefPtr not > to return dangling pointer. > In addition, below cachedImage->imageForRenderer() returns PassRefPtr also. I was thinking that this could be done in an oilpan friendly way by using PassRefPtrWillBeRawPtr. Then, I did some digging in the code and realize that such a change would propagate very deep and should probably go into a separate CL. So I am fine with this for now. https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan... File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:220: bool HTMLCanvasElement::isDrawable() const I think isPaintable would be more consistent with Blink vocabulary. https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan... Source/core/html/HTMLCanvasElement.cpp:375: return; I think this is going to be a problem problem with the new CSS blending and compositing feature. Some blend modes are supposed to have an effect even when the source is completely transparent. The spec is quite clear that canvases are supposed present a transparent bitmap when they have no context. We are currently optimizing that out because painting a transparent bitmap had no visible consequence in the past. I created this bug about it: crbug.com/438240
On 2014/12/02 15:48:16, junov wrote: > lgtm, but I would like kbr to also take a look. In particular the parts that > touch WebGL. Thank you for review! @kbr, could you review WebGL change? > https://codereview.chromium.org/758493004/diff/40001/Source/core/editing/Edit... > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/758493004/diff/40001/Source/core/editing/Edit... > Source/core/editing/Editor.cpp:432: return > toHTMLCanvasElement(node).copiedImage(FrontBuffer); > On 2014/12/01 14:31:12, dshwang wrote: > > This line return PassRefPtr<Image> so this function must return PassRefPtr not > > to return dangling pointer. > > In addition, below cachedImage->imageForRenderer() returns PassRefPtr also. > > I was thinking that this could be done in an oilpan friendly way by using > PassRefPtrWillBeRawPtr. Then, I did some digging in the code and realize that > such a change would propagate very deep and should probably go into a separate > CL. So I am fine with this for now. Yes, I also considered PassRefPtrWillBeRawPtr but strangely all RefPtr<Image> code doesn't use PassRefPtrWillBeRawPtr for some reason. I guess olipan team will deal with it when they deal with all RefPtr<Image> code. > https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan... > File Source/core/html/HTMLCanvasElement.cpp (right): > > https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan... > Source/core/html/HTMLCanvasElement.cpp:220: bool HTMLCanvasElement::isDrawable() > const > I think isPaintable would be more consistent with Blink vocabulary. That's right! Done. > https://codereview.chromium.org/758493004/diff/40001/Source/core/html/HTMLCan... > Source/core/html/HTMLCanvasElement.cpp:375: return; > I think this is going to be a problem problem with the new CSS blending and > compositing feature. Some blend modes are supposed to have an effect even when > the source is completely transparent. The spec is quite clear that canvases are > supposed present a transparent bitmap when they have no context. We are > currently optimizing that out because painting a transparent bitmap had no > visible consequence in the past. I created this bug about it: crbug.com/438240 I added the comment to refer to crbug.com/438240
Patchset #4 (id:60001) has been deleted
WebGL changes LGTM. https://codereview.chromium.org/758493004/diff/80001/Source/core/html/canvas/... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/758493004/diff/80001/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContextBase.cpp:3627: texImage2DImpl(target, level, internalformat, format, type, canvas->copiedImage(BackBuffer).get(), WebGLImageConversion::HtmlDomCanvas, m_unpackFlipY, m_unpackPremultiplyAlpha, exceptionState); In general a returned PassRefPtr is supposed to be put into a RefPtr before doing any work with it, to avoid the second and subsequent calls to get() returning NULL, but since this is a temporary it seems fine.
On 2014/12/03 02:59:54, Ken Russell wrote: > WebGL changes LGTM. Thank you for review! > > https://codereview.chromium.org/758493004/diff/80001/Source/core/html/canvas/... > File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): > > https://codereview.chromium.org/758493004/diff/80001/Source/core/html/canvas/... > Source/core/html/canvas/WebGLRenderingContextBase.cpp:3627: > texImage2DImpl(target, level, internalformat, format, type, > canvas->copiedImage(BackBuffer).get(), WebGLImageConversion::HtmlDomCanvas, > m_unpackFlipY, m_unpackPremultiplyAlpha, exceptionState); > In general a returned PassRefPtr is supposed to be put into a RefPtr before > doing any work with it, to avoid the second and subsequent calls to get() > returning NULL, but since this is a temporary it seems fine. Yes, I hesitated whether use temp RefPtr or not. :)
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/758493004/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=186402 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
