Remove obsolete comment in ShadowRoot.h

Remove obsolete comment in ShadowRoot.h

The comment links to
https://bugs.webkit.org/show_bug.cgi?id=77503
which is already resolved.

Multiple shadow roots still remain for a while and
the comment is valid about that some elements doesn't
allow shadow roots on them, which are tracked at
https://github.com/w3c/webcomponents/issues/102
https://code.google.com/p/chromium/issues/detail?id=234020

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196554

[kochi, 2015-06-04 06:26:24]

kochi@chromium.org changed reviewers:
+ hayato@chromium.org

[kochi, 2015-06-04 06:26:37]

Hayato, do you agree this comment is obsolete?

[hayato, 2015-06-04 07:25:07]

https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
File Source/core/dom/shadow/ShadowRoot.h (left):

https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
Source/core/dom/shadow/ShadowRoot.h:52: // FIXME: We will support multiple
shadow subtrees, however current implementation does not work well
The spec dropped multiple shadow roots, however, Blink is still supporting
multiple shadow roots. Blink just deprecated it.
Instead of removing the comment, it looks it might be better to update the
comment.

[kochi, 2015-06-04 07:47:38]

https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
File Source/core/dom/shadow/ShadowRoot.h (left):

https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
Source/core/dom/shadow/ShadowRoot.h:52: // FIXME: We will support multiple
shadow subtrees, however current implementation does not work well
On 2015/06/04 07:25:07, hayato wrote:
> The spec dropped multiple shadow roots, however, Blink is still supporting
> multiple shadow roots. Blink just deprecated it.
> Instead of removing the comment, it looks it might be better to update the
> comment.

What comment do you think is useful?

I think the problem this comment is about is what the GitHub issue#102
https://github.com/w3c/webcomponents/issues/102
covers, and the resolution is to throw exception where
createShadowRoot() cannot be done.

So there is something to do in implementation, but is it worth
referring to the GitHub issue here?

// TODO(hayato): throw exception on some elements which createShadowRoot()
// cannot work (e.g. <canvas>).
// See https://github.com/w3c/webcomponents/issues/102

or
// TODO(hayato): http://crbug.com/234020 make createShadowRoot() always work
// on all elements.

for longer-term solution (?).

WDYT?

[hayato, 2015-06-04 08:20:14]

https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
File Source/core/dom/shadow/ShadowRoot.h (left):

https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
Source/core/dom/shadow/ShadowRoot.h:52: // FIXME: We will support multiple
shadow subtrees, however current implementation does not work well
On 2015/06/04 07:47:38, Takayoshi Kochi wrote:
> On 2015/06/04 07:25:07, hayato wrote:
> > The spec dropped multiple shadow roots, however, Blink is still supporting
> > multiple shadow roots. Blink just deprecated it.
> > Instead of removing the comment, it looks it might be better to update the
> > comment.
> 
> What comment do you think is useful?
> 
> I think the problem this comment is about is what the GitHub issue#102
> https://github.com/w3c/webcomponents/issues/102
> covers, and the resolution is to throw exception where
> createShadowRoot() cannot be done.
> 
> So there is something to do in implementation, but is it worth
> referring to the GitHub issue here?
> 
> // TODO(hayato): throw exception on some elements which createShadowRoot()
> // cannot work (e.g. <canvas>).
> // See https://github.com/w3c/webcomponents/issues/102
> 
> or
> // TODO(hayato): http://crbug.com/234020 make createShadowRoot() always work
> // on all elements.
> 
> for longer-term solution (?).
> 
> WDYT?

Only part we have to remove is "We'll support multiple shadow subtrees", I
think. That's obsolete.

Other parts still have the status of the current implementation of multiple
shadow roots. We can't remove other comment for the reason which the description
of the CL says. We have to continue the support of multiple shadow roots for a
while.

[kochi, 2015-06-05 02:38:33]

On 2015/06/04 08:20:14, hayato wrote:
>
https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
> File Source/core/dom/shadow/ShadowRoot.h (left):
> 
>
https://codereview.chromium.org/1156133008/diff/1/Source/core/dom/shadow/Shad...
> Source/core/dom/shadow/ShadowRoot.h:52: // FIXME: We will support multiple
> shadow subtrees, however current implementation does not work well
> On 2015/06/04 07:47:38, Takayoshi Kochi wrote:
> > On 2015/06/04 07:25:07, hayato wrote:
> > > The spec dropped multiple shadow roots, however, Blink is still supporting
> > > multiple shadow roots. Blink just deprecated it.
> > > Instead of removing the comment, it looks it might be better to update the
> > > comment.
> > 
> > What comment do you think is useful?
> > 
> > I think the problem this comment is about is what the GitHub issue#102
> > https://github.com/w3c/webcomponents/issues/102
> > covers, and the resolution is to throw exception where
> > createShadowRoot() cannot be done.
> > 
> > So there is something to do in implementation, but is it worth
> > referring to the GitHub issue here?
> > 
> > // TODO(hayato): throw exception on some elements which createShadowRoot()
> > // cannot work (e.g. <canvas>).
> > // See https://github.com/w3c/webcomponents/issues/102
> > 
> > or
> > // TODO(hayato): http://crbug.com/234020 make createShadowRoot() always work
> > // on all elements.
> > 
> > for longer-term solution (?).
> > 
> > WDYT?
> 
> Only part we have to remove is "We'll support multiple shadow subtrees", I
> think. That's obsolete.

What about the link to webkit bug?  The link says it's "RESOLVED FIXED".
So I think the link should point to proper ones.

> Other parts still have the status of the current implementation of multiple
> shadow roots. We can't remove other comment for the reason which the
description
> of the CL says. We have to continue the support of multiple shadow roots for a
> while.

Okay, updated the description and wording of the original comment.

[hayato, 2015-06-05 04:34:52]

Thanks for updating.
LGTM

[kochi, 2015-06-05 04:36:52]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2015-06-05 04:37:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156133008/20001

[commit-bot: I haz the power, 2015-06-05 05:38:51]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196554