Bug 54921 – [Chromium] Issue 60965: Layout test fast/canvas/setWidthResetAfterForcedRender.html is failing on GPU path

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Naoki Takano]

[Chromium] Issue 60965:	Layout test fast/canvas/setWidthResetAfterForcedRender.html is failing on GPU path

[Naoki Takano]

Created attachment 83250 [details]
Patch

[Naoki Takano]

James,

Could you review my patch?

I guess 2D canvas acceleration doesn't need "requiresLayer()".
If it returns true, the layout is different from the non accelerated layout.

Thanks,

[Build Bot]

Attachment 83250 [details] did not build on win:
Build output: http://queues.webkit.org/results/7940821

[James Robinson]

Comment on attachment 83250 [details]
Patch

This is wrong, accelerated 2d canvases also require a layer. Why do you think this helps on that test?

[Early Warning System Bot]

Attachment 83250 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7938998

[Collabora GTK+ EWS bot]

Attachment 83250 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7943179

[Naoki Takano]

James,

Thank you for your review.

As you know, requiresLayer() function is called bye
RenderBoxModelObject::styleDidChange()
RenderObject::updateFillImages()

If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated.

For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.

I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer?

Still I'm wondering why the requiresLayer() can change rendering tree order.

Thanks,

(In reply to comment #4)
> (From update of attachment 83250 [details])
> This is wrong, accelerated 2d canvases also require a layer. Why do you think this helps on that test?

[Naoki Takano]

James,

Could you give me your thought?

Thanks,

(In reply to comment #7)
> James,
> 
> Thank you for your review.
> 
> As you know, requiresLayer() function is called bye
> RenderBoxModelObject::styleDidChange()
> RenderObject::updateFillImages()
> 
> If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated.
> 
> For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.
> 
> I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer?
> 
> Still I'm wondering why the requiresLayer() can change rendering tree order.
> 
> Thanks,
> 
> (In reply to comment #4)
> > (From update of attachment 83250 [details] [details])
> > This is wrong, accelerated 2d canvases also require a layer. Why do you think this helps on that test?

[James Robinson]

(In reply to comment #7)
> James,
> 
> Thank you for your review.
> 
> As you know, requiresLayer() function is called bye
> RenderBoxModelObject::styleDidChange()
> RenderObject::updateFillImages()
> 
> If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated.

Yes - this is expected.  An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer.  Different does not mean wrong.

> 
> For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.

I'm not sure what you mean by 'becomes the same layer of the root' - could you expand?

> 
> I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer?

The RenderLayer is required because accelerated canvas require their own composited layer.  See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check.

I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?

[Naoki Takano]

James,

Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-)

But thank you for your dedication every time.

(In reply to comment #9)
> (In reply to comment #7)
> > James,
> > 
> > Thank you for your review.
> > 
> > As you know, requiresLayer() function is called bye
> > RenderBoxModelObject::styleDidChange()
> > RenderObject::updateFillImages()
> > 
> > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated.
> 
> Yes - this is expected.  An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer.  Different does not mean wrong.
> 
> > 
> > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.
> 
> I'm not sure what you mean by 'becomes the same layer of the root' - could you expand?
> 
> > 
> > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer?
> 
> The RenderLayer is required because accelerated canvas require their own composited layer.  See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check.
> 
> I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?

[Naoki Takano]

James,

I confirmed Stephen's fix and it looks Ok with Chrome.

But it doesn't with DumpRenderTree.

As you know, we can run the test like following,
./webkit/tools/layout_tests/run_webkit_tests.sh  --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html

It fails as following,
         RenderText {#text} at (0,0) size 176x19
           text run at (0,0) width 176: "Actual output: (blank canvas)"
       RenderBlock (anonymous) at (0,152) size 784x55
-        RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
         RenderText {#text} at (0,0) size 0x0
         RenderText {#text} at (0,0) size 0x0
+layer at (8,168) size 100x50
+  RenderHTMLCanvas {CANVAS} at (0,0) size 100x50

As you see, rendering tree is different.

That's why I said
> > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.

If we have to fix completely, we have to consider DumpRenderTree part, right?

What do you think?

(In reply to comment #10)
> James,
> 
> Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-)
> 
> But thank you for your dedication every time.
> 
> (In reply to comment #9)
> > (In reply to comment #7)
> > > James,
> > > 
> > > Thank you for your review.
> > > 
> > > As you know, requiresLayer() function is called bye
> > > RenderBoxModelObject::styleDidChange()
> > > RenderObject::updateFillImages()
> > > 
> > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated.
> > 
> > Yes - this is expected.  An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer.  Different does not mean wrong.
> > 
> > > 
> > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.
> > 
> > I'm not sure what you mean by 'becomes the same layer of the root' - could you expand?
> > 
> > > 
> > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer?
> > 
> > The RenderLayer is required because accelerated canvas require their own composited layer.  See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check.
> > 
> > I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?

[Naoki Takano]

James,

Could you give me your comment?

Thanks,

(In reply to comment #11)
> James,
> 
> I confirmed Stephen's fix and it looks Ok with Chrome.
> 
> But it doesn't with DumpRenderTree.
> 
> As you know, we can run the test like following,
> ./webkit/tools/layout_tests/run_webkit_tests.sh  --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html
> 
> It fails as following,
>          RenderText {#text} at (0,0) size 176x19
>            text run at (0,0) width 176: "Actual output: (blank canvas)"
>        RenderBlock (anonymous) at (0,152) size 784x55
> -        RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
>          RenderText {#text} at (0,0) size 0x0
>          RenderText {#text} at (0,0) size 0x0
> +layer at (8,168) size 100x50
> +  RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
> 
> As you see, rendering tree is different.
> 
> That's why I said
> > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.
> 
> If we have to fix completely, we have to consider DumpRenderTree part, right?
> 
> What do you think?
> 
> (In reply to comment #10)
> > James,
> > 
> > Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-)
> > 
> > But thank you for your dedication every time.
> > 
> > (In reply to comment #9)
> > > (In reply to comment #7)
> > > > James,
> > > > 
> > > > Thank you for your review.
> > > > 
> > > > As you know, requiresLayer() function is called bye
> > > > RenderBoxModelObject::styleDidChange()
> > > > RenderObject::updateFillImages()
> > > > 
> > > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated.
> > > 
> > > Yes - this is expected.  An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer.  Different does not mean wrong.
> > > 
> > > > 
> > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.
> > > 
> > > I'm not sure what you mean by 'becomes the same layer of the root' - could you expand?
> > > 
> > > > 
> > > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer?
> > > 
> > > The RenderLayer is required because accelerated canvas require their own composited layer.  See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check.
> > > 
> > > I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?

[Naoki Takano]

James,

Could you give me your comment?

(In reply to comment #12)
> James,
> 
> Could you give me your comment?
> 
> Thanks,
> 
> (In reply to comment #11)
> > James,
> > 
> > I confirmed Stephen's fix and it looks Ok with Chrome.
> > 
> > But it doesn't with DumpRenderTree.
> > 
> > As you know, we can run the test like following,
> > ./webkit/tools/layout_tests/run_webkit_tests.sh  --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html
> > 
> > It fails as following,
> >          RenderText {#text} at (0,0) size 176x19
> >            text run at (0,0) width 176: "Actual output: (blank canvas)"
> >        RenderBlock (anonymous) at (0,152) size 784x55
> > -        RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
> >          RenderText {#text} at (0,0) size 0x0
> >          RenderText {#text} at (0,0) size 0x0
> > +layer at (8,168) size 100x50
> > +  RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
> > 
> > As you see, rendering tree is different.
> > 
> > That's why I said
> > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.
> > 
> > If we have to fix completely, we have to consider DumpRenderTree part, right?
> > 
> > What do you think?
> > 
> > (In reply to comment #10)
> > > James,
> > > 
> > > Thank you for your comment. I'll make sure Stephan's patch. Still I'm a beginner for WebKit, I have to study a lot... So I'm sorry if my patch is far from the correct fix;-)
> > > 
> > > But thank you for your dedication every time.
> > > 
> > > (In reply to comment #9)
> > > > (In reply to comment #7)
> > > > > James,
> > > > > 
> > > > > Thank you for your review.
> > > > > 
> > > > > As you know, requiresLayer() function is called bye
> > > > > RenderBoxModelObject::styleDidChange()
> > > > > RenderObject::updateFillImages()
> > > > > 
> > > > > If the returned values are true, the rendering tree becomes different from the non-accelerated rendering trees. The canvas layer order is different from non-accelerated.
> > > > 
> > > > Yes - this is expected.  An accelerated canvas requires a layer whereas a non-accelerated canvas may or may not need its own layer.  Different does not mean wrong.
> > > > 
> > > > > 
> > > > > For example, in setWidthResetAfterForce, the canvas element becomes the same layer of the root. That causes the test failure. That is why I change this part.
> > > > 
> > > > I'm not sure what you mean by 'becomes the same layer of the root' - could you expand?
> > > > 
> > > > > 
> > > > > I want to make sure the layer purpose. I thought this looked like the very special layer to overlay the rendered images in partial. Is this correct? Or is there another purpose for the layer?
> > > > 
> > > > The RenderLayer is required because accelerated canvas require their own composited layer.  See RenderLayerCompositor::requiresCompositingForCanvas() to see a similar check.
> > > > 
> > > > I'm pretty sure Stephen fixed this test with https://bugs.webkit.org/show_bug.cgi?id=54561 at http://trac.webkit.org/changeset/78922 - could you confirm if the test still fails after that revision?

[James Robinson]

(In reply to comment #11)
> James,
> 
> I confirmed Stephen's fix and it looks Ok with Chrome.
> 
> But it doesn't with DumpRenderTree.
> 
> As you know, we can run the test like following,
> ./webkit/tools/layout_tests/run_webkit_tests.sh  --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html
> 
> It fails as following,
>          RenderText {#text} at (0,0) size 176x19
>            text run at (0,0) width 176: "Actual output: (blank canvas)"
>        RenderBlock (anonymous) at (0,152) size 784x55
> -        RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
>          RenderText {#text} at (0,0) size 0x0
>          RenderText {#text} at (0,0) size 0x0
> +layer at (8,168) size 100x50
> +  RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
> 
> As you see, rendering tree is different.

Yes.  The new render tree is correct when accelerated canvas is enabled as I said in comment #9.

[Naoki Takano]

I see, so we have to change test patterns. Because even after the change, still the test says "Error" in setWidthResetAfterForcedRender.html.

(In reply to comment #14)
> (In reply to comment #11)
> > James,
> > 
> > I confirmed Stephen's fix and it looks Ok with Chrome.
> > 
> > But it doesn't with DumpRenderTree.
> > 
> > As you know, we can run the test like following,
> > ./webkit/tools/layout_tests/run_webkit_tests.sh  --accelerated-2d-canvas fast/canvas/setWidthResetAfterForcedRender.html
> > 
> > It fails as following,
> >          RenderText {#text} at (0,0) size 176x19
> >            text run at (0,0) width 176: "Actual output: (blank canvas)"
> >        RenderBlock (anonymous) at (0,152) size 784x55
> > -        RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
> >          RenderText {#text} at (0,0) size 0x0
> >          RenderText {#text} at (0,0) size 0x0
> > +layer at (8,168) size 100x50
> > +  RenderHTMLCanvas {CANVAS} at (0,0) size 100x50
> > 
> > As you see, rendering tree is different.
> 
> Yes.  The new render tree is correct when accelerated canvas is enabled as I said in comment #9.