Bug 91076 – [CSS Regions] Add the Region interface and the getRegions() API call

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Andrei Bucur]

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.

[Andrei Bucur]

Created attachment 152991 [details]
Patch

[Andreas Kling]

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?

[Andrei Bucur]

Created attachment 153009 [details]
Patch

[Andrei Bucur]

Created attachment 153813 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153813 [details]
Patch

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

[Andrei Bucur]

Created attachment 154048 [details]
Patch

[Andrei Bucur]

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*).

[Andrei Bucur]

(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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Adam Barth]

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.

[Adam Barth]

Let's discuss in #webkit what you're trying to accomplish.  That's likely to be more interactive and lead to the solution faster.

[Adam Barth]

I seem to have missed you on #webkit.

[Adam Barth]

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.

[Adam Barth]

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.

[Adam Barth]

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.

[Alan Stearns]

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.

[Andrei Bucur]

For now, the Region interface implementation is postponed until a new type of container is available.

[Michelangelo De Simone]

This bug seems stale, closing for now.

[Mihai Balan]

We still want to have this open as we'll eventually get to properly implementing it.
Re-opening for now.

[Brent Fulgham]

CSS Regions were removed in Bug 174978.