PS: Another bug will track iframe flattening.
Subframe flattening basically make framesets usable on touch devices by flattening the frames to the size of the contents. Some special case it done to obey the frameset rules as much as possible and thus not break the layout.
The original patch was written for the S60, and later ported to the iPhone.
I'm going to:
1) Extract the frameset flattening code done by Antti
1) Add supports to DRT for testing + add layout tests
2) Improve the code (there are some obvious areas of improvements)
3) Verify agains the patches in Android and (?) webOS.
4) Consider a better algorithm.
before putting this up to review.
Most of this is ongoing, but as I'm going on vacation for two weeks, I will attach the first patch, extracing the frameset flattening code by Antti, modified to work on top on trunk.
(In reply to comment #3)
> Created an attachment (id=46073) [details]
> Work on adding layout tests
should not this new test be in Skipped of other ports ?
Created attachment 46825[details]
Change behaviour after discussion w/ Antti
if scrollbars are off, and the width or height are fixed we obey them and do not expand. The reasoning is that with frame flattening no subframe much ever become scrollable, so either scrolling must be disabled or the frame must be expanded.
Attachment 46903[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring WebKit/qt/Api/qwebpage_p.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebpage.cpp; This file is exempt from the style guide.
WebCore/page/Settings.cpp:97: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1
If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks for cleaning this up, with nice test cases too!
I wonder if we could come up with a better name for the feature than "frame flattening" (and as a result, for all names in the code that use it). It has been used for a long time but it is not very descriptive. "Expanding framesets to content" perhaps. How would
Settings::expandsFramesetsToContent()
for example sound?
Since this patch does not handle iframes, are you planning to have a separate feature flag for those?
Some high level comments on the algorithm would be nice in the code (since you have figured it out), RenderFrameSet::positionFramesWithFlattening() mostly.
(In reply to comment #12)
> Thanks for cleaning this up, with nice test cases too!
>
> I wonder if we could come up with a better name for the feature than "frame
> flattening" (and as a result, for all names in the code that use it). It has
> been used for a long time but it is not very descriptive. "Expanding framesets
> to content" perhaps. How would
>
> Settings::expandsFramesetsToContent()
>
> for example sound?
I don't think that makes it much clearer. Anyway, when we do a Qt API, we can always use another name, though I think FrameSet Flattening is sufficient as it's behaviour will have to be documented anyway.
It is also easier to talk about frameset/iframe flattening as a technique instead of expanding framesets/iframes to their contents.
> Since this patch does not handle iframes, are you planning to have a separate
> feature flag for those?
Yes
> Some high level comments on the algorithm would be nice in the code (since you
> have figured it out), RenderFrameSet::positionFramesWithFlattening() mostly.
I can do that in a follow up patch if you would like.
Comment on attachment 46904[details]
Patch with 5 test which all pass on Qt
Looks good to me, but it would be good for someone else take a look too. Some this code was written by me so I'm not the ideal reviewer.
I guess the new tests should go the skip lists of architectures that lack the DRT support.
You should add appropriate copyright statements to the files.
Comment on attachment 46904[details]
Patch with 5 test which all pass on Qt
> + void layoutWithFlattening(bool fixedWidth, bool fixedHeight);
It would be better to use enum here (as bit mask). Something like layoutWithFlattening(false, false) is pretty uninformative.
Comment on attachment 46904[details]
Patch with 5 test which all pass on Qt
In the FrameView code:
"/ In flat frame layout mode the content of frame affects layout of the parent frames."
Change that to "the contents of the frame"
"Invalidate also parent frame starting from"
Change to:
"Also invalidate the parent frame starting from"
In RenderFrame:
"NOTE: width and height has been set at this point by"
Change to:
"NOTE: The width and height have been set at this point by"
I think this would be useful to test on Mac as well if you wanted to add the setting and patch more layout test controllers. That could happen in a followup though.
r=me with those comments fixed up.
Created attachment 48573[details]
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac
This is the only patch missing review and commit, the others were landed.
This shouldn't break Win and GTK ports anymore.
Comment on attachment 48575[details]
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac - v2
Will request review under my name, so that it is tested on the build bots. Jesus is not a committer and there for it doesn't run the tests.
2009-12-18 09:32 PST, Kenneth Rohde Christiansen
2010-01-07 12:25 PST, Kenneth Rohde Christiansen
2010-01-07 12:25 PST, Kenneth Rohde Christiansen
2010-01-08 10:03 PST, Kenneth Rohde Christiansen
2010-01-18 07:00 PST, Kenneth Rohde Christiansen
2010-01-18 07:52 PST, Kenneth Rohde Christiansen
2010-01-19 06:28 PST, Kenneth Rohde Christiansen
2010-01-19 06:38 PST, Kenneth Rohde Christiansen
2010-02-10 05:34 PST, Jesus Sanchez-Palencia
2010-02-10 05:35 PST, Jesus Sanchez-Palencia
2010-02-10 05:36 PST, Jesus Sanchez-Palencia
2010-02-10 05:54 PST, Jesus Sanchez-Palencia
2010-02-10 06:18 PST, Jesus Sanchez-Palencia
2010-02-11 09:26 PST, Jesus Sanchez-Palencia
2010-02-11 09:48 PST, Jesus Sanchez-Palencia