Accept URL strings in MatchPattern methods

RESOLVED FIXED in Firefox 57

Status

enhancement
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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+
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8905310 - Flags: review+ → review?(mixedpuppy)

Comment 5

2 years ago
mozreview-review
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+
(Assignee)

Comment 6

2 years ago
(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 7

2 years ago
mozreview-review
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

2 years ago
Blocks: webext-perf

Comment 10

2 years ago
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)
(Assignee)

Updated

a year ago
Flags: needinfo?(kmaglione+bmo) → qe-verify-

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.