Closed Bug 584982 Opened 10 years ago Closed 8 years ago

mark deprecated places interfaces and methods with [deprecated]

Categories

(Toolkit :: Places, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: timeless, Assigned: timeless)

References

Details

(Whiteboard: not-ready)

Attachments

(2 files)

We have a way to mark interfaces and methods as deprecated which some compilers
are able to use to complain about deprecation. This bug is for sprinkling that
marker into places idl files.

Please note that at this time we only properly trigger these compile time
warnings with msvc (bug 584953 covers the fixing the gcc side).
Attached patch patchSplinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #463473 - Flags: review?
Attachment #463473 - Flags: review? → review?(mano)
actually, this never landed :(
Keywords: checkin-needed
Attachment #463473 - Flags: approval2.0?
Someone with enough nerve could probably get away with pushing it as NPOTB, but that's a stretch for checkin-needed.
Keywords: checkin-needed
Depends on: 578478
No longer depends on: 584960
Comment on attachment 463473 [details] [diff] [review]
patch

Per bug 584998 comment 2 this doesn't work as expected yet
Attachment #463473 - Flags: approval2.0? → approval2.0-
(In reply to comment #5)
> Per bug 584998 comment 2 this doesn't work as expected yet
We are not warning free right now anyway in Places (we already have methods marked as deprecated), so I'm not sure why that matters here.
I didn't test this on non-windows, but this hides the warnings in Places and makes us not warn at all!
Attachment #510408 - Flags: review?(mak77)
Comment on attachment 510408 [details] [diff] [review]
Hide deprecation warnings v1.0

I think you should ifdef each #pragma with #ifdef _MSC_VER and add a comment on each disable that this is for xpidl deprecation

>diff --git a/toolkit/components/places/src/nsNavBookmarks.h b/toolkit/components/places/src/nsNavBookmarks.h
>diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h

do you have to wrap all the class here or is that just for simplification?

btw, with these addressed I don't see a problem. Deprecating interfaces methods is an important sign in the "contract" we give to external devs.
Attachment #510408 - Flags: review?(mak77) → review+
another couple things:
- could you adopt the patch in bug 539517 and pragma that one as well?
- does deprecation require SR?
Btw, this bug doesn't seem the right place to mass up patches with some relation. I think this disable warnings patch should go to a separate bug and block both this bug and bug 539517, the three should land at the same time to avoid warnings churn.
(In reply to comment #8)
> I think you should ifdef each #pragma with #ifdef _MSC_VER and add a comment on
> each disable that this is for xpidl deprecation
What benefit does this give us?

> do you have to wrap all the class here or is that just for simplification?
It doesn't work unless you wrap the whole class.
(In reply to comment #11)
> (In reply to comment #8)
> > I think you should ifdef each #pragma with #ifdef _MSC_VER and add a comment on
> > each disable that this is for xpidl deprecation
> What benefit does this give us?

some compiler could not like the #pragma, since the issue is only on a compiler, it's better to limit the pragma to that compiler.
Whiteboard: not0
Marking this not-ready as the second patch seems to be half-done.
Whiteboard: not0 → not-ready
we don't need this bug anymore.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.