Bug 106213 - Make the IFRAME_SEAMLESS flag runtime-enabled.
Summary: Make the IFRAME_SEAMLESS flag runtime-enabled.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 106252
  Show dependency treegraph
 
Reported: 2013-01-07 01:02 PST by Mike West
Modified: 2013-01-09 00:59 PST (History)
14 users (show)

See Also:


Attachments
Patch (8.32 KB, patch)
2013-01-07 01:08 PST, Mike West
no flags Details | Formatted Diff | Diff
Cargo culting. (29.57 KB, patch)
2013-01-07 05:11 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (30.62 KB, patch)
2013-01-07 14:14 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-01-07 01:02:50 PST
Make the IFRAME_SEAMLESS flag runtime-enabled.
Comment 1 Mike West 2013-01-07 01:08:01 PST
Created attachment 181486 [details]
Patch
Comment 2 Mike West 2013-01-07 01:09:57 PST
Hi Eric! We talked last week about hiding IFRAME_SEAMLESS behind a runtime flag to ease the burden on Chromium's release team. This patch takes care of the WebKit side of that.

+hyatt, as this might interest you as well.
Comment 3 WebKit Review Bot 2013-01-07 01:10:46 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Mike West 2013-01-07 02:23:57 PST
Hrm. I was a bit too quick to upload a patch. I'm cargo-culting my way to (hopefully) successfully enabling the flag for the various ports' DRT; new patch shortly.
Comment 5 Mike West 2013-01-07 05:11:28 PST
Created attachment 181495 [details]
Cargo culting.
Comment 6 Mike West 2013-01-07 05:12:03 PST
I'd love someone to take a look at this, especially the mac bits. I'm totally guessing. :)
Comment 7 Eric Seidel (no email) 2013-01-07 09:29:42 PST
What's the difference between a runtime feature and a Setting?  Settings are per page? Features are global?
Comment 8 Mike West 2013-01-07 09:35:43 PST
(In reply to comment #7)
> What's the difference between a runtime feature and a Setting?  Settings are per page? Features are global?

That's my vague understanding, yes. Adam pointed me to RuntimeEnabledFeatures for our work on CSP 1.1; it seems applicable here as well. :)
Comment 9 Ojan Vafai 2013-01-07 09:51:52 PST
Comment on attachment 181495 [details]
Cargo culting.

We have too many ways of doing runtime feature enabling. That's not this patch's fault though.
Comment 10 Adam Barth 2013-01-07 11:56:33 PST
> We have too many ways of doing runtime feature enabling.

Yeah.

> What's the difference between a runtime feature and a Setting?  Settings are per page? Features are global?

Pretty much.  Also, Settings can be changed over the lifetime of the process and often over the lifetime of a given Page.  Runtime features are supposed to be initialized at startup and then never changed again.

The original reason we added the global runtime features is that we wanted to hide JavaScript API completely at runtime.  In order to do that, we needed to know the value of the setting when creating the ObjectTemplates, which, once created, cannot be changed.
Comment 11 Adam Barth 2013-01-07 13:21:37 PST
Comment on attachment 181495 [details]
Cargo culting.

API change LGTM
Comment 12 WebKit Review Bot 2013-01-07 13:47:03 PST
Comment on attachment 181495 [details]
Cargo culting.

Clearing flags on attachment: 181495

Committed r138983: <http://trac.webkit.org/changeset/138983>
Comment 13 WebKit Review Bot 2013-01-07 13:47:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Csaba Osztrogonác 2013-01-07 14:05:02 PST
(In reply to comment #12)
> (From update of attachment 181495 [details])
> Clearing flags on attachment: 181495
> 
> Committed r138983: <http://trac.webkit.org/changeset/138983>

It broke the !ENABLE_IFRAME_SEAMLESS builds:

/ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static void DumpRenderTreeSupportQt::setSeamlessIFramesEnabled(bool)':
/ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1055: error: 'setSeamlessIFramesEnabled' is not a member of 'WebCore::RuntimeEnabledFeatures'
Comment 15 Mike West 2013-01-07 14:13:01 PST
(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 181495 [details] [details])
> > Clearing flags on attachment: 181495
> > 
> > Committed r138983: <http://trac.webkit.org/changeset/138983>
> 
> It broke the !ENABLE_IFRAME_SEAMLESS builds:
> 
> /ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static void DumpRenderTreeSupportQt::setSeamlessIFramesEnabled(bool)':
> /ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1055: error: 'setSeamlessIFramesEnabled' is not a member of 'WebCore::RuntimeEnabledFeatures'

Hrm. It passed both the qt and qt-wk2 EWS bots. :(

I think the fix should be straightforward though. I'll have a patch up shortly.
Comment 16 Antoine Quint 2013-01-07 14:14:39 PST
Reopening to attach new patch.
Comment 17 Antoine Quint 2013-01-07 14:14:42 PST
Created attachment 181562 [details]
Patch
Comment 18 Antoine Quint 2013-01-07 14:18:52 PST
A mishapp with webkit-patch caused me to upload a patch on this bug meant for another bug, sorry.
Comment 19 Mike West 2013-01-07 14:20:04 PST
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (From update of attachment 181495 [details] [details] [details])
> > > Clearing flags on attachment: 181495
> > > 
> > > Committed r138983: <http://trac.webkit.org/changeset/138983>
> > 
> > It broke the !ENABLE_IFRAME_SEAMLESS builds:
> > 
> > /ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp: In static member function 'static void DumpRenderTreeSupportQt::setSeamlessIFramesEnabled(bool)':
> > /ramdisk/qt-linux-release-minimal/build/Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1055: error: 'setSeamlessIFramesEnabled' is not a member of 'WebCore::RuntimeEnabledFeatures'
> 
> Hrm. It passed both the qt and qt-wk2 EWS bots. :(
> 
> I think the fix should be straightforward though. I'll have a patch up shortly.

https://bugs.webkit.org/show_bug.cgi?id=106252
Comment 20 Roger Fong 2013-01-07 16:07:39 PST
Is this feature supported on Windows?
I'm seeing 27 seamless frame tests failing on the Apple Win 7test bots.
http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r138986%20(31310)/results.html
Comment 21 Eric Seidel (no email) 2013-01-07 16:13:26 PST
There is nothing windows-specific about this (9mo) feature. :)  It's just a question of if it was ever enabled in the vsproj or not.
Comment 22 Roger Fong 2013-01-08 17:18:14 PST
(In reply to comment #21)
> There is nothing windows-specific about this (9mo) feature. :)  It's just a question of if it was ever enabled in the vsproj or not.

The feature flag has been enabled but it looks like none of the webpreferences have been so Runtime enabled features flag will be false,
which would cause Document::shouldDisplaySeamlesslyWithParent to return false.

I'll go ahead and add the extra layer.