Bug 36159 – Move code related with printing defined in WebFrame.mm to WebCore::PrintContext

• Component: Printing
• Status: RESOLVED FIXED

[Hayato Ito]

There are similar code between _computePageRectsWithPrintWidthScaleFactor in WebFrame.mm and WebCore::PrintContext.

It might be better that sharing similar code between them before starting to implement new features related with CSS3's PagedMedia.
Maybe some code related with printing defined in WebFrame.mm should move to WebCore::PrintContext, and use WebCore::PrintContext from WebKit even in Mac.

[Hayato Ito]

Created attachment 50876 [details]
refactor-computePageRects

[WebKit Review Bot]

Attachment 50876 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/927020

[Hayato Ito]

I've cleared the review flag because the patch needs to be compiled at chromium.

[Hayato Ito]

Created attachment 50880 [details]
refactor-computePageRects-fix-chromium-build

[Shinichiro Hamaji]

Comment on attachment 50880 [details]
refactor-computePageRects-fix-chromium-build

Basically, looks good. r- for a few style nitpicks.

> -    RenderView* root = toRenderView(m_frame->document()->renderer());
> -
> -    if (!root) {
> -        LOG_ERROR("document to be printed has no renderer");
> +    if (userScaleFactor <= 0) {
> +        LOG_ERROR("userScaleFactor has bad value %.2f", userScaleFactor);
>          return;
>      }
>  
> +    RenderView* root = toRenderView(m_frame->document()->renderer());
> +

It seems like NULL check for root was gone. Is this OK?

> +    const Vector<IntRect>& pageRects = printContext.pageRects();
> +    const size_t pageCount = pageRects.size();
> +    for (size_t pageNumber = 0; pageNumber < pageCount; ++pageNumber) {
> +      const IntRect& pageRect = pageRects[pageNumber];
> +      NSValue* val = [NSValue valueWithRect: NSMakeRect(pageRect.x(), pageRect.y(), pageRect.width(), pageRect.height())];
> +      [pages addObject: val];

- Two more spaces for indent here.
- Cannot we use IntRect::operator NSRect() to create NSRect? (maybe it's not in WebCore.base.exp?)

[Hayato Ito]

Created attachment 50883 [details]
refactor-computePageRects

[Hayato Ito]

Thank you for the review.

(In reply to comment #5)
> (From update of attachment 50880 [details])
> Basically, looks good. r- for a few style nitpicks.
> 
> > -    RenderView* root = toRenderView(m_frame->document()->renderer());
> > -
> > -    if (!root) {
> > -        LOG_ERROR("document to be printed has no renderer");
> > +    if (userScaleFactor <= 0) {
> > +        LOG_ERROR("userScaleFactor has bad value %.2f", userScaleFactor);
> >          return;
> >      }
> >  
> > +    RenderView* root = toRenderView(m_frame->document()->renderer());
> > +
> 
> It seems like NULL check for root was gone. Is this OK?

The reason I've omitted NULL check for |root| here is that it never happens, I guess.

NULL check for m_frame->document()->renderer() is already done in the two callers, computePageRectsWithPageSize and computePageRects.

It never happens that |root| becomes NULL in the following code, doesn't it?

    if (!m_frame->document() || !m_frame->view() || !m_frame->document()->renderer())
         return;
    RenderView* root = toRenderView(m_frame->document()->renderer());
    if (!root) {
        LOG_ERROR("document to be printed has no renderer");

Anyway, for the maximum safety, I've decided to insert NULL check at the head of PrintContext::computePageRectsWithPageSizeInternal in the new patch in case that it is called from other functions in the future.

> 
> > +    const Vector<IntRect>& pageRects = printContext.pageRects();
> > +    const size_t pageCount = pageRects.size();
> > +    for (size_t pageNumber = 0; pageNumber < pageCount; ++pageNumber) {
> > +      const IntRect& pageRect = pageRects[pageNumber];
> > +      NSValue* val = [NSValue valueWithRect: NSMakeRect(pageRect.x(), pageRect.y(), pageRect.width(), pageRect.height())];
> > +      [pages addObject: val];
> 
> - Two more spaces for indent here.
> - Cannot we use IntRect::operator NSRect() to create NSRect? (maybe it's not in
> WebCore.base.exp?)

Done. Thank you! It's already in WebCore.base.exp.

[Shinichiro Hamaji]

Comment on attachment 50883 [details]
refactor-computePageRects

This looks sane. I'll land this patch unless anyone object tomorrow. I want to wait a moment in case someone have ideas about this.

I asked him to test this manually on Mac by setting scale in the dialog box for printing and looking the print preview. He said it seems working.

[WebKit Commit Bot]

Comment on attachment 50883 [details]
refactor-computePageRects

Clearing flags on attachment: 50883

Committed r56454: <http://trac.webkit.org/changeset/56454>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

ChangeLog fixed in http://trac.webkit.org/changeset/56476.