Bug 97940 – r129934 lacks png baselines for the new test

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ojan Vafai]

It was incorrectly marked as "Failure", which doesn't include "Missing". Marked as Missing: http://trac.webkit.org/changeset/129951.

Also, is it possible to rewrite this test to not have any whitespace between elements? I was going to just rebaseline the test, but then I noticed the whitespace makes the rendertree dump and the pixel dump different on every platform (because text is rendered a different size).

[Ojan Vafai]

webkit.org/b/97013 fast/canvas/canvas-render-layer.html [ Missing ]
webkit.org/b/97013 platform/chromium/virtual/gpu/fast/canvas/canvas-render-layer.html [ Missing ]

[Brian Salomon]

Created attachment 166466 [details]
Patch

[Brian Salomon]

Created attachment 166468 [details]
Patch

[Brian Salomon]

The patched version of the test generates this on my Linux box for the Chromium port:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x121
  RenderBlock {HTML} at (0,0) size 800x121
    RenderBody {BODY} at (8,8) size 784x105
      RenderHTMLCanvas {CANVAS} at (0,0) size 100x100
      RenderHTMLCanvas {CANVAS} at (100,0) size 100x100
      RenderHTMLCanvas {CANVAS} at (200,0) size 100x100
      RenderHTMLCanvas {CANVAS} at (300,0) size 100x100
      RenderText {#text} at (0,0) size 0x0
      RenderText {#text} at (0,0) size 0x0

[WebKit Review Bot]

Comment on attachment 166468 [details]
Patch

Attachment 166468 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14081892

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-render-layer.html
fast/canvas/canvas-render-layer.html

[Ojan Vafai]

Comment on attachment 166468 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166468&action=review

The change looks good. Can you include the new expected results for whichever platform you're running on?

> LayoutTests/ChangeLog:8
> +        This eliminates some space characters from the test output.

Can you give a bit more detail here? Specifically, saying that this is so the expected result files won't depend on text rendering layout.

[Brian Salomon]

Created attachment 166498 [details]
Patch

[Brian Salomon]

(In reply to comment #6)
> (From update of attachment 166468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166468&action=review
> 
> The change looks good. Can you include the new expected results for whichever platform you're running on?

Done, the Cr-Linux txt and png files are in the most recent patch.

> 
> > LayoutTests/ChangeLog:8
> > +        This eliminates some space characters from the test output.
> 
> Can you give a bit more detail here? Specifically, saying that this is so the expected result files won't depend on text rendering layout.

Done.

[Ojan Vafai]

Comment on attachment 166498 [details]
Patch

Looks great. Thanks for fixing this!

[WebKit Review Bot]

Comment on attachment 166498 [details]
Patch

Clearing flags on attachment: 166498

Committed r130056: <http://trac.webkit.org/changeset/130056>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

Added mac expectation in http://trac.webkit.org/changeset/130067 and 130068.

[Ojan Vafai]

Unfortunately, it turns out getting rid of all the whitespace text nodes wasn't sufficient to make the rendering the same across ports. Setting a line-height:0 on the body element does though.

http://trac.webkit.org/changeset/130080

[Jon Lee]

I get those same results on mac. Can we move the expected results out of platform/?

[Jon Lee]

Rebaseline for Mac: http://trac.webkit.org/changeset/130087

[Ojan Vafai]

(In reply to comment #14)
> I get those same results on mac. Can we move the expected results out of platform/?

In theory, webkit-patch rebaseline should automatically do this once all the bots have run because it calls optimize-baselines.

[Roger Fong]

(In reply to comment #4)
> The patched version of the test generates this on my Linux box for the Chromium port:
> 
> layer at (0,0) size 800x600
>   RenderView at (0,0) size 800x600
> layer at (0,0) size 800x121
>   RenderBlock {HTML} at (0,0) size 800x121
>     RenderBody {BODY} at (8,8) size 784x105
>       RenderHTMLCanvas {CANVAS} at (0,0) size 100x100
>       RenderHTMLCanvas {CANVAS} at (100,0) size 100x100
>       RenderHTMLCanvas {CANVAS} at (200,0) size 100x100
>       RenderHTMLCanvas {CANVAS} at (300,0) size 100x100
>       RenderText {#text} at (0,0) size 0x0
>       RenderText {#text} at (0,0) size 0x0

Hi Brian, I'm getting the same results on the Apple-Windows port. Do you think its safe for me to just add Windows specific results to match the current output?

Thanks

[Brian Salomon]

(In reply to comment #17)
> (In reply to comment #4)
> > The patched version of the test generates this on my Linux box for the Chromium port:
> > 
> > layer at (0,0) size 800x600
> >   RenderView at (0,0) size 800x600
> > layer at (0,0) size 800x121
> >   RenderBlock {HTML} at (0,0) size 800x121
> >     RenderBody {BODY} at (8,8) size 784x105
> >       RenderHTMLCanvas {CANVAS} at (0,0) size 100x100
> >       RenderHTMLCanvas {CANVAS} at (100,0) size 100x100
> >       RenderHTMLCanvas {CANVAS} at (200,0) size 100x100
> >       RenderHTMLCanvas {CANVAS} at (300,0) size 100x100
> >       RenderText {#text} at (0,0) size 0x0
> >       RenderText {#text} at (0,0) size 0x0
> 
> Hi Brian, I'm getting the same results on the Apple-Windows port. Do you think its safe for me to just add Windows specific results to match the current output?
> 
> Thanks

Hi Roger, Ojan changed the test again to remove another source of text rendering dependence: http://trac.webkit.org/changeset/130087. So you may want to rerun after that change. I'm not much of an expert on how the expectations are organized in the tree but comments #15 and #16 seem to indicate that we can share expectations across all the ports/platforms.

[Roger Fong]

> Hi Roger, Ojan changed the test again to remove another source of text rendering dependence: http://trac.webkit.org/changeset/130087. So you may want to rerun after that change. I'm not much of an expert on how the expectations are organized in the tree but comments #15 and #16 seem to indicate that we can share expectations across all the ports/platforms.

Well currently there are already expected results for this test for Mac and Windows by default uses Mac results if Windows specific results do not exist. However, it looks like the result on Windows and Mac is actually different.

The current mac result looks like:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x120
  RenderBlock {HTML} at (0,0) size 800x120
    RenderBody {BODY} at (8,8) size 784x104
      RenderHTMLCanvas {CANVAS} at (0,0) size 100x100
      RenderText {#text} at (0,0) size 0x0
      RenderText {#text} at (0,0) size 0x0
layer at (108,8) size 100x100
  RenderHTMLCanvas {CANVAS} at (100,0) size 100x100
layer at (208,8) size 100x100
  RenderHTMLCanvas {CANVAS} at (200,0) size 100x100
layer at (308,8) size 100x100
  RenderHTMLCanvas {CANVAS} at (300,0) size 100x100

which looks nothing like what we're seeing on the Win and Chromium Linux ports.

I'm not sure if the results on mac are actually correct because I can find any rationale for why the mac rebaselining was the right thing to do.

Windows has a tendency to not match mac (I think it's rather silly that Windows defaults to Mac results actually) which makes me want to say that I should just make Windows specific results for this test.

[Brian Salomon]

(In reply to comment #19)
> > Hi Roger, Ojan changed the test again to remove another source of text rendering dependence: http://trac.webkit.org/changeset/130087. So you may want to rerun after that change. I'm not much of an expert on how the expectations are organized in the tree but comments #15 and #16 seem to indicate that we can share expectations across all the ports/platforms.
> 
> Well currently there are already expected results for this test for Mac and Windows by default uses Mac results if Windows specific results do not exist. However, it looks like the result on Windows and Mac is actually different.
> 
> The current mac result looks like:
> 
> layer at (0,0) size 800x600
>   RenderView at (0,0) size 800x600
> layer at (0,0) size 800x120
>   RenderBlock {HTML} at (0,0) size 800x120
>     RenderBody {BODY} at (8,8) size 784x104
>       RenderHTMLCanvas {CANVAS} at (0,0) size 100x100
>       RenderText {#text} at (0,0) size 0x0
>       RenderText {#text} at (0,0) size 0x0
> layer at (108,8) size 100x100
>   RenderHTMLCanvas {CANVAS} at (100,0) size 100x100
> layer at (208,8) size 100x100
>   RenderHTMLCanvas {CANVAS} at (200,0) size 100x100
> layer at (308,8) size 100x100
>   RenderHTMLCanvas {CANVAS} at (300,0) size 100x100
> 
> which looks nothing like what we're seeing on the Win and Chromium Linux ports.
> 
> I'm not sure if the results on mac are actually correct because I can find any rationale for why the mac rebaselining was the right thing to do.
> 
> Windows has a tendency to not match mac (I think it's rather silly that Windows defaults to Mac results actually) which makes me want to say that I should just make Windows specific results for this test.

Chromium actually generates two sets of results, one with GPU-accelerated canvas 2D and one without. The difference is that GPU acceleration should stick the latter three canvases in their own layers. This is actually what the test was designed to look for. The change that added the test, r129934, also fixed a bug where accelerated canvas elements were not correctly getting promoted to layers. The result I originally pasted in comment #4 was the unaccelerated version of cr-linux. I suspect your Mac run is using accelerated canvas2d and your Windows run is not.

[Brian Salomon]

Here is the current Chromium expectation when acceleration is enabled:

http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/platform/chromium/virtual/gpu/fast/canvas/canvas-render-layer-expected.txt

It doesn't exactly match your Mac result but is similar.

[Roger Fong]

Ah there is no such acceleration implemented on Windows...so that makes sense why I'm seeing those results.
I'll just make the results then.
Thanks!