Bug 32717 – Support frameset flattening

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

Created attachment 45157 [details]
Part 1

[Kenneth Rohde Christiansen]

Created attachment 46072 [details]
Fix some issues that occurred while adding layout test

[Kenneth Rohde Christiansen]

Created attachment 46073 [details]
Work on adding layout tests

[Antonio Gomes]

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

[Kenneth Rohde Christiansen]

Created attachment 46142 [details]
Add an additional test that currently fails

[Kenneth Rohde Christiansen]

Created attachment 46820 [details]
Frameset Flattening with layout tests

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

Created attachment 46903 [details]
Path with 5 test which all pass on Qt

[Kenneth Rohde Christiansen]

Comment on attachment 46903 [details]
Path with 5 test which all pass on Qt

Do not enable the setting by default, so r- myself

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

Created attachment 46904 [details]
Patch with 5 test which all pass on Qt

[Antti Koivisto]

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.

[Kenneth Rohde Christiansen]

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

[Simon Hausmann]

fast/frames/flattening/ should probably be added to the skiplists of the other ports, until their DRT supports toggling the setting.

[Simon Hausmann]

I think frame set flattening is a fairly descriptive name, I wouldn't even mind the attribute in QWebSettings :)

[Antti Koivisto]

Ok, ok. :)

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Kenneth Rohde Christiansen]

Hyatt, do you have any comments?

[Dave Hyatt]

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.

[Kenneth Rohde Christiansen]

Comment on attachment 46904 [details]
Patch with 5 test which all pass on Qt

Landed in r54440.

[Kenneth Rohde Christiansen]

I will do some follow up patches next week, so I'll leave this bug open now that we have so many cc'ed.

[Eric Seidel (no email)]

Comment on attachment 46904 [details]
Patch with 5 test which all pass on Qt

Cleared David Hyatt's review+ from obsolete attachment 46904 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Jesus Sanchez-Palencia]

Created attachment 48489 [details]
FrameSet Flattening enabling/disabling on Mac port.

[Jesus Sanchez-Palencia]

Created attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files

[Jesus Sanchez-Palencia]

Created attachment 48491 [details]
FrameSet Flattening enabling/disabling on Win port

[Jesus Sanchez-Palencia]

Comment on attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files

This patch depends on the previous one.

[Kenneth Rohde Christiansen]

Comment on attachment 48489 [details]
FrameSet Flattening enabling/disabling on Mac port.

Your entry in the changelog has to be the first, sorry

[Kenneth Rohde Christiansen]

Comment on attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files

cq- as it depends on the former patch which was r-

[Jesus Sanchez-Palencia]

Created attachment 48493 [details]
FrameSet Flattening enabling/disabling on Mac port - v2

fixed changelog entry

[Kenneth Rohde Christiansen]

Comment on attachment 48493 [details]
FrameSet Flattening enabling/disabling on Mac port - v2

You are removing ChangeLog entries. r- for this reason

[Jesus Sanchez-Palencia]

Created attachment 48498 [details]
FrameSet Flattening enabling/disabling on Mac port - v3

sorry for the previous ChangeLog madness

[WebKit Commit Bot]

Comment on attachment 48491 [details]
FrameSet Flattening enabling/disabling on Win port

Clearing flags on attachment: 48491

Committed r54606: <http://trac.webkit.org/changeset/54606>

[WebKit Commit Bot]

Comment on attachment 48498 [details]
FrameSet Flattening enabling/disabling on Mac port - v3

Clearing flags on attachment: 48498

Committed r54613: <http://trac.webkit.org/changeset/54613>

[WebKit Commit Bot]

Comment on attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files

Clearing flags on attachment: 48490

Committed r54626: <http://trac.webkit.org/changeset/54626>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #35)
> (From update of attachment 48490 [details])
> Clearing flags on attachment: 48490
> 
> Committed r54626: <http://trac.webkit.org/changeset/54626>

This patch broke GTK and Win build, so it was rolled out by http://trac.webkit.org/changeset/54628 .

http://build.webkit.org/builders/GTK%20Linux%20Release/builds/8711/steps/compile-webkit/logs/stdio
http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/10095/steps/compile-webkit/logs/stdio

[Jesus Sanchez-Palencia]

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.

[Jesus Sanchez-Palencia]

Created attachment 48575 [details]
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac - v2

[Kenneth Rohde Christiansen]

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.

[WebKit Commit Bot]

Comment on attachment 48575 [details]
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac - v2

Clearing flags on attachment: 48575

Committed r54719: <http://trac.webkit.org/changeset/54719>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.