Bug 73585 - [Chromium] Add a helper method to find whether an html page has custom page size styles.
Summary: [Chromium] Add a helper method to find whether an html page has custom page s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-01 13:13 PST by kmadhusu
Modified: 2011-12-02 14:07 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.93 KB, patch)
2011-12-01 13:13 PST, kmadhusu
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Patch (6.93 KB, patch)
2011-12-01 18:37 PST, kmadhusu
tkent: review-
Details | Formatted Diff | Diff
Latest patch with Changelog (9.11 KB, patch)
2011-12-02 09:34 PST, kmadhusu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description kmadhusu 2011-12-01 13:13:26 PST
Created attachment 117472 [details]
Patch

Actual bug: crbug.com/85132.

To support my changes to fix crbug.com/85132, I need to add a helper method to identify whether the page has custom page size style.
Comment 1 WebKit Review Bot 2011-12-01 13:23:03 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 2 Darin Adler 2011-12-01 15:21:25 PST
Comment on attachment 117472 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1698
> +    RefPtr<RenderStyle> style = styleForPage(pageIndex);
> +    return style->pageSizeType() != PAGE_SIZE_AUTO;

This function should not be added. You can just put all the code at at the call site in WebFrameImpl.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1481
> +    return frame()->document()->hasCustomPageSizeStyle(pageIndex);

return frame->document()->styleForPage(pageIndex)->pageSizeType() != PAGE_SIZE_AUTO;
Comment 3 Kent Tamura 2011-12-01 15:30:12 PST
Comment on attachment 117472 [details]
Patch

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

The patch contains no ChangeLog.

> LayoutTests/printing/page-format-data.html:62
> +        // Page size style tests.

Do you think layoutTestController.hasCustomPageSizeStyle() is Chromium-specific?
If so, we should make a new test in LayoutTests/chromium/printing/ directory.
Comment 4 Kent Tamura 2011-12-01 15:30:54 PST
(In reply to comment #3)
> (From update of attachment 117472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117472&action=review
> 
> The patch contains no ChangeLog.
> 
> > LayoutTests/printing/page-format-data.html:62
> > +        // Page size style tests.
> 
> Do you think layoutTestController.hasCustomPageSizeStyle() is Chromium-specific?
> If so, we should make a new test in LayoutTests/chromium/printing/ directory.

LayoutTests/platform/chromium/printing/
Comment 5 kmadhusu 2011-12-01 18:37:36 PST
Created attachment 117539 [details]
Patch

Addressed review comments and created a new test.
Comment 6 Kent Tamura 2011-12-01 19:03:23 PST
Comment on attachment 117539 [details]
Patch

Please add ChangeLog.
Comment 7 Darin Fisher (:fishd, Google) 2011-12-01 22:38:38 PST
Comment on attachment 117539 [details]
Patch

WebKit API changes LGTM
Comment 8 kmadhusu 2011-12-02 09:34:21 PST
Created attachment 117639 [details]
Latest patch with Changelog

ChangeLog added.

Thanks.
Comment 9 kmadhusu 2011-12-02 11:33:44 PST
fishd: Can you commit this patch for me?

Thanks.
Comment 10 Kent Tamura 2011-12-02 14:07:29 PST
Comment on attachment 117639 [details]
Latest patch with Changelog

Clearing flags on attachment: 117639

Committed r101851: <http://trac.webkit.org/changeset/101851>
Comment 11 Kent Tamura 2011-12-02 14:07:36 PST
All reviewed patches have been landed.  Closing bug.