Bug 82589 – [EFL] LayoutTestController needs implementation of pageSizeAndMarginsInPixels

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Alexander Shalamov]

LayoutTestController needs implementation of pageSizeAndMarginsInPixels in order to unskip printing/page-format-data.html

[Alexander Shalamov]

Created attachment 134536 [details]
Patch

[Alexander Shalamov]

Created attachment 134540 [details]
Patch

DRT support for pageSizeAndMarginsInPixels

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 134540 [details]
Patch

The changes to LTC and DRTSupportEfl look OK, but I see this test unskips the same ones unskipped in bug 82591, which at least means one of the patches will need to be rebased when the other lands. If they are both needed in order for these tests to be unskipped, please coordinate it better.

[Gyuyoung Kim]

Comment on attachment 134540 [details]
Patch

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

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:150
> +    String pageSizeAndMargins = DumpRenderTreeSupportEfl::pageSizeAndMarginsInPixels(browser->mainFrame(), pageNumber, width, height, marginTop, marginRight, marginBottom, marginLeft);

Should we use String class instead of char* in LTC ? I think we need to reduce util classes WebKit support. Because, the more we use WebKit class, the more we add visibility to eflsymbol.filter.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #4)
> (From update of attachment 134540 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134540&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:150
> > +    String pageSizeAndMargins = DumpRenderTreeSupportEfl::pageSizeAndMarginsInPixels(browser->mainFrame(), pageNumber, width, height, marginTop, marginRight, marginBottom, marginLeft);
> 
> Should we use String class instead of char* in LTC ? I think we need to reduce util classes WebKit support. Because, the more we use WebKit class, the more we add visibility to eflsymbol.filter.

I'm favor of using String in DRTSupportEfl for now; besides making the code shorter and more C++-y, it's not clear yet that adding things to eflsymbol.filter is the best approach, and even if we later realize it is, we're mostly calling the same String constructor in all these methods being added, so it would amount to one line being added to the filter file.

[Gyuyoung Kim]

(In reply to comment #5)

> I'm favor of using String in DRTSupportEfl for now; besides making the code shorter and more C++-y, it's not clear yet that adding things to eflsymbol.filter is the best approach, and even if we later realize it is, we're mostly calling the same String constructor in all these methods being added, so it would amount to one line being added to the filter file.

Because String is already being used by many codes, I'm ok. However, I think we need to sublate to use WebKit classes we don't use before. Of course, we can use WebKit class when there is no alternative.

[Alexander Shalamov]

Comment on attachment 134540 [details]
Patch

Removed review and commit-queue flags as requested.

[Alexander Shalamov]

Created attachment 135289 [details]
Patch

Removed modifications in LayoutTests/platform/efl/Skipped
Regarding comments related to usage of WTF::String, I just tried to be consistent with the rest of the EFL LayoutController / DumpRenderTreeSupportEfl code.

[Gyuyoung Kim]

Comment on attachment 135289 [details]
Patch

It looks you don't add LayoutTests/platform/efl/Skipped.

[Alexander Shalamov]

(In reply to comment #9)
> (From update of attachment 135289 [details])
> It looks you don't add LayoutTests/platform/efl/Skipped.

Test case was removed removed in https://bugs.webkit.org/show_bug.cgi?id=82591

[Gyuyoung Kim]

(In reply to comment #10)
> Test case was removed removed in https://bugs.webkit.org/show_bug.cgi?id=82591

Ok, Looks good to me now.

[Philippe Normand]

Comment on attachment 135289 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 135289 [details]
Patch

Clearing flags on attachment: 135289

Committed r113011: <http://trac.webkit.org/changeset/113011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.