Bug 91076 - [CSS Regions] Add the Region interface and the getRegions() API call
Summary: [CSS Regions] Add the Region interface and the getRegions() API call
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 87640 57132
  Show dependency treegraph
 
Reported: 2012-07-12 05:23 PDT by Andrei Bucur
Modified: 2017-07-18 08:27 PDT (History)
17 users (show)

See Also:


Attachments
Patch (53.63 KB, patch)
2012-07-18 05:14 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (53.84 KB, patch)
2012-07-18 07:27 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (87.71 KB, patch)
2012-07-23 09:59 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (92.73 KB, patch)
2012-07-24 06:53 PDT, Andrei Bucur
abarth: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (315.44 KB, application/zip)
2012-07-24 10:12 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-06 (315.56 KB, application/zip)
2012-07-24 11:07 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Bucur 2012-07-12 05:23:43 PDT
The http://www.w3.org/TR/2012/WD-css3-regions-20120503/ spec detaches the region concept from the Element and introduces the Region interface. The regions are now accessible only through the NamedFlow interface using either getRegions or getRegionsByContent.
Comment 1 Andrei Bucur 2012-07-18 05:14:59 PDT
Created attachment 152991 [details]
Patch
Comment 2 Andreas Kling 2012-07-18 06:12:11 PDT
Comment on attachment 152991 [details]
Patch

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

Looks good functionality-wise, but there are a number of overly generic variable and function names that need fixing.

> Source/WebCore/css/CSSRegion.cpp:2
> + * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved.

2012!

> Source/WebCore/css/CSSRegion.cpp:46
> +String CSSRegion::flowFrom(ExceptionCode& e) const

Style nit: We typically use "ec" for the ExceptionCode variables.

> Source/WebCore/css/CSSRegion.cpp:59
> +const AtomicString& CSSRegion::regionOverset(ExceptionCode& e) const

Style nit: We typically use "ec" for the ExceptionCode variables.

> Source/WebCore/css/CSSRegion.cpp:87
> +    default:
> +        break;
> +    }
> +
> +    return undefinedState;

We could replace the "default:" with "case RenderRegion::RegionUndefined:" here, that way the compiler will give us a warning if more values are ever added to RegionState without also adding code here.

> Source/WebCore/css/CSSRegion.h:2
> + * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved.

2012!

> Source/WebCore/css/CSSRegion.h:62
> +    RenderRegion* m_region;

We need a better name for this variable. m_renderer perhaps?

> Source/WebCore/rendering/RenderRegion.h:59
> +    CSSRegion* ensureRegionObject();

This function needs a better name. ensureRegionCSSOMWrapper()?

> Source/WebCore/rendering/RenderRegion.h:148
> +    RefPtr<CSSRegion> m_region;

This variable needs a better name. m_regionCSSOMWrapper?
Comment 3 Andrei Bucur 2012-07-18 07:27:29 PDT
Created attachment 153009 [details]
Patch
Comment 4 Andrei Bucur 2012-07-23 09:59:36 PDT
Created attachment 153813 [details]
Patch
Comment 5 WebKit Review Bot 2012-07-23 10:23:24 PDT
Comment on attachment 153813 [details]
Patch

