Bug 85118 - REGRESSION(r95249): Iframes are printed blank
Summary: REGRESSION(r95249): Iframes are printed blank
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Nobody
URL: http://code.google.com/p/chromium/sou...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-04-27 19:12 PDT by Vitaly Buka
Modified: 2012-06-04 15:59 PDT (History)
11 users (show)

See Also:


Attachments
Iframe printing error reproducer. (449 bytes, application/x-zip-compressed)
2012-04-27 19:13 PDT, Vitaly Buka
no flags Details
Iframe prinring. (22.59 KB, patch)
2012-04-27 19:28 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (17.74 KB, patch)
2012-04-27 19:52 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (18.13 KB, patch)
2012-04-28 12:47 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (9.38 KB, patch)
2012-05-01 00:08 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (9.37 KB, patch)
2012-05-01 00:11 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix, (9.26 KB, patch)
2012-05-01 00:17 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (9.04 KB, patch)
2012-05-02 13:49 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (9.04 KB, patch)
2012-05-02 13:57 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.31 KB, patch)
2012-05-12 01:45 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.31 KB, patch)
2012-05-12 01:52 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.30 KB, patch)
2012-05-12 01:54 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.31 KB, patch)
2012-05-12 01:58 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.15 KB, patch)
2012-05-16 14:17 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.24 KB, patch)
2012-05-16 15:20 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.09 KB, patch)
2012-05-21 15:21 PDT, Vitaly Buka
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (849.45 KB, application/zip)
2012-05-21 20:42 PDT, WebKit Review Bot
no flags Details
Iframe printing fix. (11.06 KB, patch)
2012-05-22 11:44 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff
Iframe printing fix. (11.06 KB, patch)
2012-05-22 11:53 PDT, Vitaly Buka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vitaly Buka 2012-04-27 19:12:05 PDT
Nightly Webkit rev115517 and WebKit in Chromium leave blank space in place of iframes.
Inconsistently reproducible on OSX Safari 5.1.5

It can be reproduced with attached files or on page
http://code.google.com/p/chromium/source/search

Original Chromium issue http://code.google.com/p/chromium/issues/detail?id=109412
Comment 1 Vitaly Buka 2012-04-27 19:13:46 PDT
Created attachment 139318 [details]
Iframe printing error reproducer.
Comment 2 Vitaly Buka 2012-04-27 19:28:02 PDT
Created attachment 139323 [details]
Iframe prinring.
Comment 3 Vitaly Buka 2012-04-27 19:52:29 PDT
Created attachment 139327 [details]
Iframe printing fix.
Comment 4 Alexey Proskuryakov 2012-04-27 23:07:29 PDT
Comment on attachment 139327 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=139327&action=review

> Source/WebCore/ChangeLog:13
> +        * page/Frame.cpp:
> +        (WebCore::Frame::resizePageRectsKeepingRatio):
> +        * rendering/RenderView.cpp:
> +        (WebCore::RenderView::printing):

Can you explain the changes in ChangeLog?

