Bug 85843 - [GTK] ENABLE_IFRAME_SEAMLESS support
Summary: [GTK] ENABLE_IFRAME_SEAMLESS support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-07 16:29 PDT by Philippe Normand
Modified: 2012-05-10 13:09 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.32 KB, patch)
2012-05-10 10:27 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2012-05-10 11:35 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2012-05-10 11:50 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-05-07 16:29:12 PDT
Since r116356 seamless frames support is optional and disabled in GTK. So the following tests fail:

Regressions: Unexpected text diff mismatch : (10)
  fast/frames/seamless/seamless-form-get-named.html = TEXT
  fast/frames/seamless/seamless-form-get.html = TEXT
  fast/frames/seamless/seamless-form-post-named.html = TEXT
  fast/frames/seamless/seamless-form-post.html = TEXT
  fast/frames/seamless/seamless-hyperlink-named.html = TEXT
  fast/frames/seamless/seamless-hyperlink.html = TEXT
  fast/frames/seamless/seamless-window-location-href.html = TEXT
  fast/frames/seamless/seamless-window-location-sandbox.html = TEXT
  fast/frames/seamless/seamless-window-location.html = TEXT
  fast/frames/seamless/seamless-window-open.html = TEXT

I'll flag those in test_expectations for now but I think we should simply enable the feature in GTK. Any thoughts, Martin?
Comment 1 Martin Robinson 2012-05-08 11:21:12 PDT
Adam, do you think it's safe to start shipping this feature in unstable releases and then in a stable release in about 6 months?
Comment 2 Adam Barth 2012-05-08 16:37:01 PDT
> Adam, do you think it's safe to start shipping this feature in unstable releases and then in a stable release in about 6 months?

Yes.  Eric is still landing some pieces of the feature, but six months is plenty of time to bake.
Comment 3 Martin Robinson 2012-05-08 16:46:03 PDT
Okay. I'd say it's quite safe to turn this feature on. it carries a lot less risk than some of our API developer facing features and we'll be able to test it more thoroughly by turning it on now.
Comment 4 Zan Dobersek 2012-05-10 10:27:42 PDT
Created attachment 141195 [details]
Patch
Comment 5 Zan Dobersek 2012-05-10 10:29:31 PDT
(In reply to comment #4)
> Created an attachment (id=141195) [details]
> Patch

This patch adds a configuration option in configure.ac to enable this feature, but is disabled by default. The feature is enabled by default in build-webkit, though.

I can however enable this feature as well in configure.ac as the comment #3 hints.
Comment 6 Martin Robinson 2012-05-10 11:22:58 PDT
Comment on attachment 141195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141195&action=review

> Tools/Scripts/webkitperl/FeatureList.pm:196
> +      define => "ENABLE_ICONDATABASE", default => isGtk(), value => \$iframeSeamlessSupport },

Looks like the define is wrong here. It should probably be ENABLE_IFRAME_SEAMLESS.
Comment 7 Zan Dobersek 2012-05-10 11:34:11 PDT
(In reply to comment #6)
> (From update of attachment 141195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141195&action=review
> 
> > Tools/Scripts/webkitperl/FeatureList.pm:196
> > +      define => "ENABLE_ICONDATABASE", default => isGtk(), value => \$iframeSeamlessSupport },
> 
> Looks like the define is wrong here. It should probably be ENABLE_IFRAME_SEAMLESS.

Caught red-handed :>

A proper patch incoming.
Comment 8 Zan Dobersek 2012-05-10 11:35:44 PDT
Created attachment 141209 [details]
Patch
Comment 9 Eric Seidel (no email) 2012-05-10 11:43:06 PDT
Comment on attachment 141209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141209&action=review

> Tools/Scripts/webkitperl/FeatureList.pm:196
> +      define => "ENABLE_IFRAME_SEAMLESS", default => isGtk(), value => \$iframeSeamlessSupport },

should still be default => 1

> configure.ac:742
> +              [],[enable_iframe_seamless="no"])

I stillt hink this should defatult to yes.
Comment 10 Zan Dobersek 2012-05-10 11:45:49 PDT
Comment on attachment 141209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141209&action=review

>> Tools/Scripts/webkitperl/FeatureList.pm:196
>> +      define => "ENABLE_IFRAME_SEAMLESS", default => isGtk(), value => \$iframeSeamlessSupport },
> 
> should still be default => 1

Given that Mac port showed some desire for an option to ship with this feature disabled, I guess this shouldn't affect them?

>> configure.ac:742
>> +              [],[enable_iframe_seamless="no"])
> 
> I stillt hink this should defatult to yes.

Figures.
Comment 11 Zan Dobersek 2012-05-10 11:50:21 PDT
Created attachment 141213 [details]
Patch
Comment 12 Eric Seidel (no email) 2012-05-10 11:55:19 PDT
Comment on attachment 141213 [details]
Patch

build-webkit option defaults are wildly inaccurate until I fix bug 85456 anyway. :)  If you're interested in this topic, we still need someone to split the Gtk feature defaults out into some easily autogenerated file to fix that bug!
Comment 13 WebKit Review Bot 2012-05-10 13:09:38 PDT
Comment on attachment 141213 [details]
Patch

Clearing flags on attachment: 141213

Committed r116679: <http://trac.webkit.org/changeset/116679>
Comment 14 WebKit Review Bot 2012-05-10 13:09:44 PDT
All reviewed patches have been landed.  Closing bug.