Closed Bug 906615 Opened 6 years ago Closed 6 years ago

Background thumbnail service shouldn't overwrite existing thumbnails, or at least foreground thumbnails should be preferred

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: ts.bugzilla, Assigned: adw)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

All thumbnails on the New Tab page have been replaced after the weekend because the existing ones are considered stale after 2 days.
This is a extension of bug 897880, where thumbnails were replaced with screenshots of error messages... Here my thumbnails were replaced with many login prompts after just two days away from the computer (my work PC). Additionally all thumbnails look bad because:
- the scaling is done to the full screen size (see bug 879546)
- some pages are full of ads (bug 904341)
Component: General → Tabbed Browser
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → General
Ever confirmed: true
Product: Firefox → Toolkit
Thanks for filing.  Other than the two other bugs you mention, however, what exactly is the bug here?  You had thumbnails that you were happy with that showed logged-in pages (e.g., a calendar from your logged-in Google Calendar account), and they were replaced by thumbnails of login prompts?
I think the bug is about the current "staleness threshold" - or if it should be there at all.
Thinking more about the New Tab page, and it's purpose for me (/the user), it is about big easily recognizable click targets. If the system replaces the image of the pages how the user remembers them with unrecognizable ones _while the user is away from the browser_, say over the weekend or over the holidays, the New Tab page fails its intended purpose.
Thanks for explaining.  This sounds like the key point:

