Mike Kaply [:mkaply] Reporter
	Description • 11 years ago

Because there is no good way to see if a page has a thumbnail (see bug 818225), we used the API:

PageThumbsStorage.getFileForURL

in our extension.

As part of the rewrite of storage in bug 753768, this API was removed.

This change was unnecessary.

getFileForURL could have been left behind and the new getFilePathForURL added.

I realize this an "internal" API, but it is a JSM file that any extension could have included.

Considering this API has survived a couple rewrites of the page thumbs code, it should have stayed.

	Ben Bucksch (:BenB)
	Comment 1 • 11 years ago

As Mike said, this function offered functionality that's necessary for some extensions, and that cannot be found in a better way.

Please do make an effort to keep APIs stable. RapidReleases are no excuse for lack of stable APIs. Extensions are one of the main reasons why people like Firefox, and unnecessary breakage like this is the very reason why extension authors abandon their extensions.

Esp. in this case, it would have been very easy to avoid this breakage. It's literally 3 lines of JS:

  getFileForURL : function(pageURL) {
   return new FileUtils.File(PageThumbsStorage.getFilePathForURL(pageURL));
  }

	Tim Taubert [:ttaubert] (inactive)
	Comment 2 • 11 years ago

Which extensions did we break exactly?

	Mike Kaply [:mkaply] Reporter
	Comment 3 • 11 years ago

web.de mailcheck.
gmx mailcheck.
mail.com mailcheck.

Their non AMO distributions have a few million users.

But there are also AMO versions.

Was AMO search to see if there were other consumers of PageThumbs?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 4 • 11 years ago

(In reply to Michael Kaply (mkaply) from comment #0)
> Because there is no good way to see if a page has a thumbnail (see bug
> 818225), we used the API:
> 
> PageThumbsStorage.getFileForURL
> 
> in our extension.

Did you consider filing a bug to add a "good way"?

	Mike Kaply [:mkaply] Reporter
	Comment 5 • 11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> (In reply to Michael Kaply (mkaply) from comment #0) 
> Did you consider filing a bug to add a "good way"?

Yes as I said, bug 818225. It was filed Dec 4 last year.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 6 • 11 years ago

Cool! Let's fix that one instead of this one.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 7 • 11 years ago

New Tab Tools and UnitedTB seem to depend on PageThumbs. I admit that I never thought about searching AMO as this was never meant to be a public API.

I guess I can reimplement this with a deprecation warning.

	Ben Bucksch (:BenB)
	Comment 8 • 11 years ago

> Cool! Let's fix that one instead of this one.

getFileForURL() is a good API, no? It just needs to be declared public.
How else would you do the API?

	Ben Bucksch (:BenB)
	Comment 9 • 11 years ago

(FWIW, some callers and usescases need access to the actual image, e.g. to make their own thumbnail arrangements, not just a boolean about its existence.)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 10 • 11 years ago

Well, it is not a good API as it returns a |nsIFile|. We are attempting to get rid of this component, as:
- it is generally slow;
- it is designed for main thread blocking I/O, which is one of the most important performance bottlenecks we need to fight.

Now, we could reimplement this with a deprecation warning and eventually get rid of that API. But it is not here to stay.

	Ben Bucksch (:BenB)
	Comment 12 • 11 years ago

It doesn't need to be nsIFile. What's needed is access to the picture, so that I can either use it in a webpage or process its data.

I am not sure what the API du jour is, but the reason for introducing and using nsIFile everywhere instead of filepaths as strings was that filepaths were not unique, esp. on Mac OS with 2 volumes of the same name.

If that's not a concern and getFilePathForURL() is a good idea, then I have no problem with that, as long as it's public and stable.

The main point of this bug was that certain APIs are necessary for exts (no matter whether they are officially public or not), and they should be kept stable. Maybe you can also take a look at your all your APIs, and see which functions may be useful for exts, and declare those public. As seen here, keeping them stable is often not that hard.

	Ben Bucksch (:BenB)
	Comment 13 • 11 years ago

Thanks for the efforts and help, BTW!

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 14 • 11 years ago

Well, the "API du jour" is called OS.File: https://developer.mozilla.org/en-US/docs/JavaScript_OS.File - it provides faster, off main thread I/O for javascript.

Sorry for not realizing that some add-ons were using this API. I'll work on bug 818225 while waiting for my compilation to complete :)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 15 • 11 years ago

Same one, minus typo.

	Tim Taubert [:ttaubert] (inactive)
	Comment 16 • 11 years ago

I think I'd rather go with Gavin's suggestion and implement the "good way" in bug 818225. PageThumbsStorage is a private API that was never meant to be for add-ons and we shouldn't encourage add-on authors to continue using it.

We should rather file a follow-up to put everything storage related into toolkit/components/thumbnails/_Storage.jsm or the like so that people rely on our abstraction and not the implementation itself.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 17 • 11 years ago

I believe that we should restore getFileForURL for the time being and remove it a few weeks after we have fixed bug 818225.

	Tim Taubert [:ttaubert] (inactive)
	Comment 18 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> I believe that we should restore getFileForURL for the time being and remove
> it a few weeks after we have fixed bug 818225.

Let's do that.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 19 • 11 years ago

Try: https://tbpl.mozilla.org/?tree=Try&rev=730f19370316

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 20 • 11 years ago

BenB, by the way, I don't think your addon is on AMO. You should uploading it. Among other stuff, this will let us find out more easily whether we break it.

	Mike Kaply [:mkaply] Reporter
	Comment 21 • 11 years ago

It is:

https://addons.mozilla.org/en-US/firefox/addon/webde-mailcheck/?src=search
https://addons.mozilla.org/en-US/firefox/addon/mailcom-mailcheck/?src=search

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 22 • 11 years ago

My bad.

	Tim Taubert [:ttaubert] (inactive)
	Comment 23 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/94dfaf6285b4

	Tim Taubert [:ttaubert] (inactive)
	Comment 24 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/94dfaf6285b4

	Tim Taubert [:ttaubert] (inactive)
	Comment 25 • 11 years ago

Comment on attachment 732843 [details] [diff] [review]
Restoring getFileForURL, with a deprecation warning, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 753768
User impact if declined: we removed a deprecated API but decided we would like to give add-on authors more time to fix their add-ons
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

	Tim Taubert [:ttaubert] (inactive)
	Comment 26 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/934f4138d07a

	Ada [:adalucinet]
	Comment 27 • 11 years ago

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0

Verified as fixed the mail.com extension (https://addons.mozilla.org/en-US/firefox/addon/mailcom-mailcheck/?src=search) on Firefox 23 RC 1 (Build ID: 20130730113002). Couldn't verify the web.de extension because my ip address is blocked (even after changing proxies) and I couldn't create an e-mail address; Could anyone with an account please verify web.de addon?

Please let me know if there are any other addons/scenarios I should cover.

	Mike Kaply [:mkaply] Reporter
	Comment 28 • 11 years ago

I can verify this is wokring on web.de

	u279076
	Comment 29 • 11 years ago

Thanks for your help Alexandra and Mike.