Summary: | [EFL] LayoutTestController needs implementation of pageSizeAndMarginsInPixels | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Shalamov <alexander.shalamov> | ||||||||
Component: | WebKit EFL | Assignee: | Alexander Shalamov <alexander.shalamov> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 82591 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Alexander Shalamov
2012-03-29 02:47:05 PDT
Created attachment 134536 [details]
Patch
Created attachment 134540 [details]
Patch
DRT support for pageSizeAndMarginsInPixels
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 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. (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. (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 on attachment 134540 [details]
Patch
Removed review and commit-queue flags as requested.
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 on attachment 135289 [details]
Patch
It looks you don't add LayoutTests/platform/efl/Skipped.
(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 (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 on attachment 135289 [details]
Patch
LGTM
Comment on attachment 135289 [details] Patch Clearing flags on attachment: 135289 Committed r113011: <http://trac.webkit.org/changeset/113011> All reviewed patches have been landed. Closing bug. |