Closed Bug 1397536 Opened 7 years ago Closed 7 years ago

Accept URL strings in MatchPattern methods

Categories

(WebExtensions :: Request Handling, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This turns out to be another big hidden chunk of overhead in webRequest processing. Creating URIs is always expensive, but the XPConnect overhead of doing it from the JS side is more expensive than creating the URIs themselves.

Accepting URL strings directly in the match methods improves things considerably.
Comment on attachment 8905310 [details]
Bug 1397536: Avoid newURI overhead for MatchPattern.

https://reviewboard.mozilla.org/r/177108/#review182094

Another approach could be to pass through the URIs from WebRequest.jsm to ext-webrequest, and only convert them to urls prior to calling the listener.  Then we'd avoid having to convert back to nsiuri.  Having matches accept either is good too.
Attachment #8905310 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8905310 [details]
Bug 1397536: Avoid newURI overhead for MatchPattern.

https://reviewboard.mozilla.org/r/177108/#review182094

Hm. You're right, re-using the existing nsIURIs is considerably faster.
Attachment #8905310 - Flags: review+ → review?(mixedpuppy)
Comment on attachment 8905310 [details]
Bug 1397536: Avoid newURI overhead for MatchPattern.

https://reviewboard.mozilla.org/r/177108/#review182108

::: toolkit/modules/addons/WebRequest.jsm:677
(Diff revision 2)
>  
>    getRequestData(channel, extraData) {
>      let data = {
>        requestId: String(channel.id),
>        url: channel.finalURL,
> +      uri: channel.finalURI,

can we uppercase URI?  it's hard on the eyes especially next to url.  Also wish finalURL was finalUrl.
Attachment #8905310 - Flags: review?(mixedpuppy) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Also wish finalURL was finalUrl.

So do I, but it's a public API at this point. :(
Comment on attachment 8905310 [details]
Bug 1397536: Avoid newURI overhead for MatchPattern.

https://reviewboard.mozilla.org/r/177108/#review182130

Great find!
Attachment #8905310 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/7282bbabab15
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: webext-perf
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.