[webkit-reviews] review denied: [Bug 90532] Shadow DOM for <img> : [Attachment 151170] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 90532: Shadow DOM for <img>
https://bugs.webkit.org/show_bug.cgi?id=90532

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

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=151170&action=review


Looks a great start! Let's have a few more iteration.

> Source/WebCore/dom/ElementShadow.cpp:77
> +	   shadowHost->willAddAuthorShadowRoot();

I think it's OK to always. invoke this.
Also, this can return false and get validateShadowRoot() merged.

> Source/WebCore/html/HTMLImageElement.cpp:80
> +    RefPtr<ImageInnerElement> innerElement =
ImageInnerElement::create(document());

Following logic is typically called and defined createShadowSubtree().
Such a naming convention is also applicable here.

> Source/WebCore/html/HTMLImageElement.cpp:134
> +	   if (shadow())

shadow() is slow. Please don't call it twice here.

> Source/WebCore/html/HTMLImageElement.cpp:164
> +	   return new (arena) RenderBlock(this);

I think this should be able to RenderInline.
Why not share the following hasContent() path?

> Source/WebCore/html/HTMLImageElement.cpp:308
> +	   innerElement()->setAttribute(heightAttr, String::number(value));

Is it OK to ignore setWidth()? Why is this needed after all?

> Source/WebCore/html/shadow/ImageInnerElement.cpp:49
> +    ASSERT(shadowAncestorNode()->hasTagName(HTMLNames::imgTag));

Let's define toHTMLImageElement() in HTMLImageELement.h.
That is a pattern for this.

> Source/WebCore/html/shadow/ImageInnerElement.cpp:62
> +    if (renderer() && renderer()->isImage() &&
!imageLoader()->hasPendingBeforeLoadEvent()) {

Please don't make such a copy-n-taste. Let's share the code somehow.
dom/StyleElement.h or html/LabelableElement might be a good reference.

> Source/WebCore/html/shadow/ImageInnerElement.cpp:76
> +RenderObject* ImageInnerElement::createRenderer(RenderArena* arena,
RenderStyle* style)

Ditto. Probably you can just invoke host's createRenderer().

> Source/WebCore/loader/ImageLoader.cpp:94
> +    , m_usesInnerElement(false)

This approach looks like an abstraction leakage.
The loader shouldn't know about shadow tree and something like that.

One possible idea is to define a "client" interface to the ImageLoader and 
implement it in HTMLImageElement to hide these detaiis.

> LayoutTests/ChangeLog:25
> +

Let's add tests for 
- events
- non-default display styles like inline, none, etc.
- light children and <content>