Bug 90532

Summary: Shadow DOM for img element
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cmarcelo, dglazkov, dominicc, eric.carlson, eric, feature-media-reviews, gustavo, gyuyoung.kim, hayato, japhet, jochen, macpherson, menard, mifenton, morrita, philn, rakuco, rniwa, tasak, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 91167, 97867    
Bug Blocks: 82313    
Attachments:
Description Flags
W.I.P.
none
Archive of layout-test-results from gce-cr-linux-07
none
Archive of layout-test-results from gce-cr-linux-04
none
WIP
none
Archive of layout-test-results from gce-cr-linux-03
none
Archive of layout-test-results from gce-cr-linux-04
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch (Changed ChangeLog)
none
Patch for landing
none
Patch for landing none

Description Shinya Kawanaka 2012-07-04 02:20:34 PDT
The spec says <img> should behave like having a ShadowRoot.
Currently when we add a Shadow DOM to <img>, it does not have almost no effect.
Comment 1 Shinya Kawanaka 2012-07-04 02:22:40 PDT
Created attachment 150741 [details]
W.I.P.
Comment 2 WebKit Review Bot 2012-07-04 02:37:56 PDT
Attachment 150741 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/html/shadow/ImageInnerElement.h:31:  #ifndef header guard has wrong style, please use: ImageInnerElement_h  [build/header_guard] [5]
Source/WebCore/html/shadow/ImageInnerElement.h:46:  The parameter name "document" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/shadow/ImageInnerElement.h:54:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/html/shadow/ImageInnerElement.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/html/shadow/ImageInnerElement.cpp:33:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gyuyoung Kim 2012-07-04 02:55:28 PDT
Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13125767
Comment 4 Early Warning System Bot 2012-07-04 02:55:51 PDT
Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13130768
Comment 5 Build Bot 2012-07-04 03:02:10 PDT
Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13133697
Comment 6 Early Warning System Bot 2012-07-04 03:02:14 PDT
Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13139457
Comment 7 WebKit Review Bot 2012-07-04 03:42:22 PDT
Comment on attachment 150741 [details]
W.I.P.

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

