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
Are these flattened on the iPhone? I'm currently not flattening fixed sized iframes, but I guess we should do that.
OK, bug found :-) Thanks Greg for the bug report! I will add some more tests and upload a new patch.
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)
Created attachment 52045 [details] Test case
Created attachment 52046 [details] First load
Created attachment 52047 [details] Second load
s/partly visible/partly visible or not visible within the viewport/
Created attachment 52054 [details] Patch for wrong logic
With the patch applied, I do not see the frames expanded. In addition, the "FRAME6" has scrollbars with this patch, but not without it.
(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.
OK. I tried your patch applied to iPhone as well, and saw the same issues there.
I think the problem is that it isn't doing relayout of the owner frame when the contents of the iframes have loaded.
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 on attachment 52054 [details] Patch for wrong logic Progression + nice tests. r=me even if there is need for followups.
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 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
I will land manually and update the results afterward. Ossy has already generated new results for the Qt bots which I have validated.
Created attachment 52172 [details] Scrollbar related patch
Attachment 52172 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1605143
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().
(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.
> > > + 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 on attachment 52054 [details] Patch for wrong logic Landed in r56852
Comment on attachment 52172 [details] Scrollbar related patch Fixed patch landed in r56854
(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
See Bug 36894
Comment 9 still applies with this patch applied and with the crasher removed.
(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?
http://trac.webkit.org/changeset/56854 appears to have broken Leopard Intel Debug (Tests)
(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
(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).
Created attachment 52201 [details] screen shot _after_ a resize of the window.
Created attachment 52202 [details] screen shot _before_ a resize of the window.
(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?
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.
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?
(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.
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
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
Created attachment 52324 [details] Patch (should make the flattening work on the site in question)
Greg, could you please test?
Great! With this "Static IFrame" and "STATIC 2" look great. Static "FRAME5" little frame doesn't float with the oval image however.
<rdar://problem/7674554>
(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 on attachment 52324 [details] Patch (should make the flattening work on the site in question) r=me
(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
(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.
Please do.
Bug 37095. Thanks Kenneth!
Comment on attachment 52324 [details] Patch (should make the flattening work on the site in question) Landed in 57080
Updated Qt iframe results committed in 57083
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
(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 :-)
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.
Also r57086
(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.
http://trac.webkit.org/changeset/57080 might have broken Tiger Intel Release
Looking at the various commits in the range, this appears to be the one that broke Tiger.
(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.
(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.
<cherry-pick-for-backport: 56855>
<cherry-pick-for-backport: 57083>
<cherry-pick-for-backport: 57086>
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