Last Comment Bug 758563 - Warn developers about the deprecation of non-__exposedProps__ COWs
: Warn developers about the deprecation of non-__exposedProps__ COWs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on:
Blocks: 553102
  Show dependency treegraph
 
Reported: 2012-05-25 03:45 PDT by Bobby Holley (PTO through June 13)
Modified: 2012-05-31 05:36 PDT (History)
6 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Warn when __exposedProps__ is missing. v1 (8.49 KB, patch)
2012-05-25 06:12 PDT, Bobby Holley (PTO through June 13)
bzbarsky: review+
Details | Diff | Review
patch. v1 (3.17 KB, patch)
2012-05-31 05:35 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review

Description Bobby Holley (PTO through June 13) 2012-05-25 03:45:35 PDT
Split off from bug 553102 comment 94. Patch coming up.
Comment 1 Bobby Holley (PTO through June 13) 2012-05-25 06:12:47 PDT
Created attachment 627210 [details] [diff] [review]
Warn when __exposedProps__ is missing. v1

Attaching a patch, with tests. Flagging bz for review.
Comment 2 Boris Zbarsky [:bz] 2012-05-25 09:29:38 PDT
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
Comment 3 Bobby Holley (PTO through June 13) 2012-05-25 09:44:18 PDT
Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6c7d23818d34
Comment 4 Jorge Villalobos [:jorgev] 2012-05-25 10:41:54 PDT
Can we make this an error instead of a warning? It would make it much easier for the review team to spot this.
Comment 5 Boris Zbarsky [:bz] 2012-05-25 10:43:38 PDT
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...
Comment 6 Jorge Villalobos [:jorgev] 2012-05-25 10:46:20 PDT
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?
Comment 7 Boris Zbarsky [:bz] 2012-05-25 10:56:28 PDT
Yeah, that could be done.  We could just add an optional argument for this to WarnOnceAbout...
Comment 8 Boris Zbarsky [:bz] 2012-05-25 10:56:55 PDT
But it would _really_ confuse developers, btw, since they do expect errors to stop execution.
Comment 9 Jorge Villalobos [:jorgev] 2012-05-25 11:02:57 PDT
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).
Comment 10 Boris Zbarsky [:bz] 2012-05-25 11:16:21 PDT
> I may be mistaken, but isn't the message introduced on bug 675075 an error?

It's a warning:

>+                                      nsIScriptError::warningFlag,
Comment 11 Serge Gautherie (:sgautherie) 2012-05-25 13:01:59 PDT
(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 :->
Comment 12 Serge Gautherie (:sgautherie) 2012-05-25 13:06:00 PDT
(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!
Comment 13 Jorge Villalobos [:jorgev] 2012-05-25 16:47:17 PDT
I didn't understand. A preference that does what?
Comment 14 Serge Gautherie (:sgautherie) 2012-05-25 17:05:45 PDT
(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.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-05-26 05:14:15 PDT
https://hg.mozilla.org/mozilla-central/rev/6c7d23818d34
Comment 16 Bobby Holley (PTO through June 13) 2012-05-31 05:35:40 PDT
Created attachment 628684 [details] [diff] [review]
patch. v1
Comment 17 Bobby Holley (PTO through June 13) 2012-05-31 05:36:22 PDT
Comment on attachment 628684 [details] [diff] [review]
patch. v1

Wrong bug. Sorry.

Note You need to log in before you can comment on or make changes to this bug.