[e10s] clean up GetBaseURIFromPackage signature

RESOLVED FIXED in mozilla2.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dougt, Assigned: MikeK)

Tracking

(Blocks 1 bug)

unspecified
mozilla2.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec-)

Details

Attachments

(1 attachment, 2 obsolete attachments)

>@@ -693,157 +331,44 @@ nsChromeRegistry::ConvertChromeURL(nsIUR
>+  rv = GetBaseURIFromPackage(package, provider, path, &baseURI);

That's a really confusing signature.  Followup to just return nsIURI?
tracking-fennec: --- → ?
being that this is just code clean up, I don't think it should block
tracking-fennec: ? → 2.0-
Currently the code can return:

NS_ERROR_NOT_INITIALIZED
NS_ERROR_FAILURE
NS_OK

and in the NS_OK case, baseURI might be set to nsnull or a value actually pointing at something.

Are we sure we want it to just be able to return nsnull or a value?
Attachment #482830 - Flags: feedback?(doug.turner)
Attachment #482830 - Flags: feedback?(doug.turner) → review?(bzbarsky)
> +  NS_ASSERTION(baseURI, "GetBaseURIFromPackage returned nsnull");

Uh... why the heck is that ok?

Shouldn't the original author of this code be reviewing this, by the way?
And also as far as comment 1 goes... this is "just code cleanup" that was a precondition for a patch landing.

Things like that are why we shouldn't allow the "land now, address comments later" crap.
(In reply to comment #3)
> > +  NS_ASSERTION(baseURI, "GetBaseURIFromPackage returned nsnull");
> 
> Uh... why the heck is that ok?
> 
> Shouldn't the original author of this code be reviewing this, by the way?

Good question, which behavior would you prefer, returning an error on nsnull or ignoring the result letting the existing code cope with it?
I wrote the code that this review comment was about.  GetBaseURIFromPackage callers handle a null baseURI case, so scratch the assertion, please.
(In reply to comment #6)
> I wrote the code that this review comment was about.  GetBaseURIFromPackage
> callers handle a null baseURI case, so scratch the assertion, please.

So it is ok that it doesn't return an error in the cases that it did before?  (see comment #2)
Yes, it's fine.
Comment on attachment 482830 [details] [diff] [review]
Change as requested (fits m-c from Oct 13 2010)

I hear a volunteer!
Attachment #482830 - Flags: review?(bzbarsky) → review?(josh)
(In reply to comment #9)
> Comment on attachment 482830 [details] [diff] [review]
> Change as requested (fits m-c from Oct 13 2010)
> 
> I hear a volunteer!

Wait 10-20 min, and I'll have an updated patch without the assert
Attachment #482830 - Attachment is obsolete: true
Attachment #482935 - Flags: review?
Attachment #482830 - Flags: review?(josh)
Attachment #482935 - Flags: review?
Attachment #482936 - Flags: review?(josh)
Assignee: nobody → mkristoffersen
Attachment #482936 - Flags: review?(josh) → review+
Keywords: checkin-needed
Comment on attachment 482936 [details] [diff] [review]
Now without the assert, but otherwise the same as preivous patch

minor change, addresses bz review comments.
Attachment #482936 - Flags: approval2.0+
Pushed: http://hg.mozilla.org/mozilla-central/rev/7896216a2724
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.