Bug 85118 – REGRESSION(r95249): Iframes are printed blank

• Status: RESOLVED
• Resolution: FIXED
• Component: Printing

[Vitaly Buka]

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

[Vitaly Buka]

Created attachment 139318 [details]
Iframe printing error reproducer.

[Vitaly Buka]

Created attachment 139323 [details]
Iframe prinring.

[Vitaly Buka]

Created attachment 139327 [details]
Iframe printing fix.

[Alexey Proskuryakov]

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.

[Vitaly Buka]

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.

[Vitaly Buka]

Created attachment 139369 [details]
Iframe printing fix.

[Vitaly Buka]

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.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

<rdar://problem/11348396>

[Kentaro Hara]

cc-ing reviewers of previous printing bugs

[Eric Seidel (no email)]

Yes, the PDF testing needs to just be removed.  I thought they were skipped on all platforms?

[Eric Seidel (no email)]

Note the author's name is in the PDF, various other machine information is likely there too:
 180 (Vitaly Buka)

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/95249 was the changeset in question, btw.  (So others don't have to construct the URL for themselves.)

[Vitaly Buka]

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

[Vitaly Buka]

Created attachment 139588 [details]
Iframe printing fix.

[Vitaly Buka]

Created attachment 139591 [details]
Iframe printing fix.

[Vitaly Buka]

Created attachment 139594 [details]
Iframe printing fix,

[Vitaly Buka]

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

[Vitaly Buka]

Alexey, any more comments?

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

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?

[Vitaly Buka]

Created attachment 139875 [details]
Iframe printing fix.

[Vitaly Buka]

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.

[Vitaly Buka]

Created attachment 139879 [details]
Iframe printing fix.

Just noticed that there is "Flags: review" option.

[Alexey Proskuryakov]

Comment on attachment 139879 [details]
Iframe printing fix.

Please set r?, not r+.

[Vitaly Buka]

(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

[Vitaly Buka]

More comments?

[Vitaly Buka]

Please review this patch.

[Kentaro Hara]

(In reply to comment #28)
> Please review this patch.

darin, hyatt: We are happy if you would take a look at the patch.

[Darin Adler]

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();

[Vitaly Buka]

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();

[Vitaly Buka]

Created attachment 141559 [details]
Iframe printing fix.

[Vitaly Buka]

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.

[Vitaly Buka]

Created attachment 141560 [details]
Iframe printing fix.

[Vitaly Buka]

Created attachment 141561 [details]
Iframe printing fix.

[Vitaly Buka]

Created attachment 141562 [details]
Iframe printing fix.

[Vitaly Buka]

Darin, please take a look again.

[Darin Adler]

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?

[Vitaly Buka]

Created attachment 142345 [details]
Iframe printing fix.

[Vitaly Buka]

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

[Vitaly Buka]

Created attachment 142351 [details]
Iframe printing fix.

Testing for parent() instead of size is better.

[Vitaly Buka]

Can we try to finish with bug this week?

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Vitaly Buka]

Created attachment 143114 [details]
Iframe printing fix.

[Vitaly Buka]

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.

[Eric Seidel (no email)]

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.

[Vitaly Buka]

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

[Vitaly Buka]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Vitaly Buka]

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

[Darin Adler]

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

[Darin Adler]

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.

[Darin Adler]

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.

[WebKit Review Bot]

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

[Vitaly Buka]

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

[WebKit Review Bot]

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

[Vitaly Buka]

Created attachment 143338 [details]
Iframe printing fix.

[Eric Seidel (no email)]

Comment on attachment 143338 [details]
Iframe printing fix.

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

> LayoutTests/ChangeLog:1
> [phone]  Vitaly Buka  <[e-mail]>

Still not fixed.

[Vitaly Buka]

Created attachment 143342 [details]
Iframe printing fix.

Ah, I didn't notice your comment on IRC about 1.

[WebKit Review Bot]

Comment on attachment 143342 [details]
Iframe printing fix.

Clearing flags on attachment: 143342

Committed r118039: <http://trac.webkit.org/changeset/118039>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

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.

[Alexey Proskuryakov]

This caused bug 88201.