Bug 36159 - Move code related with printing defined in WebFrame.mm to WebCore::PrintContext
Summary: Move code related with printing defined in WebFrame.mm to WebCore::PrintContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-16 01:01 PDT by Hayato Ito
Modified: 2010-03-24 18:11 PDT (History)
6 users (show)

See Also:


Attachments
refactor-computePageRects (9.91 KB, patch)
2010-03-16 23:02 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
refactor-computePageRects-fix-chromium-build (9.84 KB, patch)
2010-03-17 01:03 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
refactor-computePageRects (9.72 KB, patch)
2010-03-17 02:46 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2010-03-16 01:01:50 PDT
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.
Comment 1 Hayato Ito 2010-03-16 23:02:15 PDT
Created attachment 50876 [details]
refactor-computePageRects
Comment 2 WebKit Review Bot 2010-03-16 23:06:30 PDT
Attachment 50876 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/927020
Comment 3 Hayato Ito 2010-03-16 23:15:07 PDT
I've cleared the review flag because the patch needs to be compiled at chromium.
Comment 4 Hayato Ito 2010-03-17 01:03:56 PDT
Created attachment 50880 [details]
refactor-computePageRects-fix-chromium-build
Comment 5 Shinichiro Hamaji 2010-03-17 01:32:23 PDT
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?)
Comment 6 Hayato Ito 2010-03-17 02:46:28 PDT
Created attachment 50883 [details]
refactor-computePageRects
Comment 7 Hayato Ito 2010-03-17 02:58:44 PDT
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.
Comment 8 Shinichiro Hamaji 2010-03-17 21:55:55 PDT
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.
Comment 9 WebKit Commit Bot 2010-03-24 13:38:12 PDT
Comment on attachment 50883 [details]
refactor-computePageRects

Clearing flags on attachment: 50883

Committed r56454: <http://trac.webkit.org/changeset/56454>
Comment 10 WebKit Commit Bot 2010-03-24 13:38:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Seidel (no email) 2010-03-24 18:11:09 PDT
ChangeLog fixed in http://trac.webkit.org/changeset/56476.