Closed Bug 1256264 Opened 5 years ago Closed 5 years ago

requestId should be a string

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.1 - Mar 21
Tracking Status
firefox48 --- fixed

People

(Reporter: mao, Assigned: TheOne, Mentored)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [webRequest] [good first bug])

Attachments

(2 files, 3 obsolete files)

We've finally got requestId passed on every WebRequest event, but we pass an integer while in Chrome string (which appears still to be a monotonously increment integer as weel, just stringified) gets passed.
I can see the rationale behind it: if lots and lots (frankly, too many for a lifetime-long browser session) of requests are initiated, we might exhaust the 54 bits available for precise integer representation and start skipping ids, at some point in the next century or so. Hence, bonus point if you manage to make this integer potentially unlimited (i.e. a big integer), but I'd consider this fixed if we just achieve chrome compatibility by stringifying the id as it's fetched for the first time, i.e. in the RequestId.create() object.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8730173 - Flags: review?(kmaglione+bmo)
Assignee: nobody → awagner
Status: NEW → ASSIGNED
Attached patch patch v2 (obsolete) — Splinter Review
Fixed eslint errors
Attachment #8730230 - Flags: review?(kmaglione+bmo)
Attachment #8730173 - Attachment is obsolete: true
Attachment #8730173 - Flags: review?(kmaglione+bmo)
Comment on attachment 8730197 [details]
MozReview Request: Bug 1256264 Make requestId a string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39753/diff/1-2/
Comment on attachment 8730197 [details]
MozReview Request: Bug 1256264 Make requestId a string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39753/diff/1-2/
Comment on attachment 8730230 [details] [diff] [review]
patch v2

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

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
@@ +250,5 @@
>  
> +  let lastRequestId = -1;
> +  let lastRequestUrl = null;
> +  function validateRequestIdType(currentId) {
> +    browser.test.assertTrue(typeof lastRequestId === "string");

`browser.test.assertEq("string", typeof(lastRequestId), ...)` Same for the tests below.

Please also include a description of each test, or the test output is going to be difficult to follow.

@@ +252,5 @@
> +  let lastRequestUrl = null;
> +  function validateRequestIdType(currentId) {
> +    browser.test.assertTrue(typeof lastRequestId === "string");
> +    browser.test.assertTrue(typeof currentId === "string");
> +    browser.test.assertTrue(typeof parseInt(currentId, 10) === "number");

This will always be true. Do you mean `!isNaN(...)`?

@@ +253,5 @@
> +  function validateRequestIdType(currentId) {
> +    browser.test.assertTrue(typeof lastRequestId === "string");
> +    browser.test.assertTrue(typeof currentId === "string");
> +    browser.test.assertTrue(typeof parseInt(currentId, 10) === "number");
> +    browser.test.assertTrue(parseInt(lastRequestId, 10) !== parseInt(currentId, 10));

Why do you need parseInt here?

@@ +264,5 @@
> +      lastRequestUrl = details.url;
> +      lastRequestId = details.requestId;
> +    } else if (lastRequestUrl != details.url) {
> +      validateRequestIdType(details.requestId);
> +    }

Why these changes?

::: toolkit/modules/addons/WebRequest.jsm
@@ +43,5 @@
>  var RequestId = {
>    count: 1,
>    KEY: "mozilla.webRequest.requestId",
>    create(channel = null) {
> +    let id = (this.count++).toString();

`String(this.count++)`
Attachment #8730230 - Flags: review?(kmaglione+bmo)
Attached patch patch v3 (obsolete) — Splinter Review
Updated according to the comments, the open questions have been discussed and resolved in-person.
Attachment #8730267 - Flags: review?(kmaglione+bmo)
Attachment #8730230 - Attachment is obsolete: true
Attachment #8730267 - Flags: review?(kmaglione+bmo) → review+
Attachment #8730197 - Flags: review?(kmaglione+bmo)
Attached patch patch v4Splinter Review
Sorry for the noise, v4 failed some eslint tests which I didn't realize until after the review.
Attachment #8731113 - Flags: review?(kmaglione+bmo)
Attachment #8730267 - Attachment is obsolete: true
Attachment #8731113 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a3ed682cb181
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.