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

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

({dev-doc-needed})

32 Branch
mozilla40
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

4 years ago
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?
(Assignee)

Comment 1

4 years ago
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.
Attachment #8584676 - Flags: feedback?(bugs)
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 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
Attachment #8584676 - Flags: feedback?(bugs) → feedback+

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 4

4 years ago
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.
Attachment #8587494 - Flags: feedback?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8584676 - Attachment is obsolete: true
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.
(Assignee)

Comment 6

4 years ago
I looked into generating the list automatically, and I think I'll open a
followup for that.
Attachment #8589597 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8587494 - Attachment is obsolete: true
Attachment #8587494 - Flags: feedback?(bugs)
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...
(Assignee)

Comment 8

4 years ago
Indeed, I intended to, but I forgot.
Attachment #8589680 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8589597 - Attachment is obsolete: true
Attachment #8589597 - Flags: review?(bugs)
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).
(Assignee)

Comment 10

4 years ago
This should be better.
Attachment #8589733 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8589680 - Attachment is obsolete: true
Attachment #8589680 - Flags: review?(bugs)
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 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);
}
Attachment #8589733 - Flags: review?(bugs) → review-
(Assignee)

Comment 13

4 years ago
That should be better.
Attachment #8590825 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8589733 - Attachment is obsolete: true
deprecatedMethodWithContext/staticDeprecatedMethodWithContext is still only present in one of the test files, not all three...
Comment on attachment 8590825 [details] [diff] [review]
Allow to set an interface member as [Deprecated] in WebIDL. r=

So, still missing something bz wants.
Attachment #8590825 - Flags: review?(bugs)
(Assignee)

Comment 16

4 years ago
This addresses bz' last comments.
Attachment #8591558 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #8590825 - Attachment is obsolete: true
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 ;) ).
Attachment #8591558 - Flags: review?(bugs) → review?(bzbarsky)
Comment on attachment 8591558 [details] [diff] [review]
Allow to set an interface member as [Deprecated] in WebIDL. r=

r=me
Attachment #8591558 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/7bb9166310a2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.