Bug 82589 - [EFL] LayoutTestController needs implementation of pageSizeAndMarginsInPixels
Summary: [EFL] LayoutTestController needs implementation of pageSizeAndMarginsInPixels
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexander Shalamov
URL:
Keywords:
Depends on: 82591
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-29 02:47 PDT by Alexander Shalamov
Modified: 2012-08-14 05:41 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.86 KB, text/plain)
2012-03-29 02:59 PDT, Alexander Shalamov
no flags Details
Patch (5.86 KB, patch)
2012-03-29 03:21 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (4.87 KB, patch)
2012-04-03 01:23 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Shalamov 2012-03-29 02:47:05 PDT
LayoutTestController needs implementation of pageSizeAndMarginsInPixels in order to unskip printing/page-format-data.html
Comment 1 Alexander Shalamov 2012-03-29 02:59:53 PDT
Created attachment 134536 [details]
Patch
Comment 2 Alexander Shalamov 2012-03-29 03:21:46 PDT
Created attachment 134540 [details]
Patch

DRT support for pageSizeAndMarginsInPixels
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-03-29 13:13:10 PDT
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.
Comment 4 Gyuyoung Kim 2012-03-29 18:04:54 PDT
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.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-03-29 18:28:58 PDT
(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.
Comment 6 Gyuyoung Kim 2012-03-29 18:34:15 PDT
(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.
Comment 7 Alexander Shalamov 2012-04-01 22:02:08 PDT
Comment on attachment 134540 [details]
Patch

Removed review and commit-queue flags as requested.
Comment 8 Alexander Shalamov 2012-04-03 01:23:44 PDT
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.
Comment 9 Gyuyoung Kim 2012-04-03 04:01:10 PDT
Comment on attachment 135289 [details]
Patch

It looks you don't add LayoutTests/platform/efl/Skipped.
Comment 10 Alexander Shalamov 2012-04-03 04:04:24 PDT
(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
Comment 11 Gyuyoung Kim 2012-04-03 04:12:46 PDT
(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.
Comment 12 Philippe Normand 2012-04-03 04:28:44 PDT
Comment on attachment 135289 [details]
Patch

LGTM
Comment 13 WebKit Review Bot 2012-04-03 05:06:03 PDT
Comment on attachment 135289 [details]
Patch

Clearing flags on attachment: 135289

Committed r113011: <http://trac.webkit.org/changeset/113011>
Comment 14 WebKit Review Bot 2012-04-03 05:06:08 PDT
All reviewed patches have been landed.  Closing bug.