Description
Kenneth Rohde Christiansen
2009-12-18 09:30:37 PST
Created attachment 45157 [details]
Part 1
Created attachment 46072 [details]
Fix some issues that occurred while adding layout test
Created attachment 46073 [details]
Work on adding layout tests
(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 46142 [details]
Add an additional test that currently fails
Created attachment 46820 [details]
Frameset Flattening with layout tests
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.
Created attachment 46903 [details]
Path with 5 test which all pass on Qt
Comment on attachment 46903 [details]
Path with 5 test which all pass on Qt
Do not enable the setting by default, so r- myself
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.
Created attachment 46904 [details]
Patch with 5 test which all pass on Qt
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. fast/frames/flattening/ should probably be added to the skiplists of the other ports, until their DRT supports toggling the setting. I think frame set flattening is a fairly descriptive name, I wouldn't even mind the attribute in QWebSettings :) Ok, ok. :) 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. Hyatt, do you have any comments? 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 on attachment 46904 [details] Patch with 5 test which all pass on Qt Landed in r54440. 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 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. Created attachment 48489 [details]
FrameSet Flattening enabling/disabling on Mac port.
Created attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files
Created attachment 48491 [details]
FrameSet Flattening enabling/disabling on Win port
Comment on attachment 48490 [details]
FrameSet Flattening support on Mac DRT and expected files
This patch depends on the previous one.
Comment on attachment 48489 [details]
FrameSet Flattening enabling/disabling on Mac port.
Your entry in the changelog has to be the first, sorry
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-
Created attachment 48493 [details]
FrameSet Flattening enabling/disabling on Mac port - v2
fixed changelog entry
Comment on attachment 48493 [details]
FrameSet Flattening enabling/disabling on Mac port - v2
You are removing ChangeLog entries. r- for this reason
Created attachment 48498 [details]
FrameSet Flattening enabling/disabling on Mac port - v3
sorry for the previous ChangeLog madness
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 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 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> All reviewed patches have been landed. Closing bug. (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 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.
Created attachment 48575 [details]
FrameSet Flattening support on Mac, Win, Gtk and Wx DRT and expected files for Mac - v2
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 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> All reviewed patches have been landed. Closing bug. |