Bug 91076

Created attachment 152991 [details] Patch

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?

Created attachment 153009 [details] Patch

Created attachment 153813 [details] Patch

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

Created attachment 154048 [details] Patch

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

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

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

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

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

I seem to have missed you on #webkit.

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.

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

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.

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

This bug seems stale, closing for now.

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

CSS Regions were removed in Bug 174978.