Bug 32717 - Support frameset flattening
Summary: Support frameset flattening
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-18 09:30 PST by Kenneth Rohde Christiansen
Modified: 2010-02-12 04:56 PST (History)
15 users (show)

See Also:


Attachments
Part 1 (14.06 KB, patch)
2009-12-18 09:32 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Fix some issues that occurred while adding layout test (14.54 KB, patch)
2010-01-07 12:25 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Work on adding layout tests (9.47 KB, patch)
2010-01-07 12:25 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Add an additional test that currently fails (11.78 KB, patch)
2010-01-08 10:03 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Frameset Flattening with layout tests (35.12 KB, patch)
2010-01-18 07:00 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Change behaviour after discussion w/ Antti (35.17 KB, patch)
2010-01-18 07:52 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Path with 5 test which all pass on Qt (47.25 KB, patch)
2010-01-19 06:28 PST, Kenneth Rohde Christiansen
kenneth: review-
Details | Formatted Diff | Diff
Patch with 5 test which all pass on Qt (47.91 KB, patch)
2010-01-19 06:38 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
FrameSet Flattening enabling/disabling on Mac port. (6.46 KB, patch)
2010-02-10 05:34 PST, Jesus Sanchez-Palencia
kenneth: review-
Details | Formatted Diff | Diff
FrameSet Flattening support on Mac DRT and expected files (22.75 KB, patch)
2010-02-10 05:35 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
FrameSet Flattening enabling/disabling on Win port (6.09 KB, patch)
2010-02-10 05:36 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
FrameSet Flattening enabling/disabling on Mac port - v2 (47.49 KB, patch)
2010-02-10 05:54 PST, Jesus Sanchez-Palencia
kenneth: review-
Details | Formatted Diff | Diff
FrameSet Flattening enabling/disabling on Mac port - v3 (6.25 KB, patch)
2010-02-10 06:18 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac (26.96 KB, patch)
2010-02-11 09:26 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac - v2 (26.97 KB, patch)
2010-02-11 09:48 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2009-12-18 09:30:37 PST
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.
Comment 1 Kenneth Rohde Christiansen 2009-12-18 09:32:01 PST
Created attachment 45157 [details]
Part 1
Comment 2 Kenneth Rohde Christiansen 2010-01-07 12:25:08 PST
Created attachment 46072 [details]
Fix some issues that occurred while adding layout test
Comment 3 Kenneth Rohde Christiansen 2010-01-07 12:25:33 PST
Created attachment 46073 [details]
Work on adding layout tests
Comment 4 Antonio Gomes 2010-01-07 12:37:30 PST
(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 ?
Comment 5 Kenneth Rohde Christiansen 2010-01-08 10:03:34 PST
Created attachment 46142 [details]
Add an additional test that currently fails
Comment 6 Kenneth Rohde Christiansen 2010-01-18 07:00:58 PST
Created attachment 46820 [details]
Frameset Flattening with layout tests
Comment 7 Kenneth Rohde Christiansen 2010-01-18 07:52:27 PST
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.
Comment 8 Kenneth Rohde Christiansen 2010-01-19 06:28:18 PST
Created attachment 46903 [details]
Path with 5 test which all pass on Qt
Comment 9 Kenneth Rohde Christiansen 2010-01-19 06:29:22 PST
Comment on attachment 46903 [details]
Path with 5 test which all pass on Qt

Do not enable the setting by default, so r- myself
Comment 10 WebKit Review Bot 2010-01-19 06:33:24 PST
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.
Comment 11 Kenneth Rohde Christiansen 2010-01-19 06:38:34 PST
Created attachment 46904 [details]
Patch with 5 test which all pass on Qt
Comment 12 Antti Koivisto 2010-01-19 07:11:15 PST
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.
Comment 13 Kenneth Rohde Christiansen 2010-01-20 03:06:46 PST
(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 14 Simon Hausmann 2010-01-21 04:02:12 PST
fast/frames/flattening/ should probably be added to the skiplists of the other ports, until their DRT supports toggling the setting.
Comment 15 Simon Hausmann 2010-01-25 06:32:18 PST
I think frame set flattening is a fairly descriptive name, I wouldn't even mind the attribute in QWebSettings :)
Comment 16 Antti Koivisto 2010-01-25 06:42:04 PST
Ok, ok. :)
Comment 17 Antti Koivisto 2010-01-27 02:12:10 PST
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 18 Antti Koivisto 2010-01-27 02:19:46 PST
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 19 Kenneth Rohde Christiansen 2010-01-31 13:32:59 PST
Hyatt, do you have any comments?
Comment 20 Dave Hyatt 2010-02-05 12:10:08 PST
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.
Comment 21 Kenneth Rohde Christiansen 2010-02-05 12:48:24 PST
Comment on attachment 46904 [details]
Patch with 5 test which all pass on Qt

Landed in r54440.
Comment 22 Kenneth Rohde Christiansen 2010-02-05 12:49:44 PST
I will do some follow up patches next week, so I'll leave this bug open now that we have so many cc'ed.
Comment 23 Eric Seidel (no email) 2010-02-08 15:12:01 PST
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.
Comment 24 Jesus Sanchez-Palencia 2010-02-10 05:34:39 PST
Created attachment 48489 [details]
FrameSet Flattening enabling/disabling on Mac port.
Comment 25 Jesus Sanchez-Palencia 2010-02-10 05:35:31 PST
Created attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files
Comment 26 Jesus Sanchez-Palencia 2010-02-10 05:36:14 PST
Created attachment 48491 [details]
FrameSet Flattening enabling/disabling on Win port
Comment 27 Jesus Sanchez-Palencia 2010-02-10 05:37:10 PST
Comment on attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files

This patch depends on the previous one.
Comment 28 Kenneth Rohde Christiansen 2010-02-10 05:38:04 PST
Comment on attachment 48489 [details]
FrameSet Flattening enabling/disabling on Mac port.

Your entry in the changelog has to be the first, sorry
Comment 29 Kenneth Rohde Christiansen 2010-02-10 05:39:04 PST
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-
Comment 30 Jesus Sanchez-Palencia 2010-02-10 05:54:28 PST
Created attachment 48493 [details]
FrameSet Flattening enabling/disabling on Mac port - v2

fixed changelog entry
Comment 31 Kenneth Rohde Christiansen 2010-02-10 05:56:21 PST
Comment on attachment 48493 [details]
FrameSet Flattening enabling/disabling on Mac port - v2

You are removing ChangeLog entries. r- for this reason
Comment 32 Jesus Sanchez-Palencia 2010-02-10 06:18:07 PST
Created attachment 48498 [details]
FrameSet Flattening enabling/disabling on Mac port - v3

sorry for the previous ChangeLog madness
Comment 33 WebKit Commit Bot 2010-02-10 10:11:44 PST
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>
Comment 34 WebKit Commit Bot 2010-02-10 11:44:13 PST
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>
Comment 35 WebKit Commit Bot 2010-02-10 15:23:12 PST
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>
Comment 36 WebKit Commit Bot 2010-02-10 15:23:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Jesus Sanchez-Palencia 2010-02-11 09:26:34 PST
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 39 Jesus Sanchez-Palencia 2010-02-11 09:48:28 PST
Created attachment 48575 [details]
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac - v2
Comment 40 Kenneth Rohde Christiansen 2010-02-11 10:15:45 PST
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.
Comment 41 WebKit Commit Bot 2010-02-12 04:56:00 PST
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>
Comment 42 WebKit Commit Bot 2010-02-12 04:56:12 PST
All reviewed patches have been landed.  Closing bug.