Bug 69365 - [WK2] WebKitTestRunner needs LayoutTestController.dumpConfigurationForViewport
Summary: [WK2] WebKitTestRunner needs LayoutTestController.dumpConfigurationForViewport
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-04 13:19 PDT by Chang Shu
Modified: 2011-10-05 11:20 PDT (History)
4 users (show)

See Also:


Attachments
patch 1 (9.89 KB, patch)
2011-10-05 09:10 PDT, Chang Shu
darin: review+
Details | Formatted Diff | Diff
patch 2: update per review (9.98 KB, patch)
2011-10-05 10:36 PDT, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2011-10-04 13:19:34 PDT
The above api is required for passing fast/viewport tests.
Comment 1 Chang Shu 2011-10-05 09:10:17 PDT
Created attachment 109809 [details]
patch 1
Comment 2 Darin Adler 2011-10-05 09:30:08 PDT
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.
Comment 3 Darin Adler 2011-10-05 09:30:33 PDT
A better fix might have been to migrate this test feature to internals.
Comment 4 Chang Shu 2011-10-05 09:53:38 PDT
(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!
Comment 5 Chang Shu 2011-10-05 10:36:12 PDT
Created attachment 109820 [details]
patch 2: update per review
Comment 6 WebKit Review Bot 2011-10-05 11:20:15 PDT
Comment on attachment 109820 [details]
patch 2: update per review

Clearing flags on attachment: 109820

Committed r96731: <http://trac.webkit.org/changeset/96731>
Comment 7 WebKit Review Bot 2011-10-05 11:20:19 PDT
All reviewed patches have been landed.  Closing bug.