Closed Bug 350123 Opened 15 years ago Closed 15 years ago

Update URLFormatter to work with non-Fx apps

Categories

(Firefox :: General, defect)

2.0 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 2

People

(Reporter: dietrich, Assigned: dietrich)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 5 obsolete files)

- 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.
Severity: normal → major
Status: NEW → ASSIGNED
Flags: blocking-firefox2?
OS: Mac OS X 10.3 → All
Hardware: PC → All
Target Milestone: --- → Firefox 2
Blocks: 349729
Should Bugzilla Bug 349574 no "get more themes" link, themes needs work also depend on this?
Blocks: 349574
Flags: blocking-firefox2? → blocking-firefox2+
Blocks: 346186
Blocks: 350055
Any estimate on this?
Hey Rob, this is #1.5 on my list right now, aiming to fix in the next day or so.
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #235886 - Flags: review?(robert.bugzilla)
Mr. Strong, let me know if you aren't the right person to review this, or if you don't have time.
Whiteboard: [has patch][needs review robstrong]
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.
Blocks: 351152
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.
Attachment #235886 - Flags: review?(robert.bugzilla) → review-
Attached patch fix - review comments addressed (obsolete) — Splinter Review
This addresses the nits and renames the replace functions.
Attachment #235886 - Attachment is obsolete: true
Attachment #236772 - Flags: review?(robert.bugzilla)
Attached patch fix - review comments addressed (obsolete) — Splinter Review
updated patch w/ fixed indentation.
Attachment #236773 - Flags: review?(robert.bugzilla)
Attachment #236772 - Attachment is obsolete: true
Attachment #236772 - Flags: review?(robert.bugzilla)
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 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 on attachment 236773 [details] [diff] [review]
fix - review comments addressed

Canceling review, to address Christian's comments.
Attachment #236773 - Flags: review?(robert.bugzilla) → review-
Comment on attachment 236773 [details] [diff] [review]
fix - review comments addressed

eep, sorry for bugspam
Attachment #236773 - Flags: review-
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.
Attachment #236773 - Attachment is obsolete: true
>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?
(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.
Whiteboard: [has patch][needs review robstrong]
Attached patch simplified patch (obsolete) — Splinter Review
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.
Attachment #236891 - Attachment is obsolete: true
Attachment #236979 - Flags: review?(robert.bugzilla)
Whiteboard: [has patch][needs review robstrong]
Blocks: 351574
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.
No longer blocks: 350055
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
Attachment #236979 - Flags: review?(robert.bugzilla) → review+
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
Attachment #236979 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review robstrong] → [has patch][baking as of 9/8]
Blocks: 351967
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.
Attachment #237300 - Flags: approval1.8.1?
Comment on attachment 237300 [details] [diff] [review]
as checked in on trunk

a=beltzner on behalf of 181drivers
Attachment #237300 - Flags: approval1.8.1? → approval1.8.1+
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
Keywords: fixed1.8.1
Whiteboard: [has patch][baking as of 9/8]
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.
(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.
(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.
You need to log in before you can comment on or make changes to this bug.