[Webkit-unassigned] [Bug 36159] Move code related with printing defined in WebFrame.mm to WebCore::PrintContext

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36159





--- Comment #7 from Hayato Ito <hayato at chromium.org>  2010-03-17 02:58:44 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.