[Webkit-unassigned] [Bug 85118] REGRESSION(r95249): Iframes are printed blank

[bugzilla-daemon at webkit.org]

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





--- Comment #31 from Vitaly Buka <vitalybuka at chromium.org>  2012-05-11 14:47:35 PST ---
Thanks for feedback.
I got all comments except one:

>>  Also, we changed the behavior when contentRenderer is 0 so I want to see a test that covers that too.

Do you mean change from?:

FloatSize resultSize;
if (!contentRenderer())
   return FloatSize();

to:

FloatSize resultSize;
if (!contentRenderer())
   return resultSize;


(In reply to comment #30)
> (From update of attachment 139879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139879&action=review
> 
> Thanks for tackling this. Seems close to ready.
> 
> Given that the code changes are separate for horizontal vs. vertical writing mode, we need test cases that cover both. Also, we changed the behavior when contentRenderer is 0 so I want to see a test that covers that too.
> 
> > Source/WebCore/page/Frame.cpp:545
> > +        if (fabs(originalSize.width()) < numeric_limits<float>::epsilon())
> > +            return resultSize;
> 
> This does not seem like the right approach. We should have some cleaner way to avoid trying to work with originalSize rather than relying on the fact that its width is zero. I think the change should be at the printing-specific call site rather than here in this function.
> 
> > Source/WebCore/rendering/RenderView.cpp:652
> > +bool RenderView::usePrintingLayout() const
> 
> This function sounds like something that tells the view to use a printing layout. That’s why we use names like shouldUsePrintingLayout.
> 
> > Source/WebCore/rendering/RenderView.cpp:658
> > +    if (!printing() || !m_frameView || !m_frameView->frame())
> > +        return false;
> > +    FrameTree* tree = m_frameView->frame()->tree();
> > +    // Only root frame should have special handling for printing.
> > +    return tree && !tree->parent();
> 
> Since tree can’t be null here is how this should be written:
> 
>     if (!printing() || !m_frameView)
>         return false;
>     Frame* frame = m_frameView->frame();
>     return frame && !frame->tree()->parent();
(In reply to comment #30)
> (From update of attachment 139879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139879&action=review
> 
> Thanks for tackling this. Seems close to ready.
> 
> Given that the code changes are separate for horizontal vs. vertical writing mode, we need test cases that cover both. Also, we changed the behavior when contentRenderer is 0 so I want to see a test that covers that too.
> 
> > Source/WebCore/page/Frame.cpp:545
> > +        if (fabs(originalSize.width()) < numeric_limits<float>::epsilon())
> > +            return resultSize;
> 
> This does not seem like the right approach. We should have some cleaner way to avoid trying to work with originalSize rather than relying on the fact that its width is zero. I think the change should be at the printing-specific call site rather than here in this function.
> 
> > Source/WebCore/rendering/RenderView.cpp:652
> > +bool RenderView::usePrintingLayout() const
> 
> This function sounds like something that tells the view to use a printing layout. That’s why we use names like shouldUsePrintingLayout.
> 
> > Source/WebCore/rendering/RenderView.cpp:658
> > +    if (!printing() || !m_frameView || !m_frameView->frame())
> > +        return false;
> > +    FrameTree* tree = m_frameView->frame()->tree();
> > +    // Only root frame should have special handling for printing.
> > +    return tree && !tree->parent();
> 
> Since tree can’t be null here is how this should be written:
> 
>     if (!printing() || !m_frameView)
>         return false;
>     Frame* frame = m_frameView->frame();
>     return frame && !frame->tree()->parent();

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