[webkit-reviews] review granted: [Bug 45993] Convert printing to the new pagination model. : [Attachment 67955] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 45993: Convert printing to the new pagination model.
https://bugs.webkit.org/show_bug.cgi?id=45993

Attachment 67955: Patch
https://bugs.webkit.org/attachment.cgi?id=67955&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 67743)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,47 @@
> +2010-09-17  David Hyatt  <hyatt at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=45993, convert printing to
the new pagination model.

I'd like to see some discussion about what you did here.

> Index: WebCore/page/FrameView.h
> ===================================================================

> +    // FIXME: This method is retained because of embedded WebViews in
AppKit.  When a WebView is embedded inside
> +    // some enclosing view with auto-pagination, no call happens to resize
the view.  The new pagination model
> +    // needs the view to resize as a result of the breaks, but that means
that the enclosing view has to potentially
> +    // resize around that view.  Auto-pagination uses the bounds of the
actual view that's being printed to determine
> +    // the edges of the print operation, so the resize is necessary if the
enclosing view's bounds depend on the
> +    // web document's bounds.
> +    // 
> +    // This is already a problem if the view needs to be a different size
because of printer fonts or because of print stylesheets.
> +    // Mail/Dictionary work around this problem by using the
_layoutForPrinting SPI
> +    // to at least get print stylesheets and printer fonts into play, but
since WebKit doesn't know about the page offset or
> +    // page size, it can't actually paginate correctly during
_layoutForPrinting.
> +    //
> +    // We can eventually move Mail to a newer SPI that would let them opt in
to the layout-time pagination model,
> +    // but that doesn't solve the general problem of how other AppKit views
could opt in to the better model.
> +    //
> +    // NO OTHER PLATFORM BESIDES MAC SHOULD USE THIS METHOD.
>      void adjustPageHeight(float* newBottom, float oldTop, float oldBottom,
float bottomLimit);

I agree with renaming this method to make it more scarey-sounding. If I just
looked at the header, I would not have seen this comment.

> Index: WebCore/rendering/RenderView.h
> ===================================================================

> +    // FIXME: Only used by embedded WebViews inside AppKit NSViews.	Find a
way to remove.
> +    int m_bestTruncatedAt;
>      int m_truncatedAt;
> +    int m_truncatorWidth;
> +    IntRect m_printRect;
> +    bool m_forcedPageBreak;
> +    // End deprecated members.

Could these be moved into a struct just to keep them together?

> @@ -274,6 +292,7 @@ private:
>      bool m_disabled : 1;	   // true if the offset and clip part of
layoutState is disabled
>      bool m_didStart : 1;	   // true if we did a push or disable
>      bool m_didEnd : 1;	   // true if we popped or re-enabled
> +    bool m_didCreateLayoutState; // true if we actually made a layout state.


Put in the bitfield?