Attachment 153813 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13317000
Comment 6 Andrei Bucur 2012-07-24 06:53:14 PDT
Created attachment 154048 [details]
Patch
Comment 7 Andrei Bucur 2012-07-24 08:13:32 PDT
Comment on attachment 154048 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:503
> +    if (($interfaceName eq 'Element' and !($dataNode->extendedAttributes->{"CustomToJSObject"} or $dataNode->extendedAttributes->{"V8CustomToJSObject"})) or $dataNode->extendedAttributes->{"SuppressToJSObject"}) {

Is this bad to be here? Because Element is now both CSSRegion and Node, the compiler didn't know which one to pick. With this new condition and a JSElementCustom.cpp file I'm solving the ambiguity by manually forcing toV8(Element*) to call toV8(Node*).
Comment 8 Andrei Bucur 2012-07-24 08:15:00 PDT
(In reply to comment #7)
> (From update of attachment 154048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154048&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:503
> > +    if (($interfaceName eq 'Element' and !($dataNode->extendedAttributes->{"CustomToJSObject"} or $dataNode->extendedAttributes->{"V8CustomToJSObject"})) or $dataNode->extendedAttributes->{"SuppressToJSObject"}) {
> 
> Is this bad to be here? Because Element is now both CSSRegion and Node, the compiler didn't know which one to pick. With this new condition and a JSElementCustom.cpp file I'm solving the ambiguity by manually forcing toV8(Element*) to call toV8(Node*).

That is V8ElementCustom.cpp.
Comment 9 WebKit Review Bot 2012-07-24 10:12:50 PDT
Comment on attachment 154048 [details]
Patch

Attachment 154048 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13338298

New failing tests:
svg/custom/animate-reference-crash.html
svg/custom/detached-outermost-svg-crash.html
fast/dom/domListEnumeration.html
fast/dom/plugin-attributes-enumeration.html
Comment 10 WebKit Review Bot 2012-07-24 10:12:57 PDT
Created attachment 154095 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 11 WebKit Review Bot 2012-07-24 11:07:11 PDT
Comment on attachment 154048 [details]
Patch

Attachment 154048 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13338325

New failing tests:
svg/custom/animate-reference-crash.html
svg/custom/detached-outermost-svg-crash.html
fast/dom/domListEnumeration.html
fast/dom/plugin-attributes-enumeration.html
Comment 12 WebKit Review Bot 2012-07-24 11:07:19 PDT
Created attachment 154110 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 13 Adam Barth 2012-07-24 14:50:26 PDT
Comment on attachment 154048 [details]
Patch

I'm pretty sure this isn't what you want to do here.  It looks like you're trying to simulate multiple inheritance, but that doesn't work in the DOM.
Comment 14 Adam Barth 2012-07-24 14:50:58 PDT
Let's discuss in #webkit what you're trying to accomplish.  That's likely to be more interactive and lead to the solution faster.
Comment 15 Adam Barth 2012-07-24 16:30:24 PDT
I seem to have missed you on #webkit.
Comment 16 Adam Barth 2012-07-24 16:33:34 PDT
I took a look at <http://www.w3.org/TR/css3-regions/#dom-named-flow>.  I don't think that the spec works in the sense that it's trying to do things with WebIDL that aren't possible.

We'll need to revise the API to work in the DOM.  In particular, we'll need to remove the multiple inheritance.
Comment 17 Adam Barth 2012-07-24 18:12:02 PDT
We discussed this a bunch in #webkit.  I'm not convinced that the Regions spec is doing something allowed by the WebIDL spec.  Our implementation of WebIDL isn't complete in this regard.  Specifically, in our bindings generation, we flatten the supplemental structure too early, which is fine for creating the prototype objects, but doesn't have enough information to let us convert C++ objects to the appropriate JS wrapper.
Comment 18 Adam Barth 2012-07-24 18:19:19 PDT
Comment on attachment 154048 [details]
Patch

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

> Source/WebCore/bindings/js/JSCSSRegionCustom.cpp:47
> +JSValue toJS(ExecState* exec, JSDOMGlobalObject* globalObject, CSSRegion* target)
> +{
> +    if (!target)
> +        return jsNull();
> +
> +    // FIXME: Add support for pseudo-elements as well
> +    Element* elementRegion = static_cast<Element*>(target);
> +    return toJS(exec, globalObject, static_cast<Node*>(elementRegion));
> +}

The main thing we need to do is teach the code generate how to generate this sort of toJS function.  The approach used in this implementation doesn't scale to the case where we actually have more than one type of object implementing Region.  Here you're using static_cast<Element*>, which works because there isn't any other possibility.  If there were two possibilities, we'd need to use something like dynamic_cast, which we have disabled.

I'd really prefer if the Regions spec didn't use this form of fake multiple inheritance.  It seems like a large amount of complexity to incur just to use the type Region instead of Element.  However, I'm willing to believe there are considerations that I'm not aware of.  If we need to build a dynamic_cast-like mechanism for JS wrappers, we should also consider the other cases in which we've hand-built dynamic_cast-like machanisms in toJS functions.  Those include Nodes, Exceptions, Events, and EventTargets.
Comment 19 Alan Stearns 2012-07-25 13:24:38 PDT
The purpose behind "Element implements Region" is to allow future additions to what can become a CSS Region. In particular, we'd like CSS pseudo-elements (when they get a CSSOM representation) to be CSS Regions. So at some point in the near future I hope there will be a "CSSPseudoElement implements Region" line in a spec somewhere.

After more discussion on #webkit, we decided to propose a design for how to implement this to webkit-dev.
Comment 20 Andrei Bucur 2012-08-02 06:30:49 PDT
For now, the Region interface implementation is postponed until a new type of container is available.
Comment 21 Michelangelo De Simone 2013-06-13 20:29:43 PDT
This bug seems stale, closing for now.
Comment 22 Mihai Balan 2013-06-14 02:10:37 PDT
We still want to have this open as we'll eventually get to properly implementing it.
Re-opening for now.