Make default for deep parameter in importNode false

Make default for deep parameter in importNode false, as specified in:

http://dom.spec.whatwg.org/#document

Update tests to reflect this change. It matches Firefox behavior.

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

[rwlbuis, 2014-04-27 18:12:36]

PTAL.

[Inactive, 2014-04-27 19:33:56]

You are saying that it matches Firefox's behavior, which version is that? My
Firefox 28 still does a deep copy but prints the following in the console:
"The behavior of importNode() with no boolean argument is about to change from
doing a deep clone to doing a shallow clone.  Make sure to pass an explicit
boolean argument to keep your current behavior."

What is IE11's behavior? 

I am not an API OWNER but I personally think we should be conservative and print
a warning for a while before updating our behavior, like Firefox is doing.

https://codereview.chromium.org/252713004/diff/1/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/252713004/diff/1/Source/core/dom/Document.h#n...
Source/core/dom/Document.h:326: PassRefPtr<Node> importNode(Node* importedNode,
ExceptionState& ec) { return importNode(importedNode, false, ec); }
We likely don't need this method overload at all if we use [Default=false] for
the parameter in the IDL.

[rwlbuis, 2014-04-27 20:02:51]

On 2014/04/27 19:33:56, Chris Dumez wrote:
> You are saying that it matches Firefox's behavior, which version is that? My
> Firefox 28 still does a deep copy but prints the following in the console:
> "The behavior of importNode() with no boolean argument is about to change from
> doing a deep clone to doing a shallow clone.  Make sure to pass an explicit
> boolean argument to keep your current behavior."

Sorry, I should have been more clear, ft nightly.

> What is IE11's behavior? 

 Terrible :) it can not handle the default param.

[arv (Not doing code reviews), 2014-04-27 20:17:03]

We changed this back and forth once before. I'm on my phone now so I cannot
look up the cl numbers. We don't want to change this again as far as I
remember.
On Apr 27, 2014 4:02 PM, <rob.buis@samsung.com> wrote:

> On 2014/04/27 19:33:56, Chris Dumez wrote:
>
>> You are saying that it matches Firefox's behavior, which version is that?
>> My
>> Firefox 28 still does a deep copy but prints the following in the console:
>> "The behavior of importNode() with no boolean argument is about to change
>> from
>> doing a deep clone to doing a shallow clone.  Make sure to pass an
>> explicit
>> boolean argument to keep your current behavior."
>>
>
> Sorry, I should have been more clear, ft nightly.
>
>  What is IE11's behavior?
>>
>
>  Terrible :) it can not handle the default param.
>
> https://codereview.chromium.org/252713004/
>

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

[ojan, 2014-04-30 02:07:34]

http://src.chromium.org/viewvc/blink?view=revision&revision=99612

It's a bummer that the spec keeps changing here. I'm not sure what we should do.

[arv (Not doing code reviews), 2014-04-30 14:13:24]

On 2014/04/30 02:07:34, ojan wrote:
> http://src.chromium.org/viewvc/blink?view=revision&revision=99612
> 
> It's a bummer that the spec keeps changing here. I'm not sure what we should
do.

Sorry. I was confusing this with cloneNode which seems to have settled down on
deep = false.

We should make importNode have deep = false too to match cloneNode, the spec and
Firefox.

This seems like a low risk but we should probably print a warning like Firefox
is doing for a few releases.

[rwlbuis, 2014-04-30 14:56:19]

On 2014/04/30 14:13:24, arv wrote:
> On 2014/04/30 02:07:34, ojan wrote:
> > http://src.chromium.org/viewvc/blink?view=revision&revision=99612
> > 
> > It's a bummer that the spec keeps changing here. I'm not sure what we should
> do.
> 
> Sorry. I was confusing this with cloneNode which seems to have settled down on
> deep = false.
> 
> We should make importNode have deep = false too to match cloneNode, the spec
and
> Firefox.
> 
> This seems like a low risk but we should probably print a warning like Firefox
> is doing for a few releases.

Aha, yeah cloneNode and importNode are closely related anyway. I uploaded a new
patch that prints the warning.

[Inactive, 2014-04-30 14:58:28]

https://codereview.chromium.org/252713004/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/252713004/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:919: addConsoleMessage(JSMessageSource,
WarningMessageLevel, "The behavior of importNode() with no boolean argument is
about to change from doing a deep clone to doing a shallow clone.  Make sure to
pass an explicit boolean argument to keep your current behavior.");
Don't we want to use a UseCounter so that we are sure not too many websites are
calling importNode without the deep argument?

