Mark DOMStringList.contains() as deprecated, add console warning.

Mark DOMStringList.contains() as deprecated, add console warning.

Intent to Deprecate:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/j23bosJMX-8

We'd like to remove DOMStringList from the web platform in favor of
plain old arrays of strings. Usage of the contains() method is low
but non-zero.

Additionally, deprecate the named indexed getter ('item()') in
favor of an anonymous variant, to further align with a plain array.

BUG=460726
R=philipj@opera.com

[philipj_slow, 2015-03-01 00:30:21]

Yay, thanks! Can you send an Intent to Deprecate to blink-dev? The template is
in http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation

[jsbell, 2015-03-02 18:23:34]

On 2015/03/01 00:30:21, philipj_UTC7 wrote:
> Yay, thanks! Can you send an Intent to Deprecate to blink-dev? The template is
> in http://www.chromium.org/blink#TOC-Launch-Process:-Deprecation

"Intent" sent.

Based on https://codereview.chromium.org/964133002/  I should be able to
simplify the getter deprecation, but when I tried that here it didn't work...
probably did something slightly wrong. I'll iterate, add the link to the Intent,
etc.

[philipj_slow, 2015-03-06 03:03:50]

LGTM

It would also be useful to give DOMStringList::create a flag and to add more use
counters in DOMStringList::item (excluding anonymousIndexedGetter somehow) to
see if it's IDB or Location.ancestorOrigins that's using contains. In this CL or
separately is fine.

https://codereview.chromium.org/966843002/diff/40001/LayoutTests/storage/inde...
File LayoutTests/storage/indexeddb/resources/shared.js (left):

https://codereview.chromium.org/966843002/diff/40001/LayoutTests/storage/inde...
LayoutTests/storage/indexeddb/resources/shared.js:123:
db.deleteObjectStore(db.objectStoreNames.item(0));
Can you leave one use of item() in the tests so that it can't accidentally
regress? That would also show that the deprecation message works as it should.

https://codereview.chromium.org/966843002/diff/40001/Source/core/dom/DOMStrin...
File Source/core/dom/DOMStringList.idl (right):

https://codereview.chromium.org/966843002/diff/40001/Source/core/dom/DOMStrin...
Source/core/dom/DOMStringList.idl:35: // FIXME: If Array.prototype.includes is
added, add includes() and update deprecation message.
I don't think it's a given that we'll want to add DOMStringList.includes or that
doing so would help the transition, so... remove this FIXME, wait and see?

[jsbell, 2015-03-06 17:19:20]

Done and done.

I'll iterate on this to see if I can simplify the getter impl, and add the
create() flag. I'll do it in this CL since there's no rush to land this.

https://codereview.chromium.org/966843002/diff/40001/LayoutTests/storage/inde...
File LayoutTests/storage/indexeddb/resources/shared.js (left):

https://codereview.chromium.org/966843002/diff/40001/LayoutTests/storage/inde...
LayoutTests/storage/indexeddb/resources/shared.js:123:
db.deleteObjectStore(db.objectStoreNames.item(0));
On 2015/03/06 03:03:49, philipj_UTC7 wrote:
> Can you leave one use of item() in the tests so that it can't accidentally
> regress? That would also show that the deprecation message works as it should.

Sure. The behavior of item() is covered in fast/dom/domstringlist.html but
that's a textharness.js-based test so the deprecation is not captured.

https://codereview.chromium.org/966843002/diff/40001/Source/core/dom/DOMStrin...
File Source/core/dom/DOMStringList.idl (right):

https://codereview.chromium.org/966843002/diff/40001/Source/core/dom/DOMStrin...
Source/core/dom/DOMStringList.idl:35: // FIXME: If Array.prototype.includes is
added, add includes() and update deprecation message.
On 2015/03/06 03:03:49, philipj_UTC7 wrote:
> I don't think it's a given that we'll want to add DOMStringList.includes or
that
> doing so would help the transition, so... remove this FIXME, wait and see?

Done.

[jsbell, 2015-06-05 21:58:23]

Rebased this. We obviously don't want to land as-is (with both Measure and
Deprecate).

Maybe we should separate tracking for indexed getter vs items(), and land that
soonish?

[philipj_slow, 2015-06-05 22:57:37]

If you want to send an Intent to Deprecate both DOMStringList.item() and
DOMStringList.contains() I would support that. Those are what stand in the way
of using FrozenArray<DOMString> or something, and usage of contains() is low
enough that it's possible that removal would work out.

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

https://codereview.chromium.org/966843002/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:733: DOMStringListItem = 827,
This is the same as V8DOMStringList_Item_Method (822)

[jsbell, 2015-06-12 20:14:36]

On 2015/03/06 03:03:50, philipj wrote:
> It would also be useful to give DOMStringList::create a flag and to add more
use
> counters in DOMStringList::item (excluding anonymousIndexedGetter somehow) to
> see if it's IDB or Location.ancestorOrigins that's using contains.

Done.

How's this look? If we're happy I'll send (resend?) the "Intent".

The "will be removed" in the messages is optimistic, of course. Fingers
crossed...

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

https://codereview.chromium.org/966843002/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:733: DOMStringListItem = 827,
On 2015/06/05 22:57:37, philipj wrote:
> This is the same as V8DOMStringList_Item_Method (822)

Switched to using that. The resulting 'DeprecateAs=' is a little odd. I
considered adding [Deprecate] but it didn't seem worth it.

[jsbell, 2015-06-12 20:35:39]

https://codereview.chromium.org/966843002/diff/100001/Source/bindings/modules...
File Source/bindings/modules/v8/V8BindingForModules.cpp (right):

https://codereview.chromium.org/966843002/diff/100001/Source/bindings/modules...
Source/bindings/modules/v8/V8BindingForModules.cpp:72:
RefPtrWillBeRawPtr<DOMStringList> keyPaths =
DOMStringList::create(DOMStringList::IndexedDB);
Side note: I'm rather surprised to see this here. This was never documented or
specified to be a DOMStringList and Gecko doesn't implement it that way either,
and we have no tests asserting that it is a DOMStringList. This should be a
plain (but possibly frozen?) array.

I'll spin up a separate CL to change it.

[philipj_slow, 2015-06-12 21:08:36]

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

https://codereview.chromium.org/966843002/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:733: DOMStringListItem = 827,
On 2015/06/12 20:14:36, jsbell wrote:
> On 2015/06/05 22:57:37, philipj wrote:
> > This is the same as V8DOMStringList_Item_Method (822)
> 
> Switched to using that. The resulting 'DeprecateAs=' is a little odd. I
> considered adding [Deprecate] but it didn't seem worth it.

Yep, this is the first [Measure] thing being deprecated. Someone™ should make
[Deprecate] work but we need not block deprecations on that.

[philipj_slow, 2015-06-12 21:23:05]

This LGTM, and will provide better data than
https://codereview.chromium.org/1162203003

So I guess poke blink-dev to see if anyone has any further concerns about the
deprecation bit specifically.