[webkit-reviews] review granted: [Bug 85914] Add stylesheet inheritance support to IFRAME_SEAMLESS : [Attachment 140782] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 85914: Add stylesheet inheritance support to IFRAME_SEAMLESS
https://bugs.webkit.org/show_bug.cgi?id=85914

Attachment 140782: Patch
https://bugs.webkit.org/attachment.cgi?id=140782&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140782&action=review


This seems to be missing a big aspect of the spec. I looked on the github
branch and didn't see anything addressing this either.

1. You only need to add style rules / stylesheets that apply to the iframe
element. This could have big performance implications. But, it's not a
correctness issue, so a FIXME would be fine.
2. The root element of the iframe needs to inherit from the iframe. This is a
correctness issue. Would be good to add testcases for this.

"In a CSS-supporting user agent: the user agent must add all the style sheets
that apply to the iframe element to the cascade of the active document of the
iframe element's nested browsing context, at the appropriate cascade levels,
before any style sheets specified by the document itself.

In a CSS-supporting user agent: the user agent must, for the purpose of CSS
property inheritance only, treat the root element of the active document of the
iframe element's nested browsing context as being a child of the iframe
element. (Thus inherited properties on the root element of the document in the
iframe will inherit the computed values of those properties on the iframe
element instead of taking their initial values.)"

Other than that, the patch looks good to me.

> Source/WebCore/css/StyleResolver.cpp:433
> +    HTMLFrameOwnerElement* ownerElement = document()->ownerElement();
> +    Vector<StyleSheetList*> ancestorSheets;
> +    while (ownerElement && ownerElement->hasTagName(iframeTag) &&
static_cast<HTMLIFrameElement*>(ownerElement)->shouldDisplaySeamlessly()) {
> +	   ancestorSheets.append(ownerElement->document()->styleSheets());
> +	   ownerElement = ownerElement->document()->ownerElement();

I'd rather this code use seamlessParentIFrame to get the ownerElement. Seems
like it would at least simplify the while clause.