The default bug view has changed. See this FAQ.

Warn developers about the deprecation of non-__exposedProps__ COWs

RESOLVED FIXED in mozilla15

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Split off from bug 553102 comment 94. Patch coming up.
Version: unspecified → Trunk
Created attachment 627210 [details] [diff] [review]
Warn when __exposedProps__ is missing. v1

Attaching a patch, with tests. Flagging bz for review.
Attachment #627210 - Flags: review?(bzbarsky)
Comment on attachment 627210 [details] [diff] [review]
Warn when __exposedProps__ is missing. v1

Is it worth to factor out the "get the document" logic into some utility method?

Please add a localization note about not translating __exposedProps__

r=me
Attachment #627210 - Flags: review?(bzbarsky) → review+
Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6c7d23818d34
Flags: in-testsuite+
Target Milestone: --- → mozilla15
Can we make this an error instead of a warning? It would make it much easier for the review team to spot this.
An error in what sense?  If it were actually an error from the point of view of the JS, it would stop execution of the JS, breaking the extension...
In that it would show up in the Error Console as an error and not a warning. This is possible to do without stopping execution, right?
Yeah, that could be done.  We could just add an optional argument for this to WarnOnceAbout...
But it would _really_ confuse developers, btw, since they do expect errors to stop execution.
I may be mistaken, but isn't the message introduced on bug 675075 an error?

I think the message in attachment 627210 [details] [diff] [review] is clear enough about what needs fixing, so I wouldn't expect that much confusion. If this isn't much of a problem, I'd prefer to go with an error. We ask reviewers to always check for errors, and checking for warnings would be impractical (too much noise).
> I may be mistaken, but isn't the message introduced on bug 675075 an error?

It's a warning:

>+                                      nsIScriptError::warningFlag,
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> We ask
> reviewers to always check for errors, and checking for warnings would be
> impractical (too much noise).

Well, checking early for (strict, compile/runtime) warnings would be my much preferred choice :->
(In reply to Serge Gautherie (:sgautherie) from comment #11)
> (In reply to Jorge Villalobos [:jorgev] from comment #9)
> > We ask
> > reviewers to always check for errors, and checking for warnings would be
> > impractical (too much noise).
> 
> Well, checking early for (strict, compile/runtime) warnings would be my much
> preferred choice :->

(If there is no way to (eventually) fix the "too much" noise,
then) a solution would be to add an enabling preference in the meantime:
that could also help Firefox and other apps to test/prepare themselves in the meantime!
I didn't understand. A preference that does what?
(In reply to Jorge Villalobos [:jorgev] from comment #13)
> I didn't understand. A preference that does what?

A hidden pref to get the warning (as in this bug), or the error (as in bug 553102).
By default, I would suggest to enable the warning on opt builds and the error on debug ones.
https://hg.mozilla.org/mozilla-central/rev/6c7d23818d34
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Created attachment 628684 [details] [diff] [review]
patch. v1
Attachment #628684 - Flags: review?(bzbarsky)
Comment on attachment 628684 [details] [diff] [review]
patch. v1

Wrong bug. Sorry.
Attachment #628684 - Attachment is obsolete: true
Attachment #628684 - Flags: review?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.