https://codereview.chromium.org/252713004/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:920: return importNode(importedNode, false, ec);
Well, it looks like you are still changing the behavior? :D

[arv (Not doing code reviews), 2014-04-30 15:05:12]

Yes, lets add a use counter for this.

[rwlbuis, 2014-04-30 15:38:43]

Added the usecounter, reverted back to deep = true behavior so I also included
the new test expected results.

[Inactive, 2014-04-30 15:47:43]

https://codereview.chromium.org/252713004/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/252713004/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:919: UseCounter::count(this,
UseCounter::DocumentImportNodeOptionalArgument);
UseCounter::countDeprecation()

https://codereview.chromium.org/252713004/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:920: addConsoleMessage(JSMessageSource,
WarningMessageLevel, "The behavior of importNode() with no boolean argument is
about to change from doing a deep clone to doing a shallow clone.  Make sure to
pass an explicit boolean argument to keep your current behavior.");
We usually add the warning message to UseCounter.cpp.

[rwlbuis, 2014-04-30 17:01:02]

Now using countDeprecation and putting the message in UseCounter.cpp.

[arv (Not doing code reviews), 2014-04-30 17:02:46]

LGTM

[Inactive, 2014-04-30 17:04:18]

https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:275: DocumentImportNodeOptionalArgument = 256,
arv probably knows, are we allowed to reuse slots like this? Are we sure this
won't confuse the tools?

The comment at the bottom of the enum says to add new values to the end.

Also there is a comment below that says "// The above items are available in M34
branch." but your UseCounter is not part of m34.

[arv (Not doing code reviews), 2014-04-30 18:11:45]

Not LGTM

https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:275: DocumentImportNodeOptionalArgument = 256,
On 2014/04/30 17:04:18, Chris Dumez wrote:
> arv probably knows, are we allowed to reuse slots like this? Are we sure this
> won't confuse the tools?
> 
> The comment at the bottom of the enum says to add new values to the end.
> 
> Also there is a comment below that says "// The above items are available in
M34
> branch." but your UseCounter is not part of m34.


This is not allowed. We need new unique IDs for these.

[rwlbuis, 2014-04-30 18:16:51]

On 2014/04/30 18:11:45, arv wrote:
> Not LGTM
> 
>
https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
> File Source/core/frame/UseCounter.h (right):
> 
>
https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.h:275: DocumentImportNodeOptionalArgument = 256,
> On 2014/04/30 17:04:18, Chris Dumez wrote:
> > arv probably knows, are we allowed to reuse slots like this? Are we sure
this
> > won't confuse the tools?
> > 
> > The comment at the bottom of the enum says to add new values to the end.
> > 
> > Also there is a comment below that says "// The above items are available in
> M34
> > branch." but your UseCounter is not part of m34.
> 
> 
> This is not allowed. We need new unique IDs for these.

Sorry about that, I uploaded CL5 so it uses a new slot.

[rwlbuis, 2014-04-30 18:17:18]

On 2014/04/30 18:16:51, rwlbuis wrote:
> On 2014/04/30 18:11:45, arv wrote:
> > Not LGTM
> > 
> >
>
https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
> > File Source/core/frame/UseCounter.h (right):
> > 
> >
>
https://codereview.chromium.org/252713004/diff/40002/Source/core/frame/UseCou...
> > Source/core/frame/UseCounter.h:275: DocumentImportNodeOptionalArgument =
256,
> > On 2014/04/30 17:04:18, Chris Dumez wrote:
> > > arv probably knows, are we allowed to reuse slots like this? Are we sure
> this
> > > won't confuse the tools?
> > > 
> > > The comment at the bottom of the enum says to add new values to the end.
> > > 
> > > Also there is a comment below that says "// The above items are available
in
> > M34
> > > branch." but your UseCounter is not part of m34.
> > 
> > 
> > This is not allowed. We need new unique IDs for these.
> 
> Sorry about that, I uploaded CL5 so it uses a new slot.

I mean Patch Set 5.

[arv (Not doing code reviews), 2014-04-30 18:19:33]

LGTM

[rwlbuis, 2014-04-30 18:26:05]

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

[commit-bot: I haz the power, 2014-04-30 18:26:33]

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

[commit-bot: I haz the power, 2014-04-30 18:32:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:32:51]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink

[rwlbuis, 2014-04-30 18:45:39]

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

[commit-bot: I haz the power, 2014-04-30 18:45:51]

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

[commit-bot: I haz the power, 2014-04-30 19:46:38]

Change committed as 173018