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

[bugzilla-daemon at webkit.org]

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


Vitaly Buka <vitalybuka at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #139594|1                           |0
        is obsolete|                            |




--- Comment #23 from Vitaly Buka <vitalybuka at chromium.org>  2012-05-02 13:51:04 PST ---
(From update of attachment 139594)
View in context: https://bugs.webkit.org/attachment.cgi?id=139594&action=review

>> Source/WebCore/ChangeLog:13
>> +        frame requires special handling because it deppends on page size.
> 
> Frames in a frameset also depend on page size AFAICT.

I did't mean frame as html tag. I changed to Frame to make clear that it's about WebCore class.

>> Source/WebCore/rendering/RenderView.cpp:116
>> +    if (usePrintingLayout())
> 
> IIRC, these are not the only uses of printing() in this file.  Maybe I'm thinking of FrameView.  It seems odd that the inner FrameView's would need any special handling either.

The rest looks fine with current printing() implementation. It mostly disables zoom, animation or local repaints.
I am little unsure about FrameView::layout() {
...
if (!subtree && !toRenderView(root)->printing())
    adjustViewSize();
...
}

I just tried to stay conservative because my knowledge of WebKit is limited.

>> Source/WebCore/rendering/RenderView.cpp:664
>> +    }
> 
> I think there is a way to check frame->tree()->top()?  Maybe there is an isTop?  I'm surprised you have to write your own ancestor walk here.

It's now walk ancestors.
Sorry, It's the second misread of this code, so I'll try to write in different way.

>> Source/WebCore/rendering/RenderView.cpp:665
>> +    return true;
> 
> So if any of these is null, this function will return true? That seems strange.
> 
> As far as coding style goes, WebKit uses early returns, not deep nesting for conditions like this.
> 
> I think that the comment about root frame should explain why.

If pointers is null probably something wrong, so it does not matter whet we return here. I just tried to keep logic for that case.

>> Source/WebCore/rendering/RenderView.h:89
>> +    bool usePrintingLayout() const;
> 
> I'm confused why either of these would be public.  Do we need to audit other callers?

I can hide only usePrintingLayout() to private. And my personal believe that optimizing printing() calls must be separate CL.

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