New failing tests:
fast/dom/shadow/shadowdom-for-image.html
Comment 8 WebKit Review Bot 2012-07-04 03:42:25 PDT
Created attachment 150761 [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 9 Build Bot 2012-07-04 03:59:40 PDT
Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13139474
Comment 10 Build Bot 2012-07-04 04:02:08 PDT
Comment on attachment 150741 [details]
W.I.P.

Attachment 150741 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13130778
Comment 11 WebKit Review Bot 2012-07-04 04:36:56 PDT
Comment on attachment 150741 [details]
W.I.P.

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

New failing tests:
fast/dom/shadow/shadowdom-for-image.html
Comment 12 WebKit Review Bot 2012-07-04 04:37:00 PDT
Created attachment 150770 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 13 Alexey Proskuryakov 2012-07-04 12:45:29 PDT
> The spec says <img> should behave like having a ShadowRoot.

Why does the spec say so? Given that current state of shadow DOM, matching a spec doesn't sounds like a valid reason to change implementation.
Comment 14 Hayato Ito 2012-07-04 16:08:19 PDT
(In reply to comment #13)
> > The spec says <img> should behave like having a ShadowRoot.
> 
> Why does the spec say so? Given that current state of shadow DOM, matching a spec doesn't sounds like a valid reason to change implementation.

That's important point. We should not impact an img element as long as users do not add a shadow root.

It seems shinya tries to preserve the current img elemnt's structure unless users add a shadowRoot explictly.
Comment 15 Shinya Kawanaka 2012-07-04 17:24:02 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > > The spec says <img> should behave like having a ShadowRoot.
> > 
> > Why does the spec say so? Given that current state of shadow DOM, matching a spec doesn't sounds like a valid reason to change implementation.
> 
> That's important point. We should not impact an img element as long as users do not add a shadow root.
> 
> It seems shinya tries to preserve the current img elemnt's structure unless users add a shadowRoot explictly.

Yes. I'm trying to keep <img> as is if a Shadow DOM is not attached.

My implementation idea is:
  (1) <img> behavior does not change if Shadow DOM is not attached.
  (2) When attaching Shadow DOM to <img>, we add our Shadow DOM before attaching Shadow DOM created by a user.
Comment 16 Shinya Kawanaka 2012-07-04 18:27:15 PDT
Created attachment 150855 [details]
WIP
Comment 17 Shinya Kawanaka 2012-07-04 18:45:34 PDT
I will add more tests if this patch compiles.
Comment 18 Build Bot 2012-07-04 18:56:01 PDT
Comment on attachment 150855 [details]
WIP

Attachment 150855 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13136879
Comment 19 WebKit Review Bot 2012-07-04 19:30:49 PDT
Comment on attachment 150855 [details]
WIP

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

New failing tests:
fast/dom/shadow/shadowdom-for-image.html
Comment 20 WebKit Review Bot 2012-07-04 19:30:53 PDT
Created attachment 150860 [details]
Archive of layout-test-results from gce-cr-linux-03

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

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

New failing tests:
fast/dom/shadow/shadowdom-for-image.html
Comment 22 WebKit Review Bot 2012-07-04 20:30:37 PDT
Created attachment 150867 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 23 Shinya Kawanaka 2012-07-08 19:42:51 PDT
Created attachment 151170 [details]
Patch
Comment 24 Hajime Morrita 2012-07-08 23:26:42 PDT
Comment on attachment 151170 [details]
Patch

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>
Comment 25 Shinya Kawanaka 2012-07-09 18:50:30 PDT
Created attachment 151383 [details]
Patch
Comment 26 Shinya Kawanaka 2012-07-09 18:55:21 PDT
(In reply to comment #24)
> (From update of attachment 151170 [details])
> 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.

If we always call this, infinite loop will happen because we add a ShadowRoot before a user
adds a ShadowRoot...

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

Done.

> 
> > Source/WebCore/html/HTMLImageElement.cpp:134
> > +        if (shadow())
> 
> shadow() is slow. Please don't call it twice here.

Done.

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

Done.

> 
> > Source/WebCore/html/HTMLImageElement.cpp:308
> > +        innerElement()->setAttribute(heightAttr, String::number(value));
> 
> Is it OK to ignore setWidth()? Why is this needed after all?

Removed. It's not necessary.

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

Done.

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

We add ImageElement class to share it among HTMLImageElement and ImageInnerElement.
It's something like a mix-in pattern.

> 
> > Source/WebCore/html/shadow/ImageInnerElement.cpp:76
> > +RenderObject* ImageInnerElement::createRenderer(RenderArena* arena, RenderStyle* style)
> 
> Ditto. Probably you can just invoke host's createRenderer().

We don't want to call shadow() in ImageInnerElement actually.
We shared the rest of code in ImageElement.

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

Yeah... I made a ImageLoaderClient to conceal it.

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

Done.
Comment 27 Build Bot 2012-07-09 19:22:35 PDT
Comment on attachment 151383 [details]
Patch

Attachment 151383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13166435
Comment 28 Hajime Morrita 2012-07-09 19:44:25 PDT
Comment on attachment 151383 [details]
Patch

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

> Source/WebCore/dom/ElementShadow.cpp:59
> +ElementShadow::ElementShadow()

Why did you move these?

> Source/WebCore/html/HTMLImageElement.cpp:47
> +class ShadowAwareImageLoaderClient : public ImageLoaderClient {

I think HTMLImageElement or ImageElement can implement ImageLoaderClient 
instead of introducing a new class.

> Source/WebCore/html/HTMLImageElement.h:39
> +    virtual RenderObject* renderer() const = 0;

Having virtual renderer() can hide non-virtual Node::renderer(), which is confusing.
I'd prefer to have different name.

> Source/WebCore/loader/ImageLoader.h:43
> +class ImageLoaderClient {

Typical client is a pure virtual class rather than partially implemented one.

> Source/WebCore/loader/ImageLoader.h:53
> +    virtual Element* element() const { return m_element; }

Exposing the element doesn't help encapsulation. It just complicates things.
Comment 29 Shinya Kawanaka 2012-07-09 23:41:40 PDT
Created attachment 151404 [details]
Patch
Comment 30 Shinya Kawanaka 2012-07-09 23:45:22 PDT
(In reply to comment #28)
> (From update of attachment 151383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151383&action=review
> 
> > Source/WebCore/dom/ElementShadow.cpp:59
> > +ElementShadow::ElementShadow()
> 
> Why did you move these?

It's just a mistake... Sorry.

> 
> > Source/WebCore/html/HTMLImageElement.cpp:47
> > +class ShadowAwareImageLoaderClient : public ImageLoaderClient {
> 
> I think HTMLImageElement or ImageElement can implement ImageLoaderClient 
> instead of introducing a new class.

I would like to manage the lifecycle of ImageLoaderClient in ImageLoader...
So I think it would be better have a new class.

> 
> > Source/WebCore/html/HTMLImageElement.h:39
> > +    virtual RenderObject* renderer() const = 0;
> 
> Having virtual renderer() can hide non-virtual Node::renderer(), which is confusing.
> I'd prefer to have different name.

Done.

> 
> > Source/WebCore/loader/ImageLoader.h:43
> > +class ImageLoaderClient {
> 
> Typical client is a pure virtual class rather than partially implemented one.

Done.

> 
> > Source/WebCore/loader/ImageLoader.h:53
> > +    virtual Element* element() const { return m_element; }
> 
> Exposing the element doesn't help encapsulation. It just complicates things.

I think hiding all element() is too much. What we would like to do here is to distinguish
the element we get attributes from and the element we render an image to.
So it would be OK to have 2 methods to return such elements.
In most cases, they are the same for now.
Comment 31 Shinya Kawanaka 2012-07-10 00:52:38 PDT
Created attachment 151410 [details]
Patch
Comment 32 Hajime Morrita 2012-07-10 03:19:05 PDT
Comment on attachment 151410 [details]
Patch

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

As we discussed offline, we also need to take care of renderer side where
RenderObject::node() is accessed in varioius ways.

> Source/WebCore/html/HTMLImageElement.cpp:65
> +        return toElement(shadow->oldestShadowRoot()->firstChild());

It would be worth ASSERTing the tag name.

> Source/WebCore/html/HTMLImageElement.h:40
> +protected:

Let's consider to kill these virtual method to eliminate the vtable.
It looks both are used only in setImageIfNecessary(). So we can pass it as arguments instead of defining virtual method.

> Source/WebCore/html/HTMLImageElement.h:134
> +    ImageLoaderClient* m_imageLoaderClient;

It looks we can eliminate this to save object size.

> Source/WebCore/html/HTMLImageLoader.cpp:42
> +public:

Nit: Should be non-copyable and fast-allocated.
Also, client implementation should be on the client side, not loader side.

> Source/WebCore/html/HTMLImageLoader.cpp:57
> +HTMLImageLoader::HTMLImageLoader(Element* element)

So we should no longer need this constructor.
Comment 33 Shinya Kawanaka 2012-07-10 19:10:13 PDT
Created attachment 151579 [details]
Patch
Comment 34 Shinya Kawanaka 2012-07-10 19:15:57 PDT
(In reply to comment #32)
> (From update of attachment 151410 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151410&action=review
> 
> As we discussed offline, we also need to take care of renderer side where
> RenderObject::node() is accessed in varioius ways.

I added hostImageElement() in RenderImage. It will return host <img> element if node() is an inner image element. 
And I replaced node() where its tag name is checked as <img>.

> 
> > Source/WebCore/html/HTMLImageElement.cpp:65
> > +        return toElement(shadow->oldestShadowRoot()->firstChild());
> 
> It would be worth ASSERTing the tag name.

Done.

> 
> > Source/WebCore/html/HTMLImageElement.h:40
> > +protected:
> 
> Let's consider to kill these virtual method to eliminate the vtable.
> It looks both are used only in setImageIfNecessary(). So we can pass it as arguments instead of defining virtual method.

It's good idea. Done.

> 
> > Source/WebCore/html/HTMLImageElement.h:134
> > +    ImageLoaderClient* m_imageLoaderClient;
> 
> It looks we can eliminate this to save object size.

Done.

> 
> > Source/WebCore/html/HTMLImageLoader.cpp:42
> > +public:
> 
> Nit: Should be non-copyable and fast-allocated.

Done.

> Also, client implementation should be on the client side, not loader side.

Since most of the HTMLImageLoader clients are the same, I would like to provide a default implementation of HTMLImageLoaderClient... There is no suitable place to put it, I decided to
put it in HTMLImageLoader.h for now.

> 
> > Source/WebCore/html/HTMLImageLoader.cpp:57
> > +HTMLImageLoader::HTMLImageLoader(Element* element)
> 
> So we should no longer need this constructor.

Done. Removed.
Comment 35 Build Bot 2012-07-10 20:20:08 PDT
Comment on attachment 151579 [details]
Patch

Attachment 151579 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13208182
Comment 36 Build Bot 2012-07-10 20:40:00 PDT
Comment on attachment 151579 [details]
Patch

Attachment 151579 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13200162
Comment 37 Shinya Kawanaka 2012-07-10 20:57:33 PDT
Created attachment 151584 [details]
Patch
Comment 38 Shinya Kawanaka 2012-07-10 20:58:01 PDT
(In reply to comment #37)
> Created an attachment (id=151584) [details]
> Patch

Try to fix mac build.
Comment 39 Hajime Morrita 2012-07-10 22:36:30 PDT
Comment on attachment 151584 [details]
Patch

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

It looks this doesn't consider <map> element. But we should.

> Source/WebCore/svg/SVGImageLoader.h:39
> +class DefaultSVGImageLoaderClient : public ImageLoaderClient {

It looks we can hide this to .cpp file and expose only the create function.

> LayoutTests/ChangeLog:17
> +

Let's have a test with @alt text.
Comment 40 Shinya Kawanaka 2012-07-11 02:48:11 PDT
Created attachment 151660 [details]
Patch
Comment 41 Shinya Kawanaka 2012-07-11 02:49:02 PDT
(In reply to comment #39)
> (From update of attachment 151584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151584&action=review
> 
> It looks this doesn't consider <map> element. But we should.

I've added a test.

> 
> > Source/WebCore/svg/SVGImageLoader.h:39
> > +class DefaultSVGImageLoaderClient : public ImageLoaderClient {
> 
> It looks we can hide this to .cpp file and expose only the create function.

Done.

> 
> > LayoutTests/ChangeLog:17
> > +
> 
> Let's have a test with @alt text.

I've added tests including dynamic alt text update.
Comment 42 Shinya Kawanaka 2012-07-11 02:58:23 PDT
Created attachment 151664 [details]
Patch
Comment 43 Shinya Kawanaka 2012-07-12 01:25:02 PDT
Created attachment 151883 [details]
Patch
Comment 44 Shinya Kawanaka 2012-07-12 01:26:44 PDT
(In reply to comment #43)
> Created an attachment (id=151883) [details]
> Patch

As shinyak and morrita discussed this offline, I removed the default implementation of ImageLoaderClient, and used mix-in pattern.
Comment 45 Build Bot 2012-07-12 02:20:56 PDT
Comment on attachment 151883 [details]
Patch

Attachment 151883 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13198702
Comment 46 Shinya Kawanaka 2012-07-12 05:01:58 PDT
Created attachment 151913 [details]
Patch
Comment 47 Shinya Kawanaka 2012-07-12 17:31:53 PDT
Created attachment 152107 [details]
Patch
Comment 48 Ryosuke Niwa 2012-07-13 15:02:13 PDT
<img> is a tag. We're not certainly not adding shadow DOM to a HTML tag.
Comment 49 Ryosuke Niwa 2012-07-13 15:08:47 PDT
Comment on attachment 152107 [details]
Patch

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

This is an interesting approach. Looks very promising.

> Source/WebCore/ChangeLog:8
> +        This patch adds Shadow DOM support for <img> element.

Nit: s/<img>/img/.
Comment 50 Shinya Kawanaka 2012-07-16 18:05:50 PDT
Created attachment 152666 [details]
Patch (Changed ChangeLog)
Comment 51 Hajime Morrita 2012-07-16 19:32:05 PDT
Comment on attachment 152666 [details]
Patch (Changed ChangeLog)

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

Overall looks good. Added some nits.

> Source/WebCore/html/HTMLImageElement.h:142
> +    if (!node)

Nit: this looks strange. You can do this check in toHTMLImageElement()

> Source/WebCore/loader/ImageLoaderClient.h:35
> +#include "Element.h"

Do we need this? A forward declaration looks sufficient.

> Source/WebCore/loader/ImageLoaderClient.h:47
> +    virtual void derefElement() = 0;

Which element is going to be ref/deref-ed? The name could imply that.

> Source/WebCore/rendering/RenderImage.cpp:571
> +    if (node()->hasTagName(imgTag))

Could use isHTMLImageElement()
Comment 52 Shinya Kawanaka 2012-07-16 21:48:06 PDT
Created attachment 152694 [details]
Patch for landing
Comment 53 Build Bot 2012-07-16 22:10:44 PDT
Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13270039
Comment 54 WebKit Review Bot 2012-07-16 22:26:29 PDT
Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13280044
Comment 55 Early Warning System Bot 2012-07-16 22:32:44 PDT
Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13269039
Comment 56 Build Bot 2012-07-16 22:42:59 PDT
Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13282041
Comment 57 Shinya Kawanaka 2012-07-16 22:46:26 PDT
Oops.... I've uploaded a wrong patch...
Comment 58 Gyuyoung Kim 2012-07-16 22:48:00 PDT
Comment on attachment 152694 [details]
Patch for landing

Attachment 152694 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13274051
Comment 59 Shinya Kawanaka 2012-07-16 22:48:59 PDT
Created attachment 152698 [details]
Patch for landing
Comment 60 WebKit Review Bot 2012-07-17 01:27:41 PDT
Comment on attachment 152698 [details]
Patch for landing

Clearing flags on attachment: 152698

Committed r122824: <http://trac.webkit.org/changeset/122824>
Comment 61 WebKit Review Bot 2012-07-17 01:27:51 PDT
All reviewed patches have been landed.  Closing bug.