Last Comment Bug 587345 - deAgnostify nsXULPDGlobalObject some
: deAgnostify nsXULPDGlobalObject some
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla5
Assigned To: :Ms2ger
:
Mentors:
Depends on: post2.0
Blocks: 570738 620000
  Show dependency treegraph
 
Reported: 2010-08-14 09:58 PDT by :Ms2ger
Modified: 2011-04-12 10:23 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (8.97 KB, patch)
2010-08-14 09:58 PDT, :Ms2ger
jst: review+
jst: approval2.0-
Details | Diff | Splinter Review
Patch for checkin (10.35 KB, patch)
2011-03-22 11:06 PDT, :Ms2ger
no flags Details | Diff | Splinter Review
Patch for checkin (10.35 KB, patch)
2011-03-22 12:02 PDT, :Ms2ger
no flags Details | Diff | Splinter Review
Patch for checkin (10.41 KB, patch)
2011-03-23 01:46 PDT, :Ms2ger
no flags Details | Diff | Splinter Review

Description :Ms2ger 2010-08-14 09:58:43 PDT
Created attachment 466010 [details] [diff] [review]
Patch v1

Like bug #575431.
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2010-08-16 14:57:04 PDT
Comment on attachment 466010 [details] [diff] [review]
Patch v1

While changing nsnull being returned as an nsresult to NS_OK is a good chang :) I don't think we should be changing nsnull to NULL when actually dealing with pointers in C++ code.

r=jst either way.
Comment 2 :Ms2ger 2010-08-17 01:12:57 PDT
Comment on attachment 466010 [details] [diff] [review]
Patch v1

I understood we were moving toward more standard C++, including NULLs, but that might just be bsmedberg.
Comment 3 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-08-18 08:52:21 PDT
What's the win here that would induce us to take this now, instead of after branching?
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2010-08-24 16:53:34 PDT
Comment on attachment 466010 [details] [diff] [review]
Patch v1

I think we should wait until we've branched with this.
Comment 5 :Ms2ger 2010-08-25 00:57:20 PDT
Sure.
Comment 6 :Ehsan Akhgari 2011-03-22 08:16:48 PDT
I tried to land this on cedar, but it doesn't apply cleanly.  Please submit a new version of the patch (or land it on cedar yourself).
Comment 7 :Ms2ger 2011-03-22 11:06:32 PDT
Created attachment 520961 [details] [diff] [review]
Patch for checkin

This one should apply. Thanks!
Comment 9 :Ehsan Akhgari 2011-03-22 11:55:55 PDT
Backed out because it broke the build:

http://hg.mozilla.org/projects/cedar/rev/4f0c8bd1a8fa
Comment 10 :Ms2ger 2011-03-22 12:02:39 PDT
Created attachment 520985 [details] [diff] [review]
Patch for checkin

That's why we should not have assertions.
Comment 11 :Ehsan Akhgari 2011-03-22 12:19:29 PDT
Pushed to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=8e11ded21a03
Comment 12 :Ehsan Akhgari 2011-03-22 13:09:16 PDT
New try job: http://tbpl.mozilla.org/?tree=MozillaTry&rev=c355ae2f3d6f
Comment 13 :Ms2ger 2011-03-23 01:46:18 PDT
Created attachment 521127 [details] [diff] [review]
Patch for checkin

Third time's the charm, right?

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