Bug 101192 - [CSSRegions] when WebKit uses V8, there should be a single variable to store if the CSS Regions feature is enabled
Summary: [CSSRegions] when WebKit uses V8, there should be a single variable to store ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 94070
  Show dependency treegraph
 
Reported: 2012-11-05 01:23 PST by Mihai Maerean
Modified: 2012-11-30 00:49 PST (History)
14 users (show)

See Also:


Attachments
Patch (9.50 KB, patch)
2012-11-12 02:37 PST, Mihai Maerean
abarth: review-
peter+ews: commit-queue-
Details | Formatted Diff | Diff
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures. (28.94 KB, patch)
2012-11-28 04:35 PST, Mihai Maerean
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op. (29.62 KB, patch)
2012-11-28 07:44 PST, Mihai Maerean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Maerean 2012-11-05 01:23:10 PST
WebCore/page/Settings.h has a bit called m_cssRegionsEnabled that stores whether the CSS Regions feature is enabled at runtime.

When WebKit uses V8, there's also WebCore/bindings/generic/RuntimeEnabledFeatures that has a static bool isCSSRegionsEnabled.

Expected:
There should be a single place to store if CSS Regions is enabled.
Comment 1 Mihai Maerean 2012-11-12 02:37:59 PST
Created attachment 173595 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-12 02:41:44 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 3 Hajime Morrita 2012-11-12 02:46:40 PST
Comment on attachment 173595 [details]
Patch

Looks good. Please ask tkent for a rubberstamp.
Comment 4 Peter Beverloo (cr-android ews) 2012-11-12 03:09:29 PST
Comment on attachment 173595 [details]
Patch

Attachment 173595 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14804575
Comment 5 WebKit Review Bot 2012-11-12 07:37:25 PST
Comment on attachment 173595 [details]
Patch

Attachment 173595 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14818161
Comment 6 Adam Barth 2012-11-12 09:48:10 PST
Comment on attachment 173595 [details]
Patch

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

This isn't how we normally handle these cases.  Why not just have Chromium use WebSettings to enable Regions?

> Source/WebCore/page/Settings.cpp:218
> -#if ENABLE(CSS_REGIONS)
> +#if ENABLE(CSS_REGIONS) && !USE(V8)

We generally don't have USE(V8) ifdefs in Settings.cpp....
Comment 7 Kent Tamura 2012-11-13 02:16:36 PST
Comment on attachment 173595 [details]
Patch

