395277 - web-based handler updating needs l10n strategy

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

In bug 392978, a scheme was introduced for injecting updated default handlers into mimeTypes.rdf.  As per discussion in that bug, we need an l10n strategy for this.  Probably the prefs want to live in region.properties.  Ideally this would behave reasonably for users who use the same profile with builds from different locales.

Comment 1

[Axel Hecht [:Pike]]

Moving this over to my other account. I'm not sure if this should be really assigned to me, but let's leave it in my bucket until I have a constructive comment. We should reassign this bug for the implementation part then.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Sounds fine; feel free to reassign to me for impl.

Comment 3

[Axel Hecht [:Pike]]

So, let me try to find out the technical requirements here, looking at the patch in bug 392978, we need prefs like 

+pref("gecko.handlerService.schemes.webcal.0.name", "WebCal Test Handler");
+pref("gecko.handlerService.schemes.webcal.0.uriTemplate", "http://handler-test.mozilla.org/webcal?url=%s");

where webcal would be a url scheme?

Do we bother that schemes have '.' as allowed character (http://tools.ietf.org/html/rfc3986#section-3.1) ?

Is http://developer.mozilla.org/en/docs/DOM:window.navigator.registerProtocolHandler the only documentation we have so far?

I wonder, did we improve the content type restrictions on registerContentHandler?

And, of course, do we have any idea if we're having something beyond handler-test.mozilla.org for Fx3?

All of these questions (sorry for asking so late) just to answer:

- How do we reach out to which webservices?

Another set of questions are around "what do we expect to happen for sidegrades", like locale switching. Mic might be able to chat with product folks on this, Mic?

Technically, when is the data in the prefs accessed? How do we track if a user actually picked a webhandler? All of this just to figure out if we can change something in our settings when a user installs a language pack with different offerings, and, how to persist an actual dedicated choice, if we intend to do that.

Comment 4

[Mike Schroepfer]

Why is this a b1 blocker?

Comment 5

[(not currently active) Ted Mielczarek]

We considered this a blocker, but we didn't pay attention to the milestone.  Pike, do you want to push this out to M10?

Comment 6

[Axel Hecht [:Pike]]

Pushing this out to M10.

I still need some more information on what's currently implemented, dmose?

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #3)
> So, let me try to find out the technical requirements here, looking at the
> patch in bug 392978, we need prefs like 
> 
> +pref("gecko.handlerService.schemes.webcal.0.name", "WebCal Test Handler");
> +pref("gecko.handlerService.schemes.webcal.0.uriTemplate",
> "http://handler-test.mozilla.org/webcal?url=%s");
> 
> where webcal would be a url scheme?

Yes.

> Do we bother that schemes have '.' as allowed character
> (http://tools.ietf.org/html/rfc3986#section-3.1) ?

Ugh.  Good catch.  I've never seen a scheme with a . in it in the wild, and since we're only ever likely to default pretty widely used protocols, I suspect this isn't really worth fixing until we actually find a case where it's necessary.

> Is
> http://developer.mozilla.org/en/docs/DOM:window.navigator.registerProtocolHandler
> the only documentation we have so far?

Yes.  That page itself is pretty straightforward, and the WhatWG spec that it links to has more detail for those interested.  I think it would be useful if we could get someone (evangelism?) to put together a tutorial on building a simple server-side handler.  Is there other stuff you think could stand more documentation?

> I wonder, did we improve the content type restrictions on
> registerContentHandler?

I don't think anything has changed there, meaning that we drop everything except the feed type on the floor.

> And, of course, do we have any idea if we're having something beyond
> handler-test.mozilla.org for Fx3?

That's just a test for the next milestone or two; it'll disappear before Fx3 ships. The idea is that we will have some set of default handlers populated for Fx3.  One suggested list has included mailto:, webcal:, feed:, and im: or equivalent.

> All of these questions (sorry for asking so late) just to answer:
> 
> - How do we reach out to which webservices?
 
This discussion still needs to be had.  I'll file a bug shortly once I hear back from beltzner how he wants to track PM issues like this.

My suspicion is that there will be some set of core schemes that we care the most about and will want to try and find appropriate locale-compatible services for. 

> Another set of questions are around "what do we expect to happen for
> sidegrades", like locale switching. Mic might be able to chat with product
> folks on this, Mic?

Yeah, that's what I was driving at in comment 0 of this bug, actually.

> Technically, when is the data in the prefs accessed?

When profile-do-change is fired, if a new set of handlers has showed up in prefs since the last time the upgrade ran, it'll inject that set of data into mimeTypes.rdf.  See <https://bugzilla.mozilla.org/attachment.cgi?id=279983&action=diff>.

> How do we track if a user actually picked a webhandler?

Default handlers will show up in the protocol handling dialog.  If a user selects one, it gets remembered as the default.  Moreover, the user can click "always use this choice" so that the dialog doesn't pop up again.  It can be changed from the Applications pane of the preferences.

> All of this just to figure out if we can change
> something in our settings when a user installs a language pack with different
> offerings

The code as currently written has a single version number for the defaults, which means that a sidegrade is likely to confuse it.  We could potentially change that to be a (locale, version) tuple, at the cost of some complexity (though probably not a ton), and then inject the new locale's handlers also.

> and, how to persist an actual dedicated choice, if we intend to do
> that.

If I'm understanding you correctly, we already do that.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

dveditz pointed out in the design/security review that we could avoid the problem with . in URI schemes by implementing our own escaping on top of the prefs APIs.  I suspect it's not worth the effort.

Comment 9

[Daniel Veditz [:dveditz]]

If you don't fix it at the pref end then you need to reject schemes with '.' in the registration request. You're already doing error-checking there, right? Easy to add then.

Comment 10

[Daniel Veditz [:dveditz]]

A major drawback of the "localized pref" strategy is that you'll have a fixed number of prefs. All locales will have to have the same number, or you'll have to have code that expects some to be blank or missing.

Comment 11

[Mike Connor [:mconnor]]

We've done (In reply to comment #10)
> A major drawback of the "localized pref" strategy is that you'll have a fixed
> number of prefs. All locales will have to have the same number, or you'll have
> to have code that expects some to be blank or missing.

We've done it before (for feed handlers), so its not a huge deal.  We really need this fixed for b3 if at all possible.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

dveditz: re comment 10: we just iterate through all the numbered children of a given handler type, so there's no need to know the actual numbers in advance.

I think I've come up with a solution, which is simply to inject new protocol handler defaults whenever the version of protocol handler defaults changes or the locale changes.  

Since this sounds like a pretty reasonable general solution to locale-related sidegrade issues, I propose add a profile-change-teardown listener somewhere in toolkit that stuffs the result of nsILocaleService.getApplicationLocale into a preference called something like app.lastRunApplicationLocale.  pike, bsmedberg: what do you think?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Lookiong more closely, I see that getApplicationLocale returns something that doesn't have any obvious way to get a string representation.  Hmph.  I'll have to look at this more tomorrow...

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: injection l10n, v1

This is a work-in-progress patch; it doesn't yet work quite right.  I think there's only one more bug to shake out of it, but I wanted to get it attached so that folks who were wondering about the strategy could have a quick look and sanity check.

Comment 15

[Axel Hecht [:Pike]]

gecko.handlerService.defaultHandlersVersion=1

doesn't make sense to me. Is it the version of the pref format, or the number of prefs? If it was, we'd have to both change that number *and* add more locale redirect prefs for the localization. That seems like doing one thing twice.

We'll need to point to docs for this in the localization comment, too.

Mic, should we point to http://wiki.mozilla.org/Firefox_web_services_guidelines?

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Heh, neither.  It is simply the version of the set of handlers in that file.  So everytime a localizer adds / removes / changes anything in that file, they should bump the version number.  This will cause the code in nsHandlerService to rescan those preferences and inject any new handlers into mimeTypes.rdf.  It's merely an optimization to avoid a possible Ts hit.

I'll give it a more explicit comment in the next iteration.

Does that help?

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: injection l10n, v2

OK, I talked to Axel on IRC, and he thought this would be ok with clearer comments.  Here's a working version of the patch with that addressed.  Requesting review for the browser and toolkit bits from mconnor (flagged as ui-review because that's what's available here).  For the exthandler bits, requesting review from myk, and sr from biesi.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 299511 [details] [diff] [review]
injection l10n, v2

The exthandler part of the patch (nsHandlerService.js, nsExternalHelperAppService.cpp) looks fine, although I haven't tested it.  Do you have some sample localized data and a recommended strategy for switching locales?


>Index: uriloader/exthandler/nsHandlerService.js

>   fillHandlerInfo: function HS_fillHandlerInfo(aHandlerInfo, aOverrideType) {
>+
>     var type = aOverrideType || aHandlerInfo.type;
>     var typeID = this._getTypeID(this._getClass(aHandlerInfo), type);
...
>     while (possibleHandlerTargets.hasMoreElements()) {
>+
>       let possibleHandlerTarget = possibleHandlerTargets.getNext();
>       if (!(possibleHandlerTarget instanceof Ci.nsIRDFResource))
>         continue;
> 
>+
>       let possibleHandlerID = possibleHandlerTarget.ValueUTF8;
>       let possibleHandler = this._retrieveHandlerApp(possibleHandlerID);
>+
>       if (possibleHandler && (!aPreferredHandler ||

Nit: there seem to be a number of extra newlines added here, and it's not clear that they're all intentional.


>Index: uriloader/exthandler/nsIExternalProtocolService.idl

>    * @param aProtocolScheme the scheme from a URL: http, ftp, mailto, etc.
>+   * @param aFillObject     should nsIHandlerService.fillHandlerInfo be
>+   *                        called on this object before returning?  Defaults
>+   *                        to true.

Nit: it would be useful to explain here what calling fillHandlerInfo means (i.e. populating the object with user- or application-specified configuration rather than just OS-provided info).

Comment 19

[Axel Hecht [:Pike]]

I have a testing question:

We'll want to test the values of this via QA (automated or not), what do we need to do? Any files that need to be blown away in a profile between runs so that one can reproduce the pristine values for a localization?

I guess that should be added to http://wiki.mozilla.org/QA/Firefox3/TestPlan/Content_Handling_-_Web_Protocol_and_Application?

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Ick, I found some bustage.  New patch forthcoming.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fr.xpi (Mac version) for testing

This is a copy of a nightly XPI for the fr locale which I use for testing.  It has an added "Beer test handler" for the webcal: scheme.  Directions for testing after the patch gets here.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: injection l10n, v3 (WIP)

Work-in-progress patch

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: injection l10n, v4 (WIP)

This works on Mac and is mostly cleaned up.  Just need to do a bit of testing, and get the Windows / Linux bits suitably tweaked, and then it'll be ready for review.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: injection l10n, v5

Complete patch; testing is ongoing.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: injection l10n, v5 (cvs diff -uw, for review)

Requesting review, as before, from myk (r) and biesi (sr) for the exthandler parts, and mconnor (r) for the browser and toolkit parts.

This would be a good thing to unit test automatically, but making that testing happen looks like a decent amount of work, and I think it's more important that this code see real world testing in B3 than it is to block on that work.  I propose spinning off another bug for that.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 300036 [details] [diff] [review]
injection l10n, v5 (cvs diff -uw, for review)

There's a problem with this way of testing for locale changes: if the handler service doesn't get used in the first invocation following the change, _injectNewDefaults will never get called.  Given how locale changes may by done.  I have a theory on fixing this by making the defaultHandlersVersion in the RDF datastore be per-locale.  New patch forthcoming.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: l10n injection, v6

New patch which keeps track of the last injected default handler version per locale, rather than just once globally.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: l10n injection, v6 (cvs diff -w for review)

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Instructions for those who wish to test this:

1. download the platform-appropriate fr.xpi from this bug (right now, the only one in the bug is for mac; i'll fix that shortly).  after this and other locales land defaults in the tree, it'll be possibly to use regularly nightly XPIs for testing this instead of hand-spun ones like these.

2. create a new profile with the en-US build of firefox

3. type webcal:foo into the URL bar.  verify that one of the handlers offered is "Webcal Test Handler".  That will have been injected from the en-US defaults.

3. use File/Open to install fr.xpi

4. before you click on the "Restart" button in the install dialog, go to about:config and edit the general.useragent.locale pref to say "fr" (without the quotes)

5. click the "restart" button

6. after the restart, type webcal:foo into the URL bar.  In addition to the handlers offered previously (including "Webcal Test Handler"), there should also be a "Beer test handler" offered.  That will have been injected from the fr
defaults.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fr.xpi for testing (Linux version)

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 300221 [details] [diff] [review]
l10n injection, v6 (cvs diff -w for review)

>Index: uriloader/exthandler/nsHandlerService.js

>+      // if we don't have the current version of the default prefs for
>+      // this locale, inject any new default handers into the datastore
>+      if (defaultHandlersVersion < this._prefsDefaultHandlersVersion ) {

Nit: extraneous space after "this._prefsDefaultHandlersVersion".......^


>-    var version = this._getValue("urn:root", NC_DEFAULT_HANDLERS_VERSION); 
>+    var version = this._getValue("urn:root", NC_NS + this._currentLocale + "_" + 
>+                                             DEFAULT_HANDLERS_VERSION);

Nit: A colon might be preferable to an underspace as a separator here, given usage elsewhere.


>Index: uriloader/exthandler/nsIExternalProtocolService.idl

>+  /** 
>+   * Set some sane defaults for a protocol handler object.
>+   * 
>+   * @param aHandlerInfo      nsIHandlerInfo object, as returned by 
>+   *                          getProtocolHandlerFromOS
>+   * @param aOSHandlerExists  was the object above created for an extant
>+   *                          OS default handler?  This is generally the
>+   *                          value of the aFound out param from
>+   *                          getProtocolHandlerFromOS.
>+   */
>+  void setProtocolHandlerDefaults(in nsIHandlerInfo aHandlerInfo,
>+                                  in boolean aOSHandlerExists);

Nit: getProtocolHandlerFromOS -> getProtocolHandlerInfoFromOS

The uriloader/ changes otherwise look great, r=myk

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

I didn't change the :, because that seems to mostly be used in URNs, not namespaced arc-name URIs.  I've addressed the other nits.  I've also replaced the "WebCal Test Handler" with a real live "30 Boxes" handler.  Updated patch after mconnor reviews.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

We want this for beta3 so that localizers can ship locales with web-based handlers early enough to get a decent amount of testing (i.e. beta 3).

Comment 34

[Axel Hecht [:Pike]]

I don't see this happen for B3. B3 is sacked, the timestamp for pulling is probably going to be 8am PST today, i.e. almost 4 hours ago.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Agreed - we missed B3 on this one, let's focus on getting it into nightlies when the tree re-opens.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 300221 [details] [diff] [review]
l10n injection, v6 (cvs diff -w for review)

Transferring request to gavin, as mconnor has been overloaded.  Note that this is _not_ actually a UI-R request; it's a request for review of the tiny set of changes to browser/ that are part of this patch -- I ran out of bugzilla flags to request regular review.

Comment 37

[Reed Loden [:reed]]

Comment on attachment 300221 [details] [diff] [review]
l10n injection, v6 (cvs diff -w for review)

... but you didn't.

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: updated fr.xpi (Mac version) for testing

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: l10n injection, v7 (as landed)

Fixed a merge conflict; no longer includes the web-based test handler, but includes the real-live 30 boxes handler.

Comment 40

[jag (Peter Annema)]

This seems to have caused orange on the SeaMonkey-Ports SunOS 5.11 boxen.

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: sunos bustage & type-error fix (landed)

I've landed the attached patch, which should fix the SunOS lossage.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

So, is this now FIXED?

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

Whoops; yes.  Resolving.

Comment 44

[Radek 'sysKin' Czyz]

Was this bug supposed to add a reference to "30boxes.com"?

I found such reference automatically added when mimetypes.rdf is deleted and out of curiosity I traced it to this bug, but patches attached here don't match the cvs. They use a much more innocent-looking handler-test.mozilla.org host instead.

Good thing I'm not into conspiracy theories lol :)

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

No conspiracy here; see comment 32.  Most of the discussion happened elsewhere, however, in blogs and in bug 413630.  The test-handler went away because it was just a place holder until we had some real live defaults.