Closed Bug 1345922 Opened 7 years ago Closed 7 years ago

Use nsIUrlClassifierDBService.beginUpdate properly in UrlClassifierTestUtils.jsm

Categories

(Toolkit :: Safe Browsing, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(1 file)

Assignee: nobody → hchang
Blocks: 1339050
Blocks: 1203438
Attachment #8845502 - Flags: review?(francois)
Comment on attachment 8845502 [details]
Bug 1345922 - Avoid concurrent update and take the failed beginUpdate into account.

https://reviewboard.mozilla.org/r/118650/#review121676

::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:60
(Diff revision 2)
> +      }
> +      return this.useTestDatabase(tables[tableIndex])
> +        .then(() => {
> +          tableIndex++;
> +          return doOneUpdate();
> +        }, aErrMsg => {

The indentation is off here and makes this hard to read.

This line is still within `.then()` (line 57) and is the second parameter for that function.

::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:63
(Diff revision 2)
> +          tableIndex++;
> +          return doOneUpdate();
> +        }, aErrMsg => {
> +          dump("Rejected: " + aErrMsg + ". Retry later.\n");
> +          return new Promise(resolve => {
> +            timer.initWithCallback(resolve, 100, Ci.nsITimer.TYPE_ONE_SHOT);

Is 100ms enough of a wait? Should this be 500ms or 1000ms to make sure the current update finishes?

::: toolkit/components/url-classifier/tests/UrlClassifierTestUtils.jsm:65
(Diff revision 2)
> +        }, aErrMsg => {
> +          dump("Rejected: " + aErrMsg + ". Retry later.\n");
> +          return new Promise(resolve => {
> +            timer.initWithCallback(resolve, 100, Ci.nsITimer.TYPE_ONE_SHOT);
> +          })
> +          .then(doOneUpdate);

This should be indented at the same level as the `.then()` from line 57.
Attachment #8845502 - Flags: review?(francois) → review+
Comment on attachment 8845502 [details]
Bug 1345922 - Avoid concurrent update and take the failed beginUpdate into account.

https://reviewboard.mozilla.org/r/118650/#review121676

> The indentation is off here and makes this hard to read.
> 
> This line is still within `.then()` (line 57) and is the second parameter for that function.

I just followed the MDN style

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then

```
p1.then(function(value) {
  console.log(value); // Success!
}, function(reason) {
  console.log(reason); // Error!
});
```

except for putting ".then()" in the new line, which is also common in the codebase

http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/layout/style/test/animation_utils.js#259

> Is 100ms enough of a wait? Should this be 500ms or 1000ms to make sure the current update finishes?

If 100ms is not enough, the promise (line 65) will be rejected and a new timer will be init again. In other words, it's a recursive promise chain. doOneUpdate() is basically a indefinite number of promise chain and will eventually get resolved when "tableIndex == tables.length". In "all other cases", a new promise will return instead, which means the top level promise is still pending.

> This should be indented at the same level as the `.then()` from line 57.

It is one level less then the reject handler (i.e. the same level of the promise for timer.)
Hi Francois,

I didn't change anything based on your comments. I'd like to know if you agree with my explanation.
I will land it right away if you buy it :)

Thanks!
Flags: needinfo?(francois)
Comment on attachment 8845502 [details]
Bug 1345922 - Avoid concurrent update and take the failed beginUpdate into account.

https://reviewboard.mozilla.org/r/118650/#review121676

> I just followed the MDN style
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then
> 
> ```
> p1.then(function(value) {
>   console.log(value); // Success!
> }, function(reason) {
>   console.log(reason); // Error!
> });
> ```
> 
> except for putting ".then()" in the new line, which is also common in the codebase
> 
> http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/layout/style/test/animation_utils.js#259

You're right. It turns out I read it wrong the first time and that's why I thought it was badly indented.

So much for promises making everything easier to read! :)

> If 100ms is not enough, the promise (line 65) will be rejected and a new timer will be init again. In other words, it's a recursive promise chain. doOneUpdate() is basically a indefinite number of promise chain and will eventually get resolved when "tableIndex == tables.length". In "all other cases", a new promise will return instead, which means the top level promise is still pending.

Right, I was just thinking that we'll be re-trying 10 times per second. That seems a bit high. Maybe we could re-try every 500ms instead? It would be more likely that it would only have to wait once or twice before going through.
(In reply to Henry Chang [:henry][:hchang] from comment #5)
> I didn't change anything based on your comments. I'd like to know if you
> agree with my explanation.

Yes, I was wrong about indentation.

I'll let you pick the delay you think is best (100ms or 500ms) and land the patch. r+
Flags: needinfo?(francois)
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffb8aa89b8c1
Avoid concurrent update and take the failed beginUpdate into account. r=francois
https://hg.mozilla.org/mozilla-central/rev/ffb8aa89b8c1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: