350123 - Update URLFormatter to work with non-Fx apps

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

- remove nsIDictionary dependency
- update class ID to toolkit, from browser
- make or validate the necessary changes to tbird, sunbird, minimo
- update extensions.getMore* URLs to point to the new URL schema

yes, i know the product+component are wrong for this bug, but haven't yet found a more appropriate option.

Comment 1

[Worcester12345]

Should Bugzilla Bug 349574 no "get more themes" link, themes needs work also depend on this?

Comment 2

[Robert Strong (they/them - no direct email)]

Any estimate on this?

Comment 3

[Dietrich Ayala (:dietrich)]

Hey Rob, this is #1.5 on my list right now, aiming to fix in the next day or so.

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix

This fixes the component issues, and updates callers in browser and toolkit. LXR shows no other callers at this point. The dependent bugs contain the patches necessary to fix the mail and calendar builds. I've notified Doug regarding Minimo's usage of toolkit's about.xhtml.

Comment 5

[Dietrich Ayala (:dietrich)]

Mr. Strong, let me know if you aren't the right person to review this, or if you don't have time.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 235886 [details] [diff] [review]
fix

I'll have a go at understanding nsIURLFormatter... for now a couple of quick nits

>Index: browser/base/content/browser.js
...
> function formatURL(aFormat, aVars, aIsPref) {
> 
nit: prevalent style for this file is to not have the additional line after a function start

>-  var repl = null;
>+  var formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);
>+
>   if (aVars) {
>-    repl = Cc["@mozilla.org/dictionary;1"].createInstance(Ci.nsIDictionary);
>     for (var key in aVars) {
>-      var val = Cc['@mozilla.org/supports-string;1'].createInstance(Ci.nsISupportsString);
>-      val.data = aVars[key];
>-      repl.setValue(key, val);
>+      formatter.replace(key, aVars[key]);
>     }
nit: prevalent style is to not use brackets unless necessary

>Index: toolkit/components/urlformatter/public/nsIURLFormatter.idl
...
>+  /**
>+   * replace - Add a variable for the next replacment
>+   * 
>+   * @param aVariable string
>+   * @param aValue string
>+   */
>+   void replace(in AString aVariable, in AString aValue);
>+
>+   /**
>+    * clear - Clear any replacement values that have been added
>+    */
>+   void clear();
nit: there is an extra space in your indenting starting from 
>+   void replace(in AString aVariable, in AString aValue);

>Index: toolkit/components/urlformatter/src/nsURLFormatter.js
...
>+  formatURL: function uf_formatURL(aFormat) {
>     var _this = this;
>     var replacer = function(aMatch, aKey) {
>-      if (repl[aKey]) // handle vars from caller
>-        return repl[aKey];
>+      if (_this._values[aKey]) // handle vars from caller
>+        return _this._values[aKey];
>       if (_this._defaults[aKey]) // supported defaults
>         return _this._defaults[aKey]();
>       Components.utils.reportError("formatURL: Couldn't find value for key: " + aKey);
>       return '';
>     }
>-    return aFormat.replace(/%([A-Z]+)%/gi, replacer);
>+    var url = aFormat.replace(/%([A-Z]+)%/gi, replacer);
>+    this.clear(); // clear local replacement cache
>+    return url;
This seems overly complicated... could you document this so when someone else reads it they can easily grasp the reason for this approach? Also, please document what happens when a value for a key is not found and other error conditions here and in the idl.

I'll finish up the review tomorrow.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 235886 [details] [diff] [review]
fix

Addendum to comment #6
nsIURLFormatter.replace has a description of    * replace - Add a variable for the next replacment

Naming this replace is very misleading.

I'd really like it if formatURL didn't use var replacer and pass it to string.replace... something like this should be simple and having nsIURLFormatter.replace and string.replace in this component is just confusing.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: fix - review comments addressed

This addresses the nits and renames the replace functions.

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: fix - review comments addressed

updated patch w/ fixed indentation.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 236773 [details] [diff] [review]
fix - review comments addressed

toolkit/components/urlformatter/public/nsIURLFormatter.idl	
    * @param aVars nsIDictionary - NULL is acceptable if not using local replacements.
    */
-  string formatURL(in AString aFormat, in nsIDictionary aVars);
+  string formatURL(in AString aFormat);

aVars is gone now

also for formatURLPref

and shouldn't the return value be a wide string, so that it works with non-ASCII characters as well? especially considering that all input parameters are wide strings here.

since this object now has state (the substitutions), should it really be a service?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 236773 [details] [diff] [review]
fix - review comments addressed

toolkit/components/urlformatter/src/nsURLFormatter.js
+      if (_this._values[aKey]) // handle vars from caller
+        return _this._values[aKey];
       if (_this._defaults[aKey]) // supported defaults
         return _this._defaults[aKey]();

shouldn't the checks be:
  if (aKey in this._value)
?

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 236773 [details] [diff] [review]
fix - review comments addressed

Canceling review, to address Christian's comments.

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 236773 [details] [diff] [review]
fix - review comments addressed

eep, sorry for bugspam

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: wip patch - fixes Christian's nits

This patch fixes Christian's nits. 

(In reply to comment #10)
> since this object now has state (the substitutions), should it really be a
> service?
> 

I'd like to see it remain a service, so we don't have to build up and tear down the reference to the appInfo service and it's data each time a URL needs to be formatted.

The "local substitution vars" feature isn't used anywhere, so I think I'm going to remove it for now, and file a follow-up bug for adding it as an enhancement. This will allow the formatter to remain stateless and a service.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

>I'd like to see it remain a service, so we don't have to build up and tear down
>the reference to the appInfo service and it's data each time a URL needs to be
>formatted.

well is that code performance critical?

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> >I'd like to see it remain a service, so we don't have to build up and tear down
> >the reference to the appInfo service and it's data each time a URL needs to be
> >formatted.
> 
> well is that code performance critical?
> 

This code is used in the main browser UI, as well as in the extension manager. The performance hit would likely be minimal, but I don't want to add a few milliseconds to UI load time if it's not necessary.

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: simplified patch

This patch fixes Robert and Christian's comments, and removes the ability to pass in "local" vars for substitution. This feature is not required for our current use of the formatter, and can be added as an enhancement later if necessary.

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 236979 [details] [diff] [review]
simplified patch

Looks good at a glance. I won't have time to review this until tomorrow (7th) evening in case you'd like it sooner.

Comment 19

[Robert Strong (they/them - no direct email)]

Comment on attachment 236979 [details] [diff] [review]
simplified patch

>Index: toolkit/components/urlformatter/public/nsIURLFormatter.idl
...
>-  string formatURLPref(in AString aPref, in nsIDictionary aVars);
>+  AString formatURLPref(in AString aPref);

>Index: toolkit/components/urlformatter/src/nsURLFormatter.js
...
>   formatURLPref: function uf_formatURLPref(aPref, aVars) {
Singing the tune ""which one of us is different?"... formatURLPref only takes one param/

r=me with that fixed

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: as checked in on trunk

As checked in on trunk. Fixed review comment, and updated 2 callers that were not in the previous patch.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.705; previous revision: 1.704
done
Checking in browser/base/content/utilityOverlay.js;
/cvsroot/mozilla/browser/base/content/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.42; previous revision: 1.41
done
Checking in browser/components/nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.26; previous revision: 1.25
done
Checking in browser/components/safebrowsing/content/phishing-afterload-displayer.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-afterload-displayer.js,v  <--  phishing-afterload-displayer.js
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/components/urlformatter/public/nsIURLFormatter.idl;
/cvsroot/mozilla/toolkit/components/urlformatter/public/nsIURLFormatter.idl,v  <--  nsIURLFormatter.idl
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/urlformatter/src/nsURLFormatter.js;
/cvsroot/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js,v  <--  nsURLFormatter.js
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/content/about.xhtml;
/cvsroot/mozilla/toolkit/content/about.xhtml,v  <--  about.xhtml
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.33; previous revision: 1.32
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.111; previous revision: 1.110
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.40; previous revision: 1.39
done
Checking in toolkit/mozapps/extensions/content/list.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/list.js,v  <--  list.js
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/mozapps/update/content/updates.js;
/cvsroot/mozilla/toolkit/mozapps/update/content/updates.js,v  <--  updates.js
new revision: 1.64; previous revision: 1.63
done
Checking in toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.118; previous revision: 1.117
done

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 237300 [details] [diff] [review]
as checked in on trunk

Drivers: This patch allows the new product URL formatting code to play nice with non-Firefox apps, by reducing build dependencies. The changes themselves are low risk, though the patch touches a fairly large number of files.

Comment 22

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

Comment on attachment 237300 [details] [diff] [review]
as checked in on trunk

a=beltzner on behalf of 181drivers

Comment 23

[Dietrich Ayala (:dietrich)]

checked in on branch

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.202; previous revision: 1.479.2.201
done
Checking in browser/base/content/utilityOverlay.js;
/cvsroot/mozilla/browser/base/content/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.32.2.12; previous revision: 1.32.2.11
done
Checking in browser/components/nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.12.2.12; previous revision: 1.12.2.11
done
Checking in browser/components/safebrowsing/content/phishing-afterload-displayer.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-afterload-displayer.js,v  <--  phishing-afterload-displayer.js
new revision: 1.1.2.13; previous revision: 1.1.2.12
done
Checking in toolkit/components/urlformatter/public/nsIURLFormatter.idl;
/cvsroot/mozilla/toolkit/components/urlformatter/public/nsIURLFormatter.idl,v  <--  nsIURLFormatter.idl
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in toolkit/components/urlformatter/src/nsURLFormatter.js;
/cvsroot/mozilla/toolkit/components/urlformatter/src/nsURLFormatter.js,v  <--  nsURLFormatter.js
new revision: 1.2.2.3; previous revision: 1.2.2.2
done
Checking in toolkit/content/about.xhtml;
/cvsroot/mozilla/toolkit/content/about.xhtml,v  <--  about.xhtml
new revision: 1.8.8.12; previous revision: 1.8.8.11
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.18.2.14; previous revision: 1.18.2.13
done
Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.72.2.38; previous revision: 1.72.2.37
done
Checking in toolkit/mozapps/extensions/content/extensions.xml;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v  <--  extensions.xml
new revision: 1.17.2.19; previous revision: 1.17.2.18
done
Checking in toolkit/mozapps/extensions/content/list.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/list.js,v  <--  list.js
new revision: 1.2.2.8; previous revision: 1.2.2.7
done
Checking in toolkit/mozapps/update/content/updates.js;
/cvsroot/mozilla/toolkit/mozapps/update/content/updates.js,v  <--  updates.js
new revision: 1.45.2.17; previous revision: 1.45.2.16
done
Checking in toolkit/mozapps/update/src/nsUpdateService.js.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsUpdateService.js.in,v  <--  nsUpdateService.js.in
new revision: 1.77.2.40; previous revision: 1.77.2.39
done

Comment 24

[Axel Hecht [:Pike]]

how does the check-in to mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties fit with string freeze?

Even if Firefox or Thunderbird don't use those, those strings will be frozen wednesday night and seamonkey/calendar (which are the consumers here, right?) will be out of luck.

This needs at least a post in .l10n.

Comment 25

[Dietrich Ayala (:dietrich)]

(In reply to comment #24)
> how does the check-in to
> mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
> fit with string freeze?
> 
> Even if Firefox or Thunderbird don't use those, those strings will be frozen
> wednesday night and seamonkey/calendar (which are the consumers here, right?)
> will be out of luck.
> 
> This needs at least a post in .l10n.
> 

Hey Axel, I apologize if this caused a problem. However, likely due to inexperience, I don't understand what problem this has created. Can you please explain why those apps are out of luck? I think calendar has patches to use the formatter, and suite has bugs open for those tasks.

Comment 26

[Robert Kaiser]

(In reply to comment #24)
> Even if Firefox or Thunderbird don't use those, those strings will be frozen
> wednesday night and seamonkey/calendar (which are the consumers here, right?)
> will be out of luck.

SeaMonkey is no consumer of mozapps on branch, and I thin calendar will do its next release from trunk, so the problem is probably FF/TB-only.