1148496 - Allow to set an interface member as [Deprecated] in WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Paul Adenot (:padenot)]

ehsan suggested that we can use WarnOnceAbout for this. One problem is that it's on the document, and that it would be better to have it on the global object, but this can be done later.

I know nothing about codegen, so I'm listing down what needs to be done:
- make "[Deprecated]" it a legal token for an extended attribute in the parser. I think it makes sense to be able to mark an interface, a const, an attribute and a method as deprecated.
- then I need to include the right header in the generated binding file (nsIDocument.h to get WarnOnceAbout).
- then I need to find the global or the document from what's given to me in the params. Looks like I can do:

>   GlobalObject global(cx, obj);
>  if (global.Failed()) {
>    return false;
>  }
>    nsCOMPtr<nsIGlobalObject> globalHolder = 
>      do_QueryInterface(global.GetAsSupports());

and then get downcast my way to the object I want.

- then I need the codegen to spit out some lines of code anywhere (I think) in the definition of the member that does the WarnOnceAbout

Does that sounds like it would go somewhere?

Comment 1

[Paul Adenot (:padenot)]

Attachment: WIP Support marking things as deprecated in WebIDL. r=

This does the parser and include bit, and then I think I need to plug my
behaviour around line 11139, but I'm not too sure, I need to read a bit more of
this meaty file.

smaug, bz seems like he does not do reviews at the moment, does my plan looks
reasonable (see first comment)? Am I missing the obvious? Feel free to forward
to the right person if you're not a right person, I don't know who's who in this
area.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Do we want just Deprecated or some explanation why it is deprecated or perhaps localizeable identifier which would then refer to http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties 
(like http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties#128)

I think deprecation message would be good, it could link to the right MDN page or relevant spec etc.

If we don't support Workers at first step, better to still verify we don't end up doing anything
not-thread-safe. Though, if we access nsIDocument from global object, that will just fail safely
(assuming we do something like QI global to nsPIDOMWindow and access ExtantDoc).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8584676 [details] [diff] [review]
WIP Support marking things as deprecated in WebIDL. r=

if we go with the Deprecated="someIdentifier" approach, perhaps we could
generate a list similar to http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDeprecatedOperationList.h and then nsDeprecatedOperationList.h could actually include that header.

As an example of a generated list of things: GeneratedEventList.h

Comment 4

[Paul Adenot (:padenot)]

Attachment: Support marking things as deprecated in WebIDL. r=

So, this appear to work fine. Next up, I can work on automatically generating
the header file, and then move WarnOnceAbout from the document to the global,
but I'd like to deprecated what I wanted to deprecate on the Web Audio API as
soon as possible.

Comment 5

[Boris Zbarsky [:bzbarsky]]

A few drive-by comments:

1) [Deprecated] on an interface doesn't seem to do anything useful with that patch; we probably shouldn't support it there in the parser until it does.

2) I'm fairly certain this won't compile if someone adds [Deprecated] to a static attribute or method, because it will redefine "global".  We should fix that, and add tests that would have caught it (in TestCodeGen/JSImplGen/ExampleGen.webidl next to where the other static bits are).  And also tests for the non-static case.  Note that the value of "global" we want here is not the same value of "global" as the one static methods/attributes use, so what we should probably do is put the "global" we're adding into a separate scope or something.

3) You don't need the extra QI to nsIGlobalObject.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Allow to set an interface member as [Deprecated] in WebIDL. r=

I looked into generating the list automatically, and I think I'll open a
followup for that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Another drive-by comment: I don't see what [Deprecated] would do on an IDLConst, exactly, so we shouldn't add parser support for it.

Also, is there a reason you didn't add the tests to TextCodeGen.webidl and TestJSImplGen.webidl?   You really should...

Comment 8

[Paul Adenot (:padenot)]

Attachment: Allow to set an interface member as [Deprecated] in WebIDL. r=

Indeed, I intended to, but I forgot.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So... please make the changes to the three test files identical as much as possible (e.g. put them in the same place in the files).  And do add the static bits to all three if possible (you may need to comment them out for TestJSImplGen; see the other static stuff in that file).

Comment 10

[Paul Adenot (:padenot)]

Attachment: Allow to set an interface member as [Deprecated] in WebIDL. r=

This should be better.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8589733 [details] [diff] [review]
Allow to set an interface member as [Deprecated] in WebIDL. r=

deprecatedMethodWithContext/staticDeprecatedMethodWithContext are still only on TestExampleGen....

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8589733 [details] [diff] [review]
Allow to set an interface member as [Deprecated] in WebIDL. r=

Needs still bz' comments addressed.
The C++ code in  if idlNode.getExtendedAttribute("Deprecated"):
uses indentation inconsistently. 2 spaces please.

And per comment 5, 
"3) You don't need the extra QI to nsIGlobalObject."
QIing is a bit slow, so better to avoid extra QIing when possible.

And I would probably write it
if (pWindow && pWindow->GetExtantDoc()) {
  pWindow->GetExtantDoc()->WarnOnceAbout(nsIDocument::e%s);
}

Comment 13

[Paul Adenot (:padenot)]

Attachment: Allow to set an interface member as [Deprecated] in WebIDL. r=

That should be better.

Comment 14

[Boris Zbarsky [:bzbarsky]]

deprecatedMethodWithContext/staticDeprecatedMethodWithContext is still only present in one of the test files, not all three...

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8590825 [details] [diff] [review]
Allow to set an interface member as [Deprecated] in WebIDL. r=

So, still missing something bz wants.

Comment 16

[Paul Adenot (:padenot)]

Attachment: Allow to set an interface member as [Deprecated] in WebIDL. r=

This addresses bz' last comments.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8591558 [details] [diff] [review]
Allow to set an interface member as [Deprecated] in WebIDL. r=

bz said on IRC he could review this (as he has been doing already ;) ).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8591558 [details] [diff] [review]
Allow to set an interface member as [Deprecated] in WebIDL. r=

r=me

Comment 19

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb9166310a2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bb9166310a2