> Source/WebCore/rendering/RenderView.cpp:652
> +    if (Frame* frame = (m_frameView ? m_frameView->frame() : 0)) {
> +        if (FrameTree* tree = frame->tree()) {

This can't be right - first line explicitly considers a null frame, while the second one deferences the variable without checks.
Comment 5 Vitaly Buka 2012-04-28 12:19:26 PDT
Comment on attachment 139327 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=139327&action=review

>> Source/WebCore/rendering/RenderView.cpp:652
>> +        if (FrameTree* tree = frame->tree()) {
> 
> This can't be right - first line explicitly considers a null frame, while the second one deferences the variable without checks.

If frame is NULL than "if" on line 651 will fail and program will not go to 652
If you like I can unroll that too to make it more readable.
Comment 6 Vitaly Buka 2012-04-28 12:47:06 PDT
Created attachment 139369 [details]
Iframe printing fix.
Comment 7 Vitaly Buka 2012-04-28 12:47:51 PDT
Comment on attachment 139327 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=139327&action=review

>> Source/WebCore/ChangeLog:13
>> +        (WebCore::RenderView::printing):
> 
> Can you explain the changes in ChangeLog?

Done.
Comment 8 Alexey Proskuryakov 2012-04-30 11:33:41 PDT
> If frame is NULL than "if" on line 651 will fail and program will not go to 652
> If you like I can unroll that too to make it more readable.

Ugh. You're right, but this is a very unusual idiom.

Anyway, this is a wrong place to add this check. A function named "printing()" should tell whether we're printing, not make guesses about how the result will be used, returning a well-intentioned lie. You should either put the check in a different place, or rename the function to reflect what it's actually returning.

Eric, I think that you recently mentioned that PDF expected results don't work, because they are machine specific. Could you please comment on whether this is one of those cases?
Comment 9 Alexey Proskuryakov 2012-04-30 11:35:26 PDT
<rdar://problem/11348396>
Comment 10 Kentaro Hara 2012-04-30 11:37:21 PDT
cc-ing reviewers of previous printing bugs
Comment 11 Eric Seidel (no email) 2012-04-30 11:59:30 PDT
Yes, the PDF testing needs to just be removed.  I thought they were skipped on all platforms?
Comment 12 Eric Seidel (no email) 2012-04-30 12:00:22 PDT
Note the author's name is in the PDF, various other machine information is likely there too:
 180 (Vitaly Buka)
Comment 13 Eric Seidel (no email) 2012-04-30 12:01:04 PDT
http://trac.webkit.org/changeset/95249 was the changeset in question, btw.  (So others don't have to construct the URL for themselves.)
Comment 14 Vitaly Buka 2012-04-30 12:45:51 PDT
(In reply to comment #13)
> http://trac.webkit.org/changeset/95249 was the changeset in question, btw.  (So others don't have to construct the URL for themselves.)

Yes. I already tried to rollback this one. Rollback fixed some iframe issues. But not all of them. The rest addressed by ::printing() change.

So I am going to:
1. Rollback ::printing() and introduce new function ::usePrintingLayout()
2. Replace PDF test with regular js.
Comment 15 Vitaly Buka 2012-05-01 00:08:00 PDT
Created attachment 139588 [details]
Iframe printing fix.
Comment 16 Vitaly Buka 2012-05-01 00:11:07 PDT
Created attachment 139591 [details]
Iframe printing fix.
Comment 17 Vitaly Buka 2012-05-01 00:17:12 PDT
Created attachment 139594 [details]
Iframe printing fix,
Comment 18 Vitaly Buka 2012-05-01 00:18:13 PDT
(In reply to comment #8)
> > If frame is NULL than "if" on line 651 will fail and program will not go to 652
> > If you like I can unroll that too to make it more readable.
> 
> Ugh. You're right, but this is a very unusual idiom.
> 
> Anyway, this is a wrong place to add this check. A function named "printing()" should tell whether we're printing, not make guesses about how the result will be used, returning a well-intentioned lie. You should either put the check in a different place, or rename the function to reflect what it's actually returning.
> 
> Eric, I think that you recently mentioned that PDF expected results don't work, because they are machine specific. Could you please comment on whether this is one of those cases?

Done.
Comment 19 Vitaly Buka 2012-05-02 10:41:25 PDT
Alexey, any more comments?
Comment 20 Alexey Proskuryakov 2012-05-02 11:06:41 PDT
Comment on attachment 139594 [details]
Iframe printing fix,

View in context: https://bugs.webkit.org/attachment.cgi?id=139594&action=review

You should set r? flag, so that an expert in rendering/printing would eventually take a look.

> Source/WebCore/ChangeLog:13
> +        Disables special processing for printing for subframes in RenderView. Only root
> +        frame requires special handling because it deppends on page size.

Frames in a frameset also depend on page size AFAICT.

> Source/WebCore/rendering/RenderView.cpp:665
> +    if (m_frameView) {
> +        if (Frame* frame = m_frameView->frame()) {
> +            if (FrameTree* tree = frame->tree()) {
> +                // Only root frame should have special handling for printing.
> +                if (tree->parent())
> +                    return false;
> +            }
> +        }
> +    }
> +    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.
Comment 21 Eric Seidel (no email) 2012-05-02 11:10:47 PDT
Comment on attachment 139594 [details]
Iframe printing fix,

View in context: https://bugs.webkit.org/attachment.cgi?id=139594&action=review

I like the idea.  I'm not 100% sure the patch is complete yet though.

> Source/WebCore/rendering/RenderView.cpp:116
> -    if (printing())
> +    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.

> Source/WebCore/rendering/RenderView.cpp:664
> +    if (m_frameView) {
> +        if (Frame* frame = m_frameView->frame()) {
> +            if (FrameTree* tree = frame->tree()) {
> +                // Only root frame should have special handling for printing.
> +                if (tree->parent())
> +                    return false;
> +            }
> +        }
> +    }

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.

> Source/WebCore/rendering/RenderView.h:89
>      bool printing() const;
> +    bool usePrintingLayout() const;

I'm confused why either of these would be public.  Do we need to audit other callers?
Comment 22 Vitaly Buka 2012-05-02 13:49:24 PDT
Created attachment 139875 [details]
Iframe printing fix.
Comment 23 Vitaly Buka 2012-05-02 13:51:04 PDT
Comment on attachment 139594 [details]
Iframe printing fix,

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.
Comment 24 Vitaly Buka 2012-05-02 13:57:43 PDT
Created attachment 139879 [details]
Iframe printing fix.

Just noticed that there is "Flags: review" option.
Comment 25 Alexey Proskuryakov 2012-05-02 14:56:47 PDT
Comment on attachment 139879 [details]
Iframe printing fix.

Please set r?, not r+.
Comment 26 Vitaly Buka 2012-05-02 15:07:11 PDT
(In reply to comment #25)
> (From update of attachment 139879 [details])
> Please set r?, not r+.

Done.

Seems something is missing.
http://www.webkit.org/coding/contributing.html#submit
Comment 27 Vitaly Buka 2012-05-04 10:29:47 PDT
More comments?
Comment 28 Vitaly Buka 2012-05-10 16:48:18 PDT
Please review this patch.
Comment 29 Kentaro Hara 2012-05-10 16:52:09 PDT
(In reply to comment #28)
> Please review this patch.

darin, hyatt: We are happy if you would take a look at the patch.
Comment 30 Darin Adler 2012-05-10 17:32:37 PDT
Comment on attachment 139879 [details]
Iframe printing fix.

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();
Comment 31 Vitaly Buka 2012-05-11 14:47:35 PDT
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();
Comment 32 Vitaly Buka 2012-05-12 01:45:14 PDT
Created attachment 141559 [details]
Iframe printing fix.
Comment 33 Vitaly Buka 2012-05-12 01:46:55 PDT
Comment on attachment 139879 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=139879&action=review

>>>> Source/WebCore/page/Frame.cpp:545
>>>> +            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.
>> 
>> (In reply to comment #30)
> 
> 

Done.

>> 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.

Done.

>> Source/WebCore/rendering/RenderView.cpp:658
>> +    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();

Done.
Comment 34 Vitaly Buka 2012-05-12 01:52:13 PDT
Created attachment 141560 [details]
Iframe printing fix.
Comment 35 Vitaly Buka 2012-05-12 01:54:33 PDT
Created attachment 141561 [details]
Iframe printing fix.
Comment 36 Vitaly Buka 2012-05-12 01:58:47 PDT
Created attachment 141562 [details]
Iframe printing fix.
Comment 37 Vitaly Buka 2012-05-15 10:00:47 PDT
Darin, please take a look again.
Comment 38 Darin Adler 2012-05-15 12:52:02 PDT
Comment on attachment 141562 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=141562&action=review

Fix looks good but I have one question. I won’t say review- because I’m not certain the code is wrong.

> Source/WebCore/page/FrameView.cpp:3253
> -        if (docLogicalWidth > pageLogicalWidth) {
> +        if (!pageSize.isZero() && docLogicalWidth > pageLogicalWidth) {

I still don’t understand why we are in this function at all for a frame that is not the main frame. If the caller is passing a size of zero then it shouldn’t even be calling the forceLayoutForPagination function in the first place. How does this happen in practice?
Comment 39 Vitaly Buka 2012-05-16 14:17:13 PDT
Created attachment 142345 [details]
Iframe printing fix.
Comment 40 Vitaly Buka 2012-05-16 14:25:04 PDT
(In reply to comment #38)
> (From update of attachment 141562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141562&action=review
> 
> Fix looks good but I have one question. I won’t say review- because I’m not certain the code is wrong.
> 
> > Source/WebCore/page/FrameView.cpp:3253
> > -        if (docLogicalWidth > pageLogicalWidth) {
> > +        if (!pageSize.isZero() && docLogicalWidth > pageLogicalWidth) {
> 
> I still don’t understand why we are in this function at all for a frame that is not the main frame. If the caller is passing a size of zero then it shouldn’t even be calling the forceLayoutForPagination function in the first place. How does this happen in practice?

You right. We can avoid this call.
Fixed.
Please review again.
Comment 41 Vitaly Buka 2012-05-16 15:20:28 PDT
Created attachment 142351 [details]
Iframe printing fix.

Testing for parent() instead of size is better.
Comment 42 Vitaly Buka 2012-05-21 11:31:40 PDT
Can we try to finish with bug this week?
Comment 43 Eric Seidel (no email) 2012-05-21 11:55:27 PDT
Comment on attachment 142351 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=142351&action=review

I'm ready to r+ this once you answer my one question below.

> Source/WebCore/page/Frame.cpp:525
> +    if (printing && !tree()->parent())

Why do we do this for only the root frame?
Comment 44 Eric Seidel (no email) 2012-05-21 11:55:57 PDT
Comment on attachment 142351 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=142351&action=review

> Source/WebCore/ChangeLog:88
> +        (WebCore::Frame::setPrinting):

You could have added explaination for that chnage here, for example.
Comment 45 Vitaly Buka 2012-05-21 15:21:13 PDT
Created attachment 143114 [details]
Iframe printing fix.
Comment 46 Vitaly Buka 2012-05-21 15:24:55 PDT
Comment on attachment 142351 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=142351&action=review

>> Source/WebCore/ChangeLog:88
>> +        (WebCore::Frame::setPrinting):
> 
> You could have added explaination for that chnage here, for example.

Done.

>> Source/WebCore/page/Frame.cpp:525
>> +    if (printing && !tree()->parent())
> 
> Why do we do this for only the root frame?

Added comment to code.
I assume that subframes should not be affected directly by page size, only by layout of parent.
Comment 47 Eric Seidel (no email) 2012-05-21 15:36:41 PDT
Comment on attachment 142351 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=142351&action=review

>>> Source/WebCore/page/Frame.cpp:525
>>> +    if (printing && !tree()->parent())
>> 
>> Why do we do this for only the root frame?
> 
> Added comment to code.
> I assume that subframes should not be affected directly by page size, only by layout of parent.

For better or worse, each frame has to lay itself out.  It doesn't really get much from its parent, except a size.  So I suspect this part of the chagne may be wrong.

When we do layout, it's per-frame.  EAch frame does its own layout.
Comment 48 Vitaly Buka 2012-05-21 15:46:41 PDT
(In reply to comment #47)
> (From update of attachment 142351 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142351&action=review
> 
> >>> Source/WebCore/page/Frame.cpp:525
> >>> +    if (printing && !tree()->parent())
> >> 
> >> Why do we do this for only the root frame?
> > 
> > Added comment to code.
> > I assume that subframes should not be affected directly by page size, only by layout of parent.
> 
> For better or worse, each frame has to lay itself out.  It doesn't really get much from its parent, except a size.  So I suspect this part of the chagne may be wrong.
> 
> When we do layout, it's per-frame.  EAch frame does its own layout.

Actually current ::forceLayoutForPagination mostly different from ::forceLayout just buy size which is calculated from page. So we subfames we fall back to regular ::forceLayout. I don't see problem here. And I didn't found any page that looks incorrect because of that.

Also we have to do something here. Current build just doesn't print iframes at all. So it's signification improvement.
Comment 49 Vitaly Buka 2012-05-21 15:55:06 PDT
Comment on attachment 143114 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=143114&action=review

> Source/WebCore/page/Frame.cpp:529
>          view()->forceLayout();

Just for clarification. For subframes we fall back to view()->forceLayout(); which is almost the same as view()->forceLayoutForPagination but without pagesize restrictions.
Comment 50 WebKit Review Bot 2012-05-21 20:42:34 PDT
Comment on attachment 143114 [details]
Iframe printing fix.

Attachment 143114 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12740672

New failing tests:
http/tests/security/script-crossorigin-loads-correctly.html
http/tests/xmlhttprequest/zero-length-response.html
fast/loader/loadInProgress.html
fast/loader/unload-form-post-about-blank.html
Comment 51 WebKit Review Bot 2012-05-21 20:42:40 PDT
Created attachment 143182 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 52 Vitaly Buka 2012-05-22 10:50:14 PDT
(In reply to comment #50)
> (From update of attachment 143114 [details])
> Attachment 143114 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12740672
> 
> New failing tests:
> http/tests/security/script-crossorigin-loads-correctly.html
> http/tests/xmlhttprequest/zero-length-response.html
> fast/loader/loadInProgress.html
> fast/loader/unload-form-post-about-blank.html

Probably it's not mine patch. My should break all platforms.
Comment 53 Darin Adler 2012-05-22 11:08:49 PDT
(In reply to comment #52)
> Probably it's not mine patch.

Could be.

> My should break all platforms.

In the EWS, only the Chromium-Linux bot runs the regression tests. So the fact that a change should break all platforms does not tell us anything about EWS results.
Comment 54 Darin Adler 2012-05-22 11:10:14 PDT
Comment on attachment 143114 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=143114&action=review

> Source/WebCore/ChangeLog:24
> +        (WebCore):

Please remove non-helpful lines like this from ChangeLog.

> Source/WebCore/ChangeLog:29
> +        (RenderView):

And lines like this.

>> Source/WebCore/page/Frame.cpp:529
>>          view()->forceLayout();
> 
> Just for clarification. For subframes we fall back to view()->forceLayout(); which is almost the same as view()->forceLayoutForPagination but without pagesize restrictions.

If that clarification is needed, then it should probably be in a comment in the code, not just a comment during patch review.
Comment 55 Darin Adler 2012-05-22 11:11:53 PDT
Comment on attachment 143114 [details]
Iframe printing fix.

The review bot set commit- based on EWS failing, but those failures are not related to this change.
Comment 56 WebKit Review Bot 2012-05-22 11:15:10 PDT
Comment on attachment 143114 [details]
Iframe printing fix.

Rejecting attachment 143114 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors
    self._run(tool, options, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: http://queues.webkit.org/results/12762104
Comment 57 Vitaly Buka 2012-05-22 11:17:00 PDT
> If that clarification is needed, then it should probably be in a comment in the code, not just a comment during patch review.

I believe it's unndesesary. Basically I just pointed out that it goes to else {} branch.
Comment 58 WebKit Review Bot 2012-05-22 11:27:51 PDT
Comment on attachment 143114 [details]
Iframe printing fix.

Rejecting attachment 143114 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors
    self._run(tool, options, state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 50, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: http://queues.webkit.org/results/12747526
Comment 59 Vitaly Buka 2012-05-22 11:44:23 PDT
Created attachment 143338 [details]
Iframe printing fix.
Comment 60 Eric Seidel (no email) 2012-05-22 11:45:22 PDT
Comment on attachment 143338 [details]
Iframe printing fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=143338&action=review

> LayoutTests/ChangeLog:1
> +12012-05-22  Vitaly Buka  <vitalybuka@chromium.org>

Still not fixed.
Comment 61 Vitaly Buka 2012-05-22 11:53:42 PDT
Created attachment 143342 [details]
Iframe printing fix.

Ah, I didn't notice your comment on IRC about 1.
Comment 62 WebKit Review Bot 2012-05-22 13:25:33 PDT
Comment on attachment 143342 [details]
Iframe printing fix.

Clearing flags on attachment: 143342

Committed r118039: <http://trac.webkit.org/changeset/118039>
Comment 63 WebKit Review Bot 2012-05-22 13:25:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 64 Jessie Berlin 2012-05-22 16:17:07 PDT
This test should have been skipped on WK2:

@@ -1,16 +1,17 @@
-layer at (0,0) size 1000x324
-  RenderView at (0,0) size 1000x324
-layer at (0,0) size 1000x324
-  RenderBlock {HTML} at (0,0) size 1000x324
-    RenderBody {BODY} at (8,8) size 984x308
+CONSOLE MESSAGE: line 4: TypeError: 'undefined' is not a function (evaluating 'layoutTestController.setPrinting()')
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584

http://build.webkit.org/results/Lion%20Debug%20(WebKit2%20Tests)/r118064%20(7367)/printing/iframe-print-diff.txt

I will skip it myself this time, but you should be looking at the WK2 Lion bots after you check in. They are close to green, so there is no reason you couldn't have spotted this yourself.
Comment 65 Alexey Proskuryakov 2012-06-04 15:59:08 PDT
This caused bug 88201.