Bug 36662 - Add support for iframe flattening
Summary: Add support for iframe flattening
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords: Qt
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-03-26 07:52 PDT by Kenneth Rohde Christiansen
Modified: 2010-03-30 07:10 PDT (History)
6 users (show)

See Also:


Attachments
Patch (38.15 KB, patch)
2010-03-26 07:52 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 2 (should build on mac, win etc) (54.50 KB, patch)
2010-03-26 10:49 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
follow up patch for "patch 2" (3.54 KB, patch)
2010-03-26 11:45 PDT, Csaba Osztrogonác
koivisto: review-
Details | Formatted Diff | Diff
Patch 2b (should build on mac, win etc) (54.50 KB, patch)
2010-03-26 13:01 PDT, Csaba Osztrogonác
koivisto: review-
Details | Formatted Diff | Diff
Patch (56.40 KB, patch)
2010-03-29 06:54 PDT, Kenneth Rohde Christiansen
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 2010-03-26 07:52:34 PDT
Created attachment 51741 [details]
Patch

Add support for expanding iframes to their contents, just as done on the iPhone.
Comment 1 Kenneth Rohde Christiansen 2010-03-26 07:54:00 PDT
After landed we will probably need to rebase the test results with our Qt buildbot.
Comment 2 Simon Hausmann 2010-03-26 07:55:53 PDT
(In reply to comment #1)
> After landed we will probably need to rebase the test results with our Qt
> buildbot.

When you do that, please let me know which revisions for the test result fixes need to be cherry-picked.
Comment 3 Eric Seidel (no email) 2010-03-26 08:00:21 PDT
Attachment 51741 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1323023
Comment 4 Antti Koivisto 2010-03-26 08:11:08 PDT
Comment on attachment 51741 [details]
Patch

Looks good, r=me 

I don't have any really substantial comments just one naming objection:

> -    if (flattenFrameSet())
> +    if (shouldResizeFramesToContent())
>          positionFramesWithFlattening();

Why this renaming? The naming was discussed quite a bit, lets stick with the "flatten" terminology now.

You now have shouldResizeFramesToContent() method in both RenderFrameSet and RenderPartObject (with rather different semantics), even though those are not really related classes. Why do they need to have same names? Why not stick with RenderFrameSet::flattenFrameSet() and add RenderPartObject::flattenFrame() or similar?
Comment 5 Kenneth Rohde Christiansen 2010-03-26 10:49:31 PDT
Created attachment 51760 [details]
Patch 2 (should build on mac, win etc)
Comment 6 Eric Seidel (no email) 2010-03-26 11:02:11 PDT
Attachment 51760 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1444004
Comment 7 Csaba Osztrogonác 2010-03-26 11:45:34 PDT
Created attachment 51767 [details]
follow up patch for "patch 2"
Comment 8 Kenneth Rohde Christiansen 2010-03-26 11:46:46 PDT
Comment on attachment 51767 [details]
follow up patch for "patch 2"

We will put it on the cq when the other land.
Comment 9 Csaba Osztrogonác 2010-03-26 13:01:32 PDT
Created attachment 51773 [details]
Patch 2b (should build on mac, win etc)

Uploaded for Kenneth with Mac fix:
-__ZN7WebCore8Settings28setFrameFlatteningEnabledEb
+__ZN7WebCore8Settings25setFrameFlatteningEnabledEb
Comment 10 Antti Koivisto 2010-03-29 06:52:02 PDT
Comment on attachment 51767 [details]
follow up patch for "patch 2"

Please upload one patch that works without having to patch it again with a second one.
Comment 11 Kenneth Rohde Christiansen 2010-03-29 06:54:07 PDT
Created attachment 51904 [details]
Patch
Comment 12 Kenneth Rohde Christiansen 2010-03-29 08:35:40 PDT
Comment on attachment 51904 [details]
Patch

Landed in 56718
Comment 13 Csaba Osztrogonác 2010-03-29 09:33:46 PDT
(In reply to comment #11)
> Created an attachment (id=51904) [details]
> Patch

Mac expected files updated:
http://trac.webkit.org/changeset/56720
http://trac.webkit.org/changeset/56722
Comment 14 Eric Seidel (no email) 2010-03-29 11:38:05 PDT
Comment on attachment 51741 [details]
Patch

Cleared Antti Koivisto's review+ from obsolete attachment 51741 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 15 Eric Seidel (no email) 2010-03-29 11:38:09 PDT
Comment on attachment 51904 [details]
Patch

Cleared Antti Koivisto's review+ from obsolete attachment 51904 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 16 Simon Hausmann 2010-03-30 05:09:59 PDT
cherry-pick-for-backport: <r56718>
Comment 17 Simon Hausmann 2010-03-30 05:10:23 PDT
cherry-pick-for-backport: <r56720>
Comment 18 Simon Hausmann 2010-03-30 05:10:43 PDT
cherry-pick-for-backport: <r56722>
Comment 19 Simon Hausmann 2010-03-30 07:10:13 PDT
Revision r56718 cherry-picked into qtwebkit-2.0 with commit b74f1de36d3e514dc6e1d6293cd7834286071380
Comment 20 Simon Hausmann 2010-03-30 07:10:25 PDT
Revision r56720 cherry-picked into qtwebkit-2.0 with commit eb76591b64df4c459a9b291a85163f09748a4c98
Comment 21 Simon Hausmann 2010-03-30 07:10:36 PDT
Revision r56722 cherry-picked into qtwebkit-2.0 with commit bb360f64ed44718420472bdcc41a8060e0bfc1b8