Closed Bug 1317591 Opened 3 years ago Closed 3 years ago

Get rid of IsCallerChrome usage in Navigator

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

We don't want it.
Attachment #8810723 - Flags: review?(bkelly)
Attachment #8810867 - Flags: review?(bkelly)
Attachment #8810723 - Attachment is obsolete: true
Attachment #8810723 - Flags: review?(bkelly)
This restores the throwing behavior for .userAgent/appVersion/platform that was
incorrectly removed in bug 925847.
Attachment #8810868 - Flags: review?(bkelly)
Attachment #8810867 - Attachment is obsolete: true
Attachment #8810867 - Flags: review?(bkelly)
Looks like this stuff _used_ to properly throw until bug 925847 messed it up...
Comment on attachment 8810868 [details] [diff] [review]
Get rid of IsCallerChrome usage in Navigator

Review of attachment 8810868 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.  Just a few nits.  r=me

Do we have tests that exercise these paths?

::: dom/base/Navigator.cpp
@@ +324,5 @@
>  //    Navigator::nsIDOMNavigator
>  //*****************************************************************************
>  
> +void
> +Navigator::GetUserAgent(nsString& aUserAgent, CallerType aCallerType,

nit: If we're touching this, can we make these `naAString&` here and in the other methods?  I think we should avoid using concrete string types in APIs without a good reason.  Too many people copy/paste code and it spreads.

@@ +354,5 @@
> +                             aCallerType == CallerType::System,
> +                             aUserAgent);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    aRv.Throw(rv);
> +  }  

nit: trailing white space

@@ +509,4 @@
>    nsCOMPtr<nsIHttpProtocolHandler>
>      service(do_GetService(NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "http", &rv));
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    aRv.Throw(rv);

Any reason to use aRv.Throw(rv) vs just assigning directly to aRv?
Attachment #8810868 - Flags: review?(bkelly) → review+
> Do we have tests that exercise these paths

I think so, yes.

> can we make these `naAString&` here and in the other methods?

Done.

> nit: trailing white space

Fixed.

> Any reason to use aRv.Throw(rv) vs just assigning directly to aRv?

Yes, I want to kill off the operator=(nsresult) on ErrorResult in the long term.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7ad34a4a51
Get rid of IsCallerChrome usage in Navigator.  r=bkelly
https://hg.mozilla.org/mozilla-central/rev/0a7ad34a4a51
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.