Clone template contents in importNode

Clone template contents in importNode

The template element has no children but contains content, the old importNode behavior only does classical child traversal so special case template elements to import its contents in addition to the normal traversal, like HTMLTemplateElement cloneNode implementation does. Refactor the children traversal importNode into a helper method for better readability and reuse.

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

[rwlbuis, 2014-02-21 20:11:50]

PTAL

[dominicc (has gone to gerrit), 2014-02-24 05:13:02]

LGTM

Why do we have separate code paths for importing and cloning nodes if the DOM
spec uses mostly the same steps for those?

[rwlbuis, 2014-02-24 12:34:32]

On 2014/02/24 05:13:02, dominicc wrote:
> LGTM
> 
> Why do we have separate code paths for importing and cloning nodes if the DOM
> spec uses mostly the same steps for those?

Unfortunately cloneNode keeps the original owner document, whereas importing
changes the owner document, so cloneNode can't be used in importNode. I agree it
does not seem efficient :(
Thanks for the okay :)

[rwlbuis, 2014-02-24 12:34:43]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-02-24 12:34:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/173303004/50001

[commit-bot: I haz the power, 2014-02-24 13:33:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/173303004/50001

[commit-bot: I haz the power, 2014-02-24 18:46:22]

Change committed as 167698

[adamk, 2014-02-24 18:55:59]

Thanks for the fix, but please add a test for this change (and ideally push the
test upstream to github.com/w3c/web-platform-tests)

[rafaelw, 2014-05-02 22:04:14]

Why wasn't a layout test included with this patch?

[dominicc (has gone to gerrit), 2014-05-03 00:11:24]

Yes, it should have a layout test. I suppose I mentally bucketed this as
"refactoring" but it is not. Rob, did you subsequently write a test for
this?
On May 3, 2014 7:04 AM, <rafaelw@chromium.org> wrote:

> Why wasn't a layout test included with this patch?
>
> https://codereview.chromium.org/173303004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[rwlbuis, 2014-05-03 02:43:11]

On 2014/05/03 00:11:24, dominicc wrote:
> Yes, it should have a layout test. I suppose I mentally bucketed this as
> "refactoring" but it is not. Rob, did you subsequently write a test for
> this?
> On May 3, 2014 7:04 AM, <mailto:rafaelw@chromium.org> wrote:
> 
> > Why wasn't a layout test included with this patch?
> >
> > https://codereview.chromium.org/173303004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Sure I'll try to come up with a test.

[rwlbuis, 2014-05-04 15:28:07]

On 2014/05/03 02:43:11, rwlbuis wrote:
> On 2014/05/03 00:11:24, dominicc wrote:
> > Yes, it should have a layout test. I suppose I mentally bucketed this as
> > "refactoring" but it is not. Rob, did you subsequently write a test for
> > this?
> > On May 3, 2014 7:04 AM, <mailto:rafaelw@chromium.org> wrote:
> > 
> > > Why wasn't a layout test included with this patch?
> > >
> > > https://codereview.chromium.org/173303004/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> Sure I'll try to come up with a test.

Wait! It seems a test was added:

https://codereview.chromium.org/180293002

Let me know if that is enough.