Closed Bug 1256030 Opened 4 years ago Closed 4 years ago

Remove deprecated functions calls from NetUtil.jsm

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → mozilla
Blocks: 1254752
Status: NEW → ASSIGNED
Hey Pat, sorry I forgot that the first time around within Bug 1254752, but we also have to update NetUtil.jsm. There is also test_NetUtil.js which provides pretty good testcoverage for the functionality provided within NetUtil.jsm - all tests pass.

Btw, we should also make sure to throw an exception if the arguemnts.length != 1 so we catch callsites which still use the old API right away.

Thanks!
Attachment #8729862 - Flags: review?(mcmanus)
Will it fix Pocket?
Flags: needinfo?(mozilla)
(In reply to arni2033 from comment #2)
> Will it fix Pocket?

Oh, pocket broke as well? I thought I fixed it with [1]. Apparently not. I'll have a look.

[1] https://hg.mozilla.org/mozilla-central/rev/0b1dbcc5db72
Flags: needinfo?(mozilla)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> (In reply to arni2033 from comment #2)
> > Will it fix Pocket?

I filed Bug 1256050 and will fix Pocket there.
Pat, given comment [1] the change in that patch is probably not completely right either. What do you suggest we do for example for the case:
> NetUtil.newChannel(file);

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1254752#c25
Flags: needinfo?(mcmanus)
I guess this could be made working on a backward-compatible way by picking the triggeringPrincipal/securityFlags from the file/uri's protocol, and contentPolicyType from the extension?
Jorge, because of the update within Bug 1254752, we also have to update NetUtil.newChannel() because ioservice.newChannelFromURI() was removed within Bug 1254752. This change will cause some addons to experience problems and those addons need to be updated.

I will make sure that we update all of our devdocs to reflect the necessary changes. Anything else I need to be aware of?
Flags: needinfo?(jorge)
Pat, I discussed that briefly with Jonas and we both agree we should provide a way to also allow NetUtil.newChannel() to be called for loading an nsIFile. Hence I updated the patch to include that.

For addons the new API call would look like this:
> NetUtil.newChannel({uri: file, loadUsingSystemPrincipal: true});

Can we all agree on that?
Attachment #8729862 - Attachment is obsolete: true
Attachment #8729862 - Flags: review?(mcmanus)
Flags: needinfo?(mcmanus)
Attachment #8730351 - Flags: review?(mcmanus)
Comment on attachment 8730351 [details] [diff] [review]
bug_1256030_remove_deprecated_functions_from_netutiljsm.patch

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

makes sense to me
Attachment #8730351 - Flags: review?(mcmanus) → review+
(In reply to Patrick McManus [:mcmanus] from comment #9)
> makes sense to me

thanks Pat!
Keywords: checkin-needed
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> I will make sure that we update all of our devdocs to reflect the necessary
> changes. Anything else I need to be aware of?

That should be enough, thanks.
Flags: needinfo?(jorge)
https://hg.mozilla.org/mozilla-central/rev/043d7553482f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Please note that we are reverting changes within Bug 1257339 and restoring the deprecated NetUtil.newChannel() API for now. It's still deprecated and shouldn't be used anymore. However, addons have the desire to still use that API. So now, as a first step we are going to log a deprecation warning to the console.
Blocks: 1257339
See Also: → 1287993
You need to log in before you can comment on or make changes to this bug.