Do you want to add V8EnabledAtRuntime to CSSRegions-related IDL attributes?
Comment 8 Mihai Maerean 2012-11-13 02:26:32 PST
(In reply to comment #7)
> (From update of attachment 173595 [details])
> Do you want to add V8EnabledAtRuntime to CSSRegions-related IDL attributes?

Yes, I will add this kind of changes in the IDL files (Element, Document etc):
-    [Conditional=CSS_REGIONS] readonly attribute DOMString webkitRegionOverset;
-    [Conditional=CSS_REGIONS] sequence<Range> webkitGetRegionFlowRanges();
+    [Conditional=CSS_REGIONS, V8EnabledAtRuntime=cssRegions] readonly attribute DOMString webkitRegionOverset;
+    [Conditional=CSS_REGIONS, V8EnabledAtRuntime=cssRegions] sequence<Range> webkitGetRegionFlowRanges();

in order to fix https://bugs.webkit.org/show_bug.cgi?id=94070 [CSSRegions] broken feature detection: syntax parsing succeeds when ENABLE_CSS_REGIONS is false
Comment 9 Kent Tamura 2012-11-13 02:34:44 PST
ok.  So, the patch shouldn't have Settings.{cpp.h} changes.  JSC platforms can't have a runtime flag for CSS Regions.
Comment 10 Mihai Maerean 2012-11-13 05:57:46 PST
(In reply to comment #9)
> ok.  So, the patch shouldn't have Settings.{cpp.h} changes.  JSC platforms can't have a runtime flag for CSS Regions.

While developing the CSS Regions feature, it is needed that the CSS Regions code is built for the build bots of the JSC platforms so that the code doesn't get broken accidentally.

The JSC platforms must have a way to disable the CSS Regions code at runtime. This is the purpose of the cssRegionsEnabled property inside Settings (which is set to False by default).

For the platforms (like Chromium) that have WebRuntimeFeatures and RuntimeEnabledFeatures, there must be a property for the CSS Regions feature inside RuntimeEnabledFeatures like for the other features that are present in RuntimeEnabledFeatures. This way, the feature can be enabled by using the "experimental WebKit features" switch in chrome://flags/.

For Chromium, there would be 2 variables (one in Settings and one static variable in RuntimeEnabledFeatures)  storing whether the CSS Regions should be active at run-time. These 2 variables can become out of sync and cause bugs.

This bug (101192) is meant to fix this by changing Settings to point to RuntimeEnabledFeatures when RuntimeEnabledFeatures is available.

This is why there are changes in Settings.{cpp.h}. 

My question is what should we use instead of USE(V8) since that is not desired (see comment #6) and also because it doesn't build on cr-android?
Comment 11 Kent Tamura 2012-11-13 06:09:29 PST
I don't think introducing two flags is a correct solution.

How to implement the runtime switch for JSC?  I think a straight-forward way is to implement EnabledAtRuntime IDL extended attribute for JSC.
Comment 12 Mihai Maerean 2012-11-13 07:51:49 PST
(In reply to comment #6)
> Why not just have Chromium use WebSettings to enable Regions?

chromium/src/content/renderer/render_thread_impl.cc uses WebRuntimeFeatures and RuntimeEnabledFeatures to enable similar features:

void RenderThreadImpl::EnsureWebKitInitialized() {
...
  WebRuntimeFeatures::enableStyleScoped(command_line.HasSwitch(switches::kEnableExperimentalWebKitFeatures));
  WebRuntimeFeatures::enableCSSExclusions(command_line.HasSwitch(switches::kEnableExperimentalWebKitFeatures));

The idea was to use switches::kEnableExperimentalWebKitFeatures in a way that is similar to how it has been used before.

Using WebSettings would be nice because it would have a direct connection with WebCore::Settings.

Unfortunately, there is no mention of WebSettings inside chromium/src/content/renderer/render_thread_impl.cc .

How should I get access to WebSettings from inside render_thread_impl.cc?
Comment 13 Adam Barth 2012-11-13 08:21:53 PST
> The idea was to use switches::kEnableExperimentalWebKitFeatures in a way that is similar to how it has been used before.

That's fine, but then you shouldn't use WebCore::Settings.  The problem is that you're trying to use two different mechanisms to control whether the feature is enabled.  You need to pick one and use that.

> How should I get access to WebSettings from inside render_thread_impl.cc?

You can't.  WebSettings is a per WebView concept.  render_thread_impl is shared across all the WebViews in a given process.  Try grepping around for uses of WebSettings in Chromium and you'll see how to do it.
Comment 14 Mihai Maerean 2012-11-13 09:57:44 PST
(In reply to comment #13)
> > The idea was to use switches::kEnableExperimentalWebKitFeatures in a way that is similar to how it has been used before.
> 
> That's fine, but then you shouldn't use WebCore::Settings.  The problem is that you're trying to use two different mechanisms to control whether the feature is enabled.  You need to pick one and use that.

We are forced to use WebRuntimeFeatures and RuntimeEnabledFeatures in order to stop the feature from leaking information at run-time by adding V8EnabledAtRuntime=cssRegions in the IDL files.

On the other hand, we are forced to use the bit in Settings so that the JSC platforms build the code (so people don't accidentally break it) and also keep the feature disabled at run-time.

So we can't remove any on them.

The solution that I see is to have them both and hopefully the 2 variables won't get out of sync as the webkit/chromium code evolves over time.

Is this acceptable?
Comment 15 James Robinson 2012-11-13 10:03:42 PST
(In reply to comment #10)
> The JSC platforms must have a way to disable the CSS Regions code at runtime.

Are you sure about this?
Comment 16 Adam Barth 2012-11-13 10:05:30 PST
> Is this acceptable?

It sounds like the Settings approach doesn't really work for JSC either.  If it worked for JSC, then you could use it for V8 as well.  The ability to turn this feature off at runtime for JSC is half-baked.  IMHO, you should either fully bake it (likely by implementing EnabledAtRuntime for JSC) or you should remove it (e.g., and use ENABLE macros to enable/disable the feature for JSC).
Comment 17 Mihai Maerean 2012-11-28 04:35:26 PST
Created attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

Work in Progress.
Comment 18 WebKit Review Bot 2012-11-28 04:54:35 PST
Comment on attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

Attachment 176456 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15018420
Comment 19 Mihai Maerean 2012-11-28 05:56:59 PST
(In reply to comment #18)
> (From update of attachment 176456 [details])
> Attachment 176456 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15018420

My changes did include the fix for Chromium, but the changes are located in Source/WebKit/chromium/webkit/glue/ (brought by update-webkit --chromium) and aren't picked up by webkit-patch or git diff.

I have googled for "how to submit patch affects both webkit chromium glue code", but I didn't find the answer.

So how do I add the changes found in Source/WebKit/chromium/webkit/glue/ to the patch so the chromium-ews can successfully build the sources?
Comment 20 Adam Barth 2012-11-28 06:00:43 PST
> So how do I add the changes found in Source/WebKit/chromium/webkit/glue/ to the patch so the chromium-ews can successfully build the sources?

webkit/glue is located in svn.chromium.org:

http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/

You would first need to land the changes in svn.chromium.org and then increase the value of chromium_rev in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS so that WebKit can see your change to Chromium.
Comment 21 Adam Barth 2012-11-28 06:03:27 PST
Comment on attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

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

> Source/WebKit/chromium/public/WebSettings.h:-104
> -    virtual void setExperimentalCSSRegionsEnabled(bool) = 0;

I suspect the problem is that you're trying to change this public API.  What you should do instead is move this function to the bottom of the interface and make it a no-op.  After this patch lands and Chromium's webkit_rev is increased, you can remove any callers in Chromium.  Once the Chromium callers are gone and WebKit's chromium_rev is increased, you can remove the deprecated API.
Comment 22 Peter Beverloo (cr-android ews) 2012-11-28 06:55:36 PST
Comment on attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

Attachment 176456 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15016417
Comment 23 Mihai Maerean 2012-11-28 07:44:46 PST
Created attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.
Comment 24 Hajime Morrita 2012-11-29 22:55:39 PST
Comment on attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.

rs=me, all green, adam's comments being addressed.
Comment 25 WebKit Review Bot 2012-11-30 00:04:35 PST
Comment on attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.

Rejecting attachment 176486 [details] from commit-queue.

mmaerean@adobe.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 26 WebKit Review Bot 2012-11-30 00:49:20 PST
Comment on attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.

Clearing flags on attachment: 176486

Committed r136210: <http://trac.webkit.org/changeset/136210>
Comment 27 WebKit Review Bot 2012-11-30 00:49:26 PST
All reviewed patches have been landed.  Closing bug.