Bug 69365 – [WK2] WebKitTestRunner needs LayoutTestController.dumpConfigurationForViewport

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chang Shu]

The above api is required for passing fast/viewport tests.

[Chang Shu]

Created attachment 109809 [details]
patch 1

[Darin Adler]

Comment on attachment 109809 [details]
patch 1

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

> Source/WebKit2/WebProcess/WebPage/WebPage.h:437
> +    // For Testing purpose

This has grammar and formatting problems and should say:

    // For testing purposes.

Or:

    // For testing purposes:

> Source/WebKit2/WebProcess/WebPage/WebPage.h:441
> +    String viewportAsText(int deviceDPI, int deviceWidth, int deviceHeight, int availableWidth, int availableHeight);

This is not a good name for the function because it does not return “the viewport” it returns “viewport attributes”. So it should be named viewportAttributesAsText or viewportConfigurationAsText. Leaving out the AsText suffix might be OK too.

[Darin Adler]

A better fix might have been to migrate this test feature to internals.

[Chang Shu]

(In reply to comment #3)
> A better fix might have been to migrate this test feature to internals.

window.internal, right. It should be a good approach. We can probably take care of this later as it requires quite some changes in wk1 and layout tests themselves.
Thanks for the review!

[Chang Shu]

Created attachment 109820 [details]
patch 2: update per review

[WebKit Review Bot]

Comment on attachment 109820 [details]
patch 2: update per review

Clearing flags on attachment: 109820

Committed r96731: <http://trac.webkit.org/changeset/96731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.