Bug 67723 – Fixed Layout Mode should be adjustable from layoutTestController for testing on Chromium platforms

• Component: New Bugs
• Status: RESOLVED FIXED

[Fady Samuel]

Fixed Layout Mode should be adjustable from eventSender for testing on Chromium platforms

[Fady Samuel]

Created attachment 106614 [details]
Patch

[Adam Barth]

This patch looks good, but the API should probably be on layoutTestController.  EventSender is really more for sending events.

[Fady Samuel]

Created attachment 106637 [details]
Patch

[Adam Barth]

Comment on attachment 106637 [details]
Patch

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

Why do you have two copies of fixed-layout-360x240-expected.png ?  They have the same sha1.  Presumably we can remove LayoutTests/platform/chromium-linux/fast/repaint/fixed-layout-360x240-expected.png.

> LayoutTests/fast/repaint/fixed-layout-360x240.html:8
> +            window.layoutTestController.enableFixedLayoutMode(true);
> +            window.layoutTestController.setFixedLayoutSize(360, 240);

Does this test need to be skipped on non-chromium platforms?

[Fady Samuel]

Created attachment 106642 [details]
Wrong bug: please ignore.

[Fady Samuel]

(In reply to comment #4)
> (From update of attachment 106637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106637&action=review
> 
> Why do you have two copies of fixed-layout-360x240-expected.png ?  They have the same sha1.  Presumably we can remove LayoutTests/platform/chromium-linux/fast/repaint/fixed-layout-360x240-expected.png.
> 
> > LayoutTests/fast/repaint/fixed-layout-360x240.html:8
> > +            window.layoutTestController.enableFixedLayoutMode(true);
> > +            window.layoutTestController.setFixedLayoutSize(360, 240);
> 
> Does this test need to be skipped on non-chromium platforms?

It does, I'm not sure how to do that? Do I just include a different layout test result as the default and another under the chromium directory?

[Adam Barth]

> It does, I'm not sure how to do that? Do I just include a different layout test result as the default and another under the chromium directory?

In this case, because this test is testing a Chromium WebKit API, I would put the test in the LayoutTest/platform/chromium/fast/repaint/fixed-layout-360x240.html

That will cause the test to only be run on Chromium.  Another approach is to add the test to all the other port's Skipped files, which would be appropriate if this was a test for a feature we'd expect other ports to implement eventually.

[Fady Samuel]

Created attachment 106662 [details]
Patch

[Fady Samuel]

Done. I opted for the former. 

(In reply to comment #7)
> > It does, I'm not sure how to do that? Do I just include a different layout test result as the default and another under the chromium directory?
> 
> In this case, because this test is testing a Chromium WebKit API, I would put the test in the LayoutTest/platform/chromium/fast/repaint/fixed-layout-360x240.html
> 
> That will cause the test to only be run on Chromium.  Another approach is to add the test to all the other port's Skipped files, which would be appropriate if this was a test for a feature we'd expect other ports to implement eventually.

[WebKit Review Bot]

Comment on attachment 106662 [details]
Patch

Clearing flags on attachment: 106662

Committed r94779: <http://trac.webkit.org/changeset/94779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.