(In reply to Thomas Stache from comment #2)
> _while the user is away from the browser_,

You haven't done anything, but the thumbnails have changed.  This is unlike the "foreground" service, which also captures new thumbnails often, but only after the user visits the page and sees for himself what it looks like.

Let me more precisely summarize the bug.  If I've misunderstood or you disagree with the new summary, let me know.
Summary: Background thumbnails overwrite existing ones - considered stale too soon → If a page already has a thumbnail, the background thumbnail service should capture a new thumbnail only after the user visits it
Nicely put!
I'm not sure the new topic captures the essence of this bug.

"If a page already has a thumbnail, the background thumbnail service should capture a new thumbnail only after the user visits it" implies that immediately after I visit Facebook it's OK for the background service to recapture the thumbnail - but by design, the background service is *still* going to capture the Facebook login page, *not* the main page.

ie, the foreground service captures what the user sees.  The background service often captures something completely unlike that the user sees.  The backgrounds service works well for, say, news sites, but less well for sites like Facebook.

I don't have an answer to this.  The only thing guaranteed to capture what the user actually sees is the foreground service - but that's slow, janky, and may have truly sensitive data.  I'm starting to think the b/g service should only capture missing thumbnails, but that implies the foreground service (and thus the problems it has) can't go away.
(In reply to Mark Hammond (:markh) from comment #5)
> I'm starting to think the b/g service
> should only capture missing thumbnails, but that implies the foreground
> service (and thus the problems it has) can't go away.

This sounds right. Didn't we conclude a while ago that foreground thumbnailing going away wasn't a goal anymore?
Mark and I talked about this bug yesterday.  Mark summed up our conclusion:

> markh: yeah.  So maybe for now we just decline to overwrite anything with
> the b/g service, and leave open the bigger question of exactly what the f/g
> should and should not capture

I took the absolutist position that the pages captured by the fg should be a subset of the pages captured by the bg.  In other words, if both services were to capture the same, "non-sensitive" URL, then the thumbnails should be the same (excepting differences due to technical issues, like the presence of ads due to AdBlock+ not working in the content process).

Mark pointed out that it's difficult to describe what "sensitive" means, both in technical terms and in lay terms that all users would agree on.  And if you use a big hammer and say it means everything excluded by PB mode, as I wanted to do, then you end up declining captures for pages that a reasonable user would not consider private, like the front page of phonebook.mozilla.org, which results in thumbnails that show things like "Authorization required" at best, or no thumbnails at all.
Summary: If a page already has a thumbnail, the background thumbnail service should capture a new thumbnail only after the user visits it → Background thumbnail service shouldn't overwrite existing thumbnails, or at least foreground thumbnails should be preferred
(In reply to Drew Willcoxon :adw from comment #7)
> I took the absolutist position that the pages captured by the fg should be a
> subset of the pages captured by the bg.

Sigh, I mean, the thumbnails produced by the fg should be a subset of the thumbnails produced by the bg.
Is this all we're talking about?  Replace captureIfStale with captureIfMissing?  (i.e., the original intent of Tim's bug 897880 was right.)

I suppose it would be OK for the bg to overwrite its own thumbnails, but the only reason to do that would be to refresh outdated images, and I don't think that's worth doing for the types of pages declined by the fg.  We'd need to record metadata about which service captured which thumbnail in that case.
Attachment #793725 - Flags: feedback?(mhammond)
Comment on attachment 793725 [details] [diff] [review]
captureIfStale -> captureIfMissing

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

yeah, I think that's all we need to do.

::: browser/base/content/newtab/newTab.js
@@ +8,5 @@
>  let Ci = Components.interfaces;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/PageThumbs.jsm");

do we still need PageThumbs?
Attachment #793725 - Flags: feedback?(mhammond) → feedback+
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
(In reply to Mark Hammond (:markh) from comment #10)
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  Cu.import("resource://gre/modules/PageThumbs.jsm");
> 
> do we still need PageThumbs?

Yes, Site.prototype.refreshThumbnail in sites.js calls getThumbnailURL.  (Maybe we should consolidate the two services in PageThumbs.jsm...)

I need to update the tests, but this can start gathering review comments (and I'm not expecting r+ yet).  It addresses both this bug and bug 809051, as we talked about.  Changes:

* renames PageThumbs.captureIfStale to PageThumbs.captureAndStoreIfStale and
  makes it use the fg service (and moves it down below captureAndStore)
  * gBrowserThumbnails calls it
* adds BackgroundPageThumbs.captureIfMissing
  * Site in sites.js calls it
* changes PageThumbs._store's wasError parameter to an overwrite parameter
  * bg never overwrites
  * fg overwrites if response is not an error
* Stops touching thumbnails when the response is an error.  bg shouldn't touch
  because it declines capture when the file already exists.  I don't think the
  fg needs to touch either: no reason not to attempt another capture if/when
  the user opens the page again.
* As an optimization, makes PageThumbs.captureAndStore skip drawing if the
  response is an error and a file already exists

Should staleness checking be built into PageThumbs.captureAndStore, rather than having a separate captureAndStoreIfStale?

Similarly, should missing checking be built into BackgroundPageThumbs.capture, rather than having captureIfMissing?
Attachment #793725 - Attachment is obsolete: true
Attachment #794526 - Flags: review?(mhammond)
Comment on attachment 794526 [details] [diff] [review]
patch

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

Looks good to me, although as you mentioned there was more work to be done, I didn't review as thoroughly as a "real" review, but I see no issues here.

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +79,5 @@
> +   * @param options  An optional object that configures the capture.  See
> +   *                 description for capture().
> +   */
> +  captureIfMissing: function (url, options={}) {
> +    // The file may be created or removed between the time that fileExistsForURL

if I read this correctly, we are just checking if the file exists "early" for performance reasons, but at capture time we make the "real" check and still decline to overwrite it at that time if it suddenly existed in the meantime?  If so, I think we can simplify this comment and explicitly call out that a final check is made at store time.

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +325,5 @@
> +    // resolves and the time the capture's write starts, so this isn't meant to
> +    // be foolproof.  It's only meant to avoid capture at all in the case where
> +    // the file already exists and is not soon removed.  If the file is removed
> +    // after fileExistsForURL resolves, then this will be incorrect, but that's
> +    // not likely to happen often, and no big deal when it does.

ditto here - the comment is implying "it's no big deal if a file is created in the meantime - we'll just overwrite it anyway" versus "it's no bug deal if the file is created in the meantime, as _store() will still refuse to overwrite" (IIUC obviously ;)
Attachment #794526 - Flags: review?(mhammond) → feedback+
Attached patch big patch (obsolete) — Splinter Review
This builds on top of the preloader patch in bug 910036.

Some big changes:

Added a bindOnDoneToOptions() to BPT to make it hard not to call the client's callback without passing it the captured URL.  Initially I forgot to do that for BPT.captureIfMissing in the fileExistsForURL case, and I noticed that BPT.capture also forgot in the case where thumbnailing is disabled.

Make PT.captureAndStore return a promise rather than taking a callback to be consistent with other PT methods, especially the new captureAndStoreIfStale.  It makes the tests nicer, too.

Basically rewrite browser_thumbnails_update.js.  Some rewriting was inevitable since the bg service never overwrites anymore, but I also stopped using page colors to determine whether files have been updated.  Instead it uses file modification times.

Add PageThumbsStorage.isFileRecent and have PT.captureAndStoreIfStale call it, rather than calling PageThumbsWorker directly as captureIfStale did, since going through Storage rather than dealing with Worker directly seems to be the norm.
Attachment #794526 - Attachment is obsolete: true
Attachment #797697 - Flags: review?(mhammond)
Comment on attachment 797697 [details] [diff] [review]
big patch

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

Sorry, but I don't think some of the rewrites are a net-win, and in particular, the test no longer checking the content are actually a net-loss IMO.

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +50,2 @@
>      if (!PageThumbs._prefEnabled()) {
> +      schedule(options.onDone);

isn't this going to call onDone with the passed URL instead of null?

@@ +374,5 @@
>        callOnDones();
>        return;
>      }
> +    PageThumbs._store(this.url, data.finalURL, data.imageData, false).
> +      then(callOnDones);

I really prefer the leading period - without it, a quick scan of the source implies there is a new block in which "then" is called, rather than chained calls.

@@ +427,5 @@
>  function schedule(callback) {
>    Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL);
>  }
> +
> +function bindOnDoneToOptions(options, url) {

I don't like the way this both modifies and returns |options| - reading the callers of this above, it certainly looks like a new options object is returned.  I think you should just drop the return value.

TBH, I think the bindOnDoneToOptions thing is a net-loss to readability, but it's subjective, so I wont be insisting on this.

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +288,2 @@
>     */
> +  captureAndStore: function PageThumbs_captureAndStore(aBrowser) {

we've dropped |aCallback| here but the capture() method still takes one.  Given the only users of |aCallback| for captureAndStore() is the tests, I don't see this as a win - IMO it's preferable for the callback to be used so the runtime doesn't suffer the overhead of a promise that's never actually used.

@@ +301,5 @@
> +        try {
> +          let blob = yield this.captureToBlob(aBrowser.contentWindow);
> +          let buffer = yield TaskUtils.readBlob(blob);
> +          yield this._store(originalURL, url, buffer, !wasError);
> +        } catch (_) {}

This should either Cu.reportError or reject the promise - preferably the former, as I'm not convinced rejecting the promise would report the exception anywhere either.  (The old code didn't really do the right thing either)

But as above, I don't think using a promise here is an improvement.  It almost could be argued that .capture() should be a promise, but that's not actually used by the runtime either.

@@ +327,5 @@
> +  },
> +
> +  /**
> +   * Checks if an existing thumbnail for the specified URL is either missing
> +   * or stale, and if so, captures and stores it.  Once the thumbnail is stored,

Pre-existing issue: I think the comment about the observer should be moved somewhere else - *every* completed capture will cause such a notification regardless of what method was invoked to capture it.

::: toolkit/components/thumbnails/test/browser_thumbnails_background.js
@@ +310,5 @@
> +    is(capturedURL, url, "Captured URL should be URL passed to capture");
> +    ok(file.exists(), "Thumbnail should be cached after capture: " + file.path);
> +
> +    let past = Date.now() - 1000000000;
> +    let pastFudge = past + 30000;

why 30000 here?  It seems arbitrary, and issues we had in the past are related to clock precision, so and discrepancy should be (much) less than a second.  (ie, it's pragmatic to assume < 1 second slop, but something would have gone terribly wrong to allow 30 seconds of slop)

@@ +312,5 @@
> +
> +    let past = Date.now() - 1000000000;
> +    let pastFudge = past + 30000;
> +    file.lastModifiedTime = past;
> +    ok(file.lastModifiedTime < pastFudge, "Last modified time should stick!");

shouldn't this be something like: ok(Math.abs(file.lastModifiedTime - past) < pastFudge, ...); ?

@@ +314,5 @@
> +    let pastFudge = past + 30000;
> +    file.lastModifiedTime = past;
> +    ok(file.lastModifiedTime < pastFudge, "Last modified time should stick!");
> +    capturedURL = yield captureIfMissing(url);
> +    is(capturedURL, url, "Captured URL should be URL passed to second capture");

I don't see the point in this check - the implementation is just returning the same param as what was passed in, and not anything related to the result of the capture.  (I guess it doesn't hurt, although it's a little misleading - I initially read it as "check the correct URL was captured" when really it's more like "check what I passed in is what I think I passed in")

::: toolkit/components/thumbnails/test/browser_thumbnails_update.js
@@ +33,5 @@
> +  ok(file.exists(), "Thumbnail should be cached after capture: " + file.path);
> +
> +  // Make the file really fresh and capture again.
> +  let future = Date.now() + 1000000000;
> +  let futureFudge = future - 30000;

same issue here with a magic number that seems far larger than necessary.

@@ +50,3 @@
>  }
>  
> +function staleOverwrite() {

I think the color checks were valuable - they checked that we captured what we intended to capture rather than simply that the mtime on the file changed.  How can we be confident the mtime wasn't modified by something external (eg, a fg capture) and that the code we are testing didn't fail with an unreported error in a promise?

::: toolkit/components/thumbnails/test/head.js
@@ +116,5 @@
>  function captureAndCheckColor(aRed, aGreen, aBlue, aMessage) {
>    let browser = gBrowser.selectedBrowser;
>  
>    // Capture the screenshot.
> +  PageThumbs.captureAndStore(browser).then(function () {

I think the addition of a promise that is used only here isn't an improvement - it's hard to argue the runtime overhead is worth the improvement to the tests (ie, this test hasn't become any more readable even though it causes runtime overhead.)

::: toolkit/components/thumbnails/test/thumbnails_update.sjs
@@ +3,5 @@
>  
>  // This server-side script is used for browser_thumbnails_update.  One of the
>  // main things it must do in all cases is ensure a Cache-Control: no-store
>  // header, so the foreground capture doesn't interfere with the testing.
>  

again, I think the check of the content has value.
Attachment #797697 - Flags: review?(mhammond) → review-
> > +      schedule(options.onDone);
> 
> isn't this going to call onDone with the passed URL instead of null?

That's the point.

> > +    PageThumbs._store(this.url, data.finalURL, data.imageData, false).
> > +      then(callOnDones);
> 
> I really prefer the leading period - without it, a quick scan of the source
> implies there is a new block in which "then" is called, rather than chained
> calls.

OK, but that's what the indentation's for.

> TBH, I think the bindOnDoneToOptions thing is a net-loss to readability, but
> it's subjective, so I wont be insisting on this.

The point is to ensure that onDone is always called with the passed-in URL.  It hurts readability where captureIfMissing uses it because that's an API entry point, but everywhere else readability's actually improved, because now calling onDone is a simple function call without having to worry about binding it to the right object or testing whether it's defined.

> ::: toolkit/components/thumbnails/PageThumbs.jsm
> @@ +288,2 @@
> >     */
> > +  captureAndStore: function PageThumbs_captureAndStore(aBrowser) {
> 
> we've dropped |aCallback| here but the capture() method still takes one. 
> Given the only users of |aCallback| for captureAndStore() is the tests, I
> don't see this as a win - IMO it's preferable for the callback to be used so
> the runtime doesn't suffer the overhead of a promise that's never actually
> used.

What's the overhead?  Is it anything but negligible?  It's a runtime penalty that's miniscule at biggest when on the other hand there's a clear benefit to the caller's code simplicity, when the caller is able to yield.  That's the right trade-off.

> > +    let past = Date.now() - 1000000000;
> > +    let pastFudge = past + 30000;
> 
> why 30000 here?  It seems arbitrary, and issues we had in the past are
> related to clock precision, so and discrepancy should be (much) less than a
> second.  (ie, it's pragmatic to assume < 1 second slop, but something would
> have gone terribly wrong to allow 30 seconds of slop)

It's just a bigger fudge value than one second, which I'll point out is also arbitrary.  It's just as correct but 30 times safer.

> > +    let past = Date.now() - 1000000000;
> > +    let pastFudge = past + 30000;
> > +    file.lastModifiedTime = past;
> > +    ok(file.lastModifiedTime < pastFudge, "Last modified time should stick!");
> 
> shouldn't this be something like: ok(Math.abs(file.lastModifiedTime - past)
> < pastFudge, ...); ?

pastFudge is past + 30000, not 30000.

> > +    capturedURL = yield captureIfMissing(url);
> > +    is(capturedURL, url, "Captured URL should be URL passed to second capture");
> 
> I don't see the point in this check - the implementation is just returning
> the same param as what was passed in, and not anything related to the result
> of the capture.

That's right, it's testing that the callback is passed the same URL that the caller passed to the capture method.

> I think the color checks were valuable - they checked that we captured what
> we intended to capture rather than simply that the mtime on the file
> changed.  How can we be confident the mtime wasn't modified by something
> external (eg, a fg capture)

That's a fair point.

> >    // Capture the screenshot.
> > +  PageThumbs.captureAndStore(browser).then(function () {
> 
> I think the addition of a promise that is used only here isn't an
> improvement

It's not an improvement for this caller because it can't yield.  (Yielding in a spawn() would mean writing a callback for spawn, which is no better than writing a callback for captureAndStore.)  But it is an improvement for other callers in the tests.
(In reply to Drew Willcoxon :adw from comment #15)
> > > +      schedule(options.onDone);
> > 
> > isn't this going to call onDone with the passed URL instead of null?
> 
> That's the point.

Fair enough - but that was a behaviour change I wasn't expecting (ie, I assumed it was intentional that null was initially passed in that case)

> > > +    PageThumbs._store(this.url, data.finalURL, data.imageData, false).
> > > +      then(callOnDones);
> > 
> > I really prefer the leading period - without it, a quick scan of the source
> > implies there is a new block in which "then" is called, rather than chained
> > calls.
> 
> OK, but that's what the indentation's for.

I guess my point here is that indentation typically means "new block" on a casual scan.  A leading period means (to my eye) that it's not a block.

> > TBH, I think the bindOnDoneToOptions thing is a net-loss to readability, but
> > it's subjective, so I wont be insisting on this.
> 
> The point is to ensure that onDone is always called with the passed-in URL. 

Fair enough - I thought it was more about ensuring the correct |this|.  Binding the URL actually seems to hurt readability IMO - it's not obvious when the callback is made that a param is actually passed.

> It hurts readability where captureIfMissing uses it because that's an API
> entry point, but everywhere else readability's actually improved, because
> now calling onDone is a simple function call without having to worry about
> binding it to the right object or testing whether it's defined.

Fair enough.

> > ::: toolkit/components/thumbnails/PageThumbs.jsm
> > @@ +288,2 @@
> > >     */
> > > +  captureAndStore: function PageThumbs_captureAndStore(aBrowser) {
> > 
> > we've dropped |aCallback| here but the capture() method still takes one. 
> > Given the only users of |aCallback| for captureAndStore() is the tests, I
> > don't see this as a win - IMO it's preferable for the callback to be used so
> > the runtime doesn't suffer the overhead of a promise that's never actually
> > used.
> 
> What's the overhead?  Is it anything but negligible?  It's a runtime penalty
> that's miniscule at biggest when on the other hand there's a clear benefit
> to the caller's code simplicity, when the caller is able to yield.  That's
> the right trade-off.

There's some minor runtime overhead (eg, to the garbage collector), potential error handling overhead (I still don't trust promises to always report errors) and cognitive overhead to the reader of the code.  I'm afraid I still don't see how this is justified simply so a utility function in head.js becomes simpler.  Eg, I'd be much happier if the promise was exposed by that utility function (meaning no runtime overhead and the test functions that call it still get the same benefits)

> > > +    let past = Date.now() - 1000000000;
> > > +    let pastFudge = past + 30000;
> > 
> > why 30000 here?  It seems arbitrary, and issues we had in the past are
> > related to clock precision, so and discrepancy should be (much) less than a
> > second.  (ie, it's pragmatic to assume < 1 second slop, but something would
> > have gone terribly wrong to allow 30 seconds of slop)
> 
> It's just a bigger fudge value than one second, which I'll point out is also
> arbitrary.  It's just as correct but 30 times safer.

The correct value would, IMO, be the precision of the file-system timestamps on the worst platform (windows).  Using 1 second with a comment about this precision seems clearer to me than it would be with a comment about sub-second precision but 30 seconds worth of slop.

> That's right, it's testing that the callback is passed the same URL that the
> caller passed to the capture method.

Fair enough - but a casual read implied to me that the intention was to check that the correct URL was captured.

> It's not an improvement for this caller because it can't yield.  (Yielding
> in a spawn() would mean writing a callback for spawn, which is no better
> than writing a callback for captureAndStore.)  But it is an improvement for
> other callers in the tests.

As mentioned above, I've no objection to the tests using promises here - I'm just struggling to justify the runtime using them purely to help this one test function when a solution that offers the same benefits is trivial.

I'm sorry if you feel the r- was unjustified, and apologize for what, in retrospect, must have sounded like nothing but negativity.  If we can get the color checks back in, I'll let you use your best judgement on what else in my comments should be addressed when I get to review it again.
(In reply to Mark Hammond (:markh) from comment #16)
> There's some minor runtime overhead (eg, to the garbage collector),
> potential error handling overhead (I still don't trust promises to always
> report errors) and cognitive overhead to the reader of the code.

What's "minor," though?  It's got to be so minor as to not worth worrying about.  Should we write everything in asm.js, too?

To me it's less cognitive overhead, but I guess like you say it's subjective.

> I'm afraid I still don't see how this is justified simply so a utility
> function in head.js becomes simpler.

But it's not just that one caller (which I'm not claiming becomes simpler, it doesn't).  We're talking about PageThumbs.captureAndStore, right?  In the patch, it's called three other times in tests and once in PageThumbs.jsm, all using yield, and those callers are easier to read IMO because they're using yield and promises.

> I'm sorry if you feel the r- was unjustified, and apologize for what, in
> retrospect, must have sounded like nothing but negativity.

No no, no hard feelings! :-)  I just don't understand some of your reasoning.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #797697 - Attachment is obsolete: true
Attachment #801996 - Flags: review?(mhammond)
Comment on attachment 801996 [details] [diff] [review]
patch v3

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

I like this much better - thanks!
Attachment #801996 - Flags: review?(mhammond) → review+
Attached patch patch v4Splinter Review
I'll post the small interdiff next.  A couple of problems with the r+'ed patch:

The additional aOverwrite parameter to PageThumbsStorage.copy caused the browser_thumbnails_storage.js test to fail because it reversed what happens when the target file already exists.  This new patch changes the parameter to be aNoOverwrite so that existing copy and (hypothetical) writeData callers don't need to be updated: they can continue to not pass in a value for that parameter, so aNoOverwrite is falsey in those cases, preserving current behavior.

OS.File throws an error when noOverwrite is true and the target file exists.  Seems weird, but OK.  Eat that pseudo-error.  Otherwise it propagates up to the console via TaskUtils.captureErrors in PageThumbs.jsm.  This is the main reason I'm asking for review again.
Attachment #801996 - Attachment is obsolete: true
Attachment #803463 - Flags: review?(mhammond)
Comment on attachment 803464 [details] [diff] [review]
patches v3/v4 interdiff

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

sounds (and looks) good.
Attachment #803464 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/55b3a19125d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 949655
You need to log in before you can comment on or make changes to this bug.