Bug 36798 - iframe flattening doesn't flatten
Summary: iframe flattening doesn't flatten
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL: http://www.samisite.com/test-csb2nf/i...
Keywords: InRadar, Qt
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-03-29 18:17 PDT by Greg Bolsinga
Modified: 2010-04-06 02:46 PDT (History)
12 users (show)

See Also:


Attachments
Test case (1.45 KB, text/html)
2010-03-30 09:09 PDT, Kenneth Rohde Christiansen
no flags Details
First load (15.17 KB, image/png)
2010-03-30 09:09 PDT, Kenneth Rohde Christiansen
no flags Details
Second load (14.08 KB, image/png)
2010-03-30 09:10 PDT, Kenneth Rohde Christiansen
no flags Details
Patch for wrong logic (9.66 KB, patch)
2010-03-30 10:57 PDT, Kenneth Rohde Christiansen
koivisto: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Scrollbar related patch (5.80 KB, patch)
2010-03-31 08:26 PDT, Kenneth Rohde Christiansen
koivisto: review+
Details | Formatted Diff | Diff
screen shot _after_ a resize of the window. (199.15 KB, image/png)
2010-03-31 13:10 PDT, Greg Bolsinga
no flags Details
screen shot _before_ a resize of the window. (204.45 KB, image/png)
2010-03-31 13:11 PDT, Greg Bolsinga
no flags Details
Patch (should make the flattening work on the site in question) (9.78 KB, patch)
2010-04-01 13:02 PDT, Kenneth Rohde Christiansen
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Bolsinga 2010-03-29 18:17:26 PDT
I built Mac OS X with WebKitFrameFlatteningEnabledPreferenceKey with the value YES.

The page at the URL doesn't expand the iframes.

This went in with http://trac.webkit.org/changeset/56718
Comment 1 Kenneth Rohde Christiansen 2010-03-30 06:20:17 PDT
Are these flattened on the iPhone? I'm currently not flattening fixed sized iframes, but I guess we should do that.
Comment 2 Kenneth Rohde Christiansen 2010-03-30 06:52:57 PDT
OK, bug found :-) Thanks Greg for the bug report!

I will add some more tests and upload a new patch.
Comment 3 Kenneth Rohde Christiansen 2010-03-30 09:08:49 PDT
OK, I noticed something really strange that I might need some help to fix.

If the iframe that is supposed to be flattened, would be partly visible when
flattened, it will only be flattened every second reload.

It will also show scrollbars (though they are suppressed)
Comment 4 Kenneth Rohde Christiansen 2010-03-30 09:09:23 PDT
Created attachment 52045 [details]
Test case
Comment 5 Kenneth Rohde Christiansen 2010-03-30 09:09:52 PDT
Created attachment 52046 [details]
First load
Comment 6 Kenneth Rohde Christiansen 2010-03-30 09:10:12 PDT
Created attachment 52047 [details]
Second load
Comment 7 Kenneth Rohde Christiansen 2010-03-30 09:10:48 PDT
s/partly visible/partly visible or not visible within the viewport/
Comment 8 Kenneth Rohde Christiansen 2010-03-30 10:57:57 PDT
Created attachment 52054 [details]
Patch for wrong logic
Comment 9 Greg Bolsinga 2010-03-30 11:56:59 PDT
With the patch applied, I do not see the frames expanded. In addition, the "FRAME6" has scrollbars with this patch, but not without it.
Comment 10 Kenneth Rohde Christiansen 2010-03-30 12:17:37 PDT
(In reply to comment #9)
> With the patch applied, I do not see the frames expanded. In addition, the
> "FRAME6" has scrollbars with this patch, but not without it.

Nope, it only makes the check equal to the one used by the iPhone, and thus fixes the tests that are supplied with the patch which weren't working before.

It still doesn't solve all issues, just some of them.
Comment 11 Greg Bolsinga 2010-03-30 12:46:19 PDT
OK. I tried your patch applied to iPhone as well, and saw the same issues there.
Comment 12 Kenneth Rohde Christiansen 2010-03-30 12:49:11 PDT
I think the problem is that it isn't doing relayout of the owner frame when the
contents of the iframes have loaded.
Comment 13 Kenneth Rohde Christiansen 2010-03-30 21:32:58 PDT
The showing of scrollbars is an issue.

The iframe when flattened needs to be the size of the contents plus the border (default 2 pixels inset), but if I do not add additional two pixels with width and height I get scrollbars.

Calling suppressScrollbars as I currently do, doesn't work and a updateWidgetPositions() reshows the scrollbars. Dave Hyatt tells me that as we cannot turn scrollbars off (as we base our policy for doing iframe flattening using that) we might need to add a virtual method to ScrollView for using zero sized scrollbars for subframes when using frame flattening.
Comment 14 Antti Koivisto 2010-03-31 05:28:48 PDT
Comment on attachment 52054 [details]
Patch for wrong logic

Progression + nice tests. r=me even if there is need for followups.
Comment 15 Antti Koivisto 2010-03-31 05:31:52 PDT
I think there should be a real way to turn scrollbars of for frames. No scrollbar objects created etc. Currently there is no good way to do this. Even invisible or zero width scrollbars still end up running lots of hairy code that can affect layout, generate repaints etc.
Comment 16 WebKit Commit Bot 2010-03-31 07:07:23 PDT
Comment on attachment 52054 [details]
Patch for wrong logic

Rejecting patch 52054 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Last 500 characters of output:

Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12591 test cases.
fast/frames/flattening/iframe-flattening-fixed-height.html -> new (results generated in /Users/eseidel/Projects/CommitQueue/LayoutTests/platform/mac/fast/frames/flattening)

Exiting early after 1 failures. 7115 tests run.
122.70s total testing time

7114 test cases (99%) succeeded
1 test case (<1%) was new
2 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1591105
Comment 17 Kenneth Rohde Christiansen 2010-03-31 07:21:03 PDT
I will land manually and update the results afterward. Ossy has already generated new results for the Qt bots which I have validated.
Comment 18 Kenneth Rohde Christiansen 2010-03-31 08:26:27 PDT
Created attachment 52172 [details]
Scrollbar related patch
Comment 19 Eric Seidel (no email) 2010-03-31 08:30:04 PDT
Attachment 52172 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1605143
Comment 20 Antti Koivisto 2010-03-31 08:30:46 PDT
Comment on attachment 52172 [details]
Scrollbar related patch

r=me

> +bool FrameView::avoidScrollbarCreation()
> +{
> +    // with frame flattening no subframe can have scrollbars
> +    // but we also cannot turn scrollbars of as we determine
> +    // our flattening policy using that.
> +
> +    if (m_frame->page()->mainFrame() == m_frame)
> +        false;
> +
> +    if (!m_frame->settings() || m_frame->settings()->frameFlatteningEnabled())
> +        return true;

I suspect there no need to null test m_frame->setting().
Comment 21 Antonio Gomes 2010-03-31 08:41:54 PDT
(In reply to comment #20)
> (From update of attachment 52172 [details])
> r=me
> 
> > +bool FrameView::avoidScrollbarCreation()
> > +{
> > +    // with frame flattening no subframe can have scrollbars
> > +    // but we also cannot turn scrollbars of as we determine
> > +    // our flattening policy using that.
> > +
> > +    if (m_frame->page()->mainFrame() == m_frame)
> > +        false;

you want a 'return' here. pls fix that before landing.
Comment 22 Kenneth Rohde Christiansen 2010-03-31 08:50:33 PDT
> > > +    if (m_frame->page()->mainFrame() == m_frame)
> > > +        false;
> 
> you want a 'return' here. pls fix that before landing.

Good catch! I just came to the same conclusion after tracing a bug :-)
Comment 23 Kenneth Rohde Christiansen 2010-03-31 10:56:30 PDT
Comment on attachment 52054 [details]
Patch for wrong logic

Landed in r56852
Comment 24 Kenneth Rohde Christiansen 2010-03-31 11:03:53 PDT
Comment on attachment 52172 [details]
Scrollbar related patch

Fixed patch landed in r56854
Comment 25 Csaba Osztrogonác 2010-03-31 11:47:58 PDT
(In reply to comment #24)
> (From update of attachment 52172 [details])
> Fixed patch landed in r56854

Followup patch landed in http://trac.webkit.org/changeset/56855
Comment 26 Greg Bolsinga 2010-03-31 11:52:23 PDT
See  Bug 36894
Comment 27 Greg Bolsinga 2010-03-31 12:11:13 PDT
Comment 9 still applies with this patch applied and with the crasher removed.
Comment 28 Kenneth Rohde Christiansen 2010-03-31 12:13:24 PDT
(In reply to comment #27)
> Comment 9 still applies with this patch applied and with the crasher removed.

Yes, I'm still trying to figure out what is going wrong. Your comment #9 says that you seen scrollbars. Do you still see that?
Comment 29 Adam Barth 2010-03-31 12:18:30 PDT
http://trac.webkit.org/changeset/56854 appears to have broken Leopard Intel Debug (Tests)
Comment 30 Kenneth Rohde Christiansen 2010-03-31 12:20:51 PDT
(In reply to comment #29)
> http://trac.webkit.org/changeset/56854 appears to have broken Leopard Intel
> Debug (Tests)

Potential fix has been committed in r56856
Comment 31 Greg Bolsinga 2010-03-31 13:09:48 PDT
(In reply to comment #28)
> Yes, I'm still trying to figure out what is going wrong. Your comment #9 says
> that you seen scrollbars. Do you still see that?

Yes I do, in "Static IFrame" and "Static 2". However when I just went to screen shot it to attach it to the bug report, I resized the window, and now "Static 2" is full size. However "Static IFrame" is still showing scrollbars (and the frame would not appear to be flattened).
Comment 32 Greg Bolsinga 2010-03-31 13:10:28 PDT
Created attachment 52201 [details]
screen shot _after_ a resize of the window.
Comment 33 Greg Bolsinga 2010-03-31 13:11:37 PDT
Created attachment 52202 [details]
screen shot _before_ a resize of the window.
Comment 34 Kenneth Rohde Christiansen 2010-03-31 13:33:15 PDT
(In reply to comment #33)
> Created an attachment (id=52202) [details]
> screen shot _before_ a resize of the window.

Ah, I recall that mac uses different code for scrollbars due to the fact that Widget is implemented using a platform widget.

Antti, do you have any input on how to turn off subframe scrollbars on mac?
Comment 35 Kenneth Rohde Christiansen 2010-03-31 16:28:31 PDT
I have been debugging the issue with the iframe outside of the viewport not being flattened.

Apparently the contentsSize is wrong when not inside the viewport, meaning that

setContentsSize(IntSize(root->rightLayoutOverflow(), root->bottomLayoutOverflow()));

sets the wrong value.

This also happens to partly visible iframes, but only every second reload.

Any help appreciated.
Comment 36 Kenneth Rohde Christiansen 2010-03-31 17:14:41 PDT
Hyatt, I have a question.

RenderPartObject:;layout() calls m_overflow.clear(); addShadowOverflow();

Is it right calling this for flattened iframes?

Also, why is it calling m_overflow.clear and not clearLayoutOverflow() defined in RenderBox?
Comment 37 Antti Koivisto 2010-04-01 05:35:53 PDT
(In reply to comment #34)
> Antti, do you have any input on how to turn off subframe scrollbars on mac?

Hyatt knows about the scrollbars, I'm not sure if anyone else does.
Comment 38 Kenneth Rohde Christiansen 2010-04-01 10:03:00 PDT
The case of a partly visible iframe:

LOAD:

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

RELOAD

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

RELOAD

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400

RELOAD

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 200/62
FrameView::adjustViewSize(): layout overflow : 204/66
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 200/66
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
Comment 39 Kenneth Rohde Christiansen 2010-04-01 10:19:43 PDT
If not visible at all, every load gives:

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 200/15
FrameView::adjustViewSize(): layout overflow : 200/15
FrameView::adjustViewSize(): layout overflow : 200/15
FrameView::adjustViewSize(): layout overflow : 204/19
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 200/19
FrameView::adjustViewSize(): layout overflow : 200/19
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400


If full visible, every load gives:

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

layoutWithFlattening begin
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
layoutWithFlattening end

FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
FrameView::adjustViewSize(): layout overflow : 400/400
Comment 40 Kenneth Rohde Christiansen 2010-04-01 13:02:15 PDT
Created attachment 52324 [details]
Patch (should make the flattening work on the site in question)
Comment 41 Kenneth Rohde Christiansen 2010-04-01 13:02:42 PDT
Greg, could you please test?
Comment 42 Greg Bolsinga 2010-04-01 14:00:24 PDT
Great! With this "Static IFrame" and "STATIC 2" look great.

Static "FRAME5" little frame doesn't float with the oval image however.
Comment 43 Greg Bolsinga 2010-04-05 09:57:39 PDT
<rdar://problem/7674554>
Comment 44 Kenneth Rohde Christiansen 2010-04-05 10:04:31 PDT
(In reply to comment #42)
> Great! With this "Static IFrame" and "STATIC 2" look great.
> 
> Static "FRAME5" little frame doesn't float with the oval image however.

I didn't quite understand the issue. If you can provide a reduced test, I will happily look into fixing it.
Comment 45 Dave Hyatt 2010-04-05 10:06:39 PDT
Comment on attachment 52324 [details]
Patch (should make the flattening work on the site in question)

r=me
Comment 46 Kenneth Rohde Christiansen 2010-04-05 10:09:03 PDT
(In reply to comment #44)
> (In reply to comment #42)
> > Great! With this "Static IFrame" and "STATIC 2" look great.
> > 
> > Static "FRAME5" little frame doesn't float with the oval image however.
> 
> I didn't quite understand the issue. If you can provide a reduced test, I will
> happily look into fixing it.

Do you mean the thing hovering on top of STATIC 2? That also gives me a similar
result on the iPhone OS 2.0
Comment 47 Greg Bolsinga 2010-04-05 10:10:52 PDT
(In reply to comment #46)
> (In reply to comment #44)
> > (In reply to comment #42)
> > > Great! With this "Static IFrame" and "STATIC 2" look great.
> > > 
> > > Static "FRAME5" little frame doesn't float with the oval image however.
> > 
> > I didn't quite understand the issue. If you can provide a reduced test, I will
> > happily look into fixing it.
> 
> Do you mean the thing hovering on top of STATIC 2? That also gives me a similar
> result on the iPhone OS 2.0

Yeah, that is the part, and it is also not centered properly on iPhone. I can file a separate bug if you'd like.
Comment 48 Kenneth Rohde Christiansen 2010-04-05 10:13:06 PDT
Please do.
Comment 49 Greg Bolsinga 2010-04-05 10:17:52 PDT
Bug 37095. Thanks Kenneth!
Comment 50 Kenneth Rohde Christiansen 2010-04-05 10:20:44 PDT
Comment on attachment 52324 [details]
Patch (should make the flattening work on the site in question)

Landed in 57080
Comment 51 Kenneth Rohde Christiansen 2010-04-05 10:37:00 PDT
Updated Qt iframe results committed in 57083
Comment 52 Adam Barth 2010-04-05 11:11:19 PDT
This looks to have regressed a number of iframe flattening tests:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r57081%20(12596)/results.html
Comment 53 Kenneth Rohde Christiansen 2010-04-05 11:12:30 PDT
(In reply to comment #52)
> This looks to have regressed a number of iframe flattening tests:
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r57081%20(12596)/results.html

I will have a look at it. I was expecting this and watching the bots :-)
Comment 54 Adam Barth 2010-04-05 11:13:43 PDT
Would you be willing to come on #webkit to coordinate?  If you expected this to light the tree on fire, why did you land it manually?  The commit-queue could have cannaried it for you without disrupting everyone else.
Comment 55 Greg Bolsinga 2010-04-05 11:55:16 PDT
Also r57086
Comment 56 Kenneth Rohde Christiansen 2010-04-05 12:03:22 PDT
(In reply to comment #54)
> Would you be willing to come on #webkit to coordinate?  If you expected this to
> light the tree on fire, why did you land it manually?  The commit-queue could
> have cannaried it for you without disrupting everyone else.

As discussed on irc, I needed the new results generated by the mac and Qt bot. The Qt bot generate slightly different results that I get locally due to font differences (sucks!).

Having EWS to run the tests and give back the results, would solve the problem.
Comment 57 WebKit Review Bot 2010-04-05 12:10:53 PDT
http://trac.webkit.org/changeset/57080 might have broken Tiger Intel Release
Comment 58 Eric Seidel (no email) 2010-04-05 12:18:14 PDT
Looking at the various commits in the range, this appears to be the one that broke Tiger.
Comment 59 Kenneth Rohde Christiansen 2010-04-05 12:41:33 PDT
(In reply to comment #58)
> Looking at the various commits in the range, this appears to be the one that
> broke Tiger.

Adam Barth is building and testing. The code shouldn't be affecting the non-flattening code path.
Comment 60 Kenneth Rohde Christiansen 2010-04-05 14:35:20 PDT
(In reply to comment #59)
> (In reply to comment #58)
> > Looking at the various commits in the range, this appears to be the one that
> > broke Tiger.
> 
> Adam Barth is building and testing. The code shouldn't be affecting the
> non-flattening code path.

A clean rebuild on Tiger make the failing test work again.
Comment 61 Simon Hausmann 2010-04-06 02:40:30 PDT
<cherry-pick-for-backport: 56855>
Comment 62 Simon Hausmann 2010-04-06 02:40:55 PDT
<cherry-pick-for-backport: 57083>
Comment 63 Simon Hausmann 2010-04-06 02:41:13 PDT
<cherry-pick-for-backport: 57086>
Comment 64 Simon Hausmann 2010-04-06 02:46:02 PDT
Revision r56852 cherry-picked into qtwebkit-2.0 with commit 85def4da93f5509994f4349d4b415263498d1e1d
Revision r56854 cherry-picked into qtwebkit-2.0 with commit 7cbfd1fda76f72bc21d39f5a6a326cb2fff6f05d
Revision r56855 cherry-picked into qtwebkit-2.0 with commit fb15164cd65dd69200199c005a06b9bec6374069
Revision r56856 cherry-picked into qtwebkit-2.0 with commit 59daec93fc7ad1f4c5dbeb88b67aca17d6f4cc3b
Revision r57080 cherry-picked into qtwebkit-2.0 with commit 37453501b2151c33c89a7002fef63e1a3103c7df
Revision r57083 cherry-picked into qtwebkit-2.0 with commit a1eed0c2297d458e938ce8491142045c431301e2
Revision r57086 cherry-picked into qtwebkit-2.0 with commit 732efe4699b61e7e4b71773ff9272eceae28fe8b