Closed Bug 890133 Opened 7 years ago Closed 7 years ago

Background thumbnails should prevent same URL being queued multiple times.

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

If a request is made to queue a URL which is already in the queue, the new request should be ignored.
This patch maintains a map of URLs.  If the same URL is added when the URL already exists in the queue, then we take the onDone callback (if one exists), stick it in the callbacks for the existing queue entry then return.  This means options.onDone() is called, but no other elements in |options| will be respected for the duplicate item.

The test just nukes the captured file after the first callback, then checks the file still doesn't exist on the second callback - if the duplicate was not detected and the same item was queued twice, the file *would* appear on the second callback.
Assignee: nobody → mhammond
Attachment #771954 - Flags: review?(adw)
Comment on attachment 771954 [details] [diff] [review]
Don't allow same URL to be queued twice.

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

General übernit: Please capitalize the sentences in your code comments. :-)

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +49,5 @@
>          Services.tm.mainThread.dispatch(options.onDone.bind(options, url), 0);
>        return;
>      }
> +    this._captureQueue = this._captureQueue || [];
> +    this._captureQueueByUrl = this._captureQueueByUrl || new Map();

"captureQueueByUrl" is a strange name since it's a map and not a queue, nor is it mapping URLs to capture queues.  capturesByURL?  Also, please put URL in all caps to match the style in this file.

@@ +55,5 @@
> +    // existing one, we just add the callback to that one and we are done.
> +    let existing = this._captureQueueByUrl.get(url);
> +    if (existing) {
> +      if (options.onDone) {
> +        existing.doneCallbacks.push(options.onDone);

push(options.onDone.bind(options))

Also, please remove the braces here to match the style in this file.

@@ +57,5 @@
> +    if (existing) {
> +      if (options.onDone) {
> +        existing.doneCallbacks.push(options.onDone);
> +      }
> +      // the queue must already be underway, so nothing else to do...

I don't understand the first part of this comment.  !options.onDone only means the caller doesn't care about getting notified, right?

@@ +60,5 @@
> +      }
> +      // the queue must already be underway, so nothing else to do...
> +      return;
> +    }
> +    // it's a new one...

If you really need a comment here, please be specific and say it's a new capture instead of it's a new one.  And no ellipsis necessary.  Commit! :-)

@@ +227,5 @@
>    this.options = options;
>    this.id = Capture.nextID++;
> +  this.doneCallbacks = [];
> +  if (options.onDone) {
> +    this.doneCallbacks.push(options.onDone);

push(options.onDone.bind(options))

No braces please.

@@ +300,5 @@
>  
> +    let callOnDones = function callOnDonesFn() {
> +      for (let callback of this.doneCallbacks) {
> +        try {
> +          this.options.onDone(this.url);

callback(this.url);

Or, instead of binding options to the callbacks earlier, callback.call(this.options, this.url), whichever you think is better.

::: toolkit/components/thumbnails/test/browser_thumbnails_background.js
@@ +165,5 @@
>      imports.BackgroundPageThumbs._destroyBrowserTimeout = 1000;
>  
>      yield capture(url1);
>      ok(file1.exists(), "First file should exist after capture.");
> +    file1.remove(false);

Thanks!

@@ +264,5 @@
>         "Thumbnail file should exist even though it alerted.");
>      file.remove(false);
>    },
> +
> +  function noDuplicates() {

Makes sense to me.
Attachment #771954 - Flags: review?(adw) → review+
FWIW my personal opinion about braces vs. no-braces for single-line blocks has shifted somewhat over time (towards always bracing), but in general I think we should be tolerant of a lack of consistency (even within the same file) and not try to enforce one way vs. the other.
I have a personal opinion on braces too, but inconsistency is the reason I commented on it here.  Inconsistent style within a file -- a function even, in this case -- can just make it look sloppy.

So, uh, I do not rescind my review comment to please match brace style.
https://hg.mozilla.org/mozilla-central/rev/e2ed327feb31
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.