Closed Bug 323798 Opened 18 years ago Closed 18 years ago

Keyword URL Needs to be localized

Categories

(Firefox :: Search, defect, P1)

2.0 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 2 beta2

People

(Reporter: rebron, Assigned: Gavin)

References

Details

(Keywords: fixed1.8.1, Whiteboard: [mustfix])

Attachments

(1 file, 2 obsolete files)

This setting needs to be localizable for Firefox 2.0.
*** Bug 323803 has been marked as a duplicate of this bug. ***
Assignee: nobody → l10n
Attached patch patch (obsolete) — Splinter Review
Adds keyword.URL to region.properties, change the protocol handler to get the pref with getComplexValue.

I'm not entirely familiar with the string classes, nor am I sure that I got the error handling right (is the NS_FAILED || IsEmpty check redundant?), but it seems to work in my limited testing.
Assignee: l10n → gavin.sharp
Status: NEW → ASSIGNED
Attachment #208771 - Flags: ui-review?(darin)
Attachment #208771 - Flags: review?(l10n)
Flags: blocking-firefox2?
Attachment #208771 - Flags: ui-review?(darin) → superreview?(darin)
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Firefox 2
Version: unspecified → 1.5 Branch
Sigh... I also changed firefox.js, but it's not in the patch:
-pref("keyword.URL", "http://www.google.com/search?btnI=I%27m+Feeling+Lucky&ie=UTF-8&oe=UTF-8&q=");
+pref("keyword.URL", "chrome://browser-region/locale/region.properties");
I need a rationale here. I have my doubts that this is true.
This follows the assumption that your set language had something to do with what 
you do. Do we have a use case here?
Is this something we want to have as branding, or would we rather do a 
meta-webservice in terms of a redirect, maybe even basing on geo location?
Or is this something that we want to delegate to the l10n team, and how would the
trademarks policy look here?

There are may things in region.properties, which are bad design, and I'm working
on cutting that file down to one or two entries, so I need more than a bug filed.
Another question, does this patch prevent the user from overriding the keyword URL via about:config?  Or, does it do the right thing if they specify a non-chrome URL for the value of the pref?  I wonder if the keyword protocol handler should fallback to using getCharPref if getComplexValue fails.
(In reply to comment #5)
> Another question, does this patch prevent the user from overriding the keyword
> URL via about:config?  Or, does it do the right thing if they specify a
> non-chrome URL for the value of the pref?  I wonder if the keyword protocol
> handler should fallback to using getCharPref if getComplexValue fails.'

I tested that case, and nothing breaks. getComplexValue does the fallback automatically.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpref/src/nsPrefBranch.cpp&rev=1.67#245
OK, that makes sense.  However, I guess that this will break other apps that do not define this value in a properties file, right?
Comment on attachment 208771 [details] [diff] [review]
patch

This is branding more than l10n, so I'm going to r- this patch.
I'll come up with a constructive comment on where that should be once I draft bug 324330
Attachment #208771 - Flags: review?(l10n) → review-
Attachment #208771 - Flags: superreview?(darin)
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: Firefox 2 → Firefox 2 beta1
Version: 1.5.0.x Branch → 2.0 Branch
(In reply to comment #8)
> I'll come up with a constructive comment on where that should be once I draft
> bug 324330

Ok. Do you think that this bug should depend on bug 324330?
Priority: -- → P4
Phil, we need to know if this is something that *must* be part of beta1 as per the l10n strategy going forward. Please add 181b1+ to the status whiteboard. Thanks.
pushing out non-critical-path bugs to b2
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
> (In reply to comment #8)
> Ok. Do you think that this bug should depend on bug 324330?

No, it should not. Axel's said that bug won't make Firefox 2.0. Axel, where should this go now?(In reply to comment #9)
I don't think it's a must for beta1, but I'd probably say yes for beta 2.  I don't know how many people use this feature, though.

This is possibly related to the discussion we're having in bug 336338, because Google has asked us to standardize on a http://www.google.com/query.cgi?...&rls=org.mozilla:en-US:official&hl=en format for all locales, and let them handle the redirection
(In reply to comment #13
> http://www.google.com/query.cgi?...&rls=org.mozilla:en-US:official&hl=en

Having the "org.mozilla" and "official" parts be dynamic will require either some extra code in nsDefaultURIFixup.cpp, or preprocessing of the pref at build time, since the keyword URL is currently just prepended to the search terms before the request is sent.

I'm going to be making that parameter work for the shipped Google search plugin in bug 335460. It would be interesting to have the keyword functionality be handled by a hidden search plugin, since that would allow us to leverage the dynamic parameters from the search service, however the fact that the keyword fixup is implemented at a lower level than the search service probably makes that unfeasible.
Whiteboard: [mustfix]
Attached patch updated patch (obsolete) — Splinter Review
Since it looks like bug 324330 won't make Firefox 2, I've updated my previous patch to address Darin's comments about backwards compatibility (the code changed location as well, due to bug 337339).
Attachment #208771 - Attachment is obsolete: true
Attachment #230541 - Flags: review?(l10n)
Whiteboard: [mustfix] → [patch-r?][mustfix]
Attachment #230541 - Flags: review?(l10n) → review+
Attachment #230541 - Flags: superreview?(darin)
Attachment #230541 - Flags: superreview?(darin) → superreview?(bzbarsky)
Comment on attachment 230541 [details] [diff] [review]
updated patch

>Index: docshell/base/nsDefaultURIFixup.cpp
>+        url = NS_ConvertUTF16toUTF8(wurl);

  NS_CopyUTF16toUTF8(wurl, url);

sr=bzbarsky with that
Attachment #230541 - Flags: superreview?(bzbarsky) → superreview+
Attached patch for checkinSplinter Review
Attachment #230541 - Attachment is obsolete: true
mozilla/docshell/base/nsDefaultURIFixup.cpp 	1.50
mozilla/browser/locales/en-US/chrome/browser-region/region.properties 	1.16
mozilla/browser/app/profile/firefox.js 	1.135
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Priority: P4 → P1
Resolution: --- → FIXED
Whiteboard: [patch-r?][mustfix] → [need-a][mustfix]
Comment on attachment 230681 [details] [diff] [review]
for checkin

This patch adds a check for a localized pref before falling back to the standard non-localized pref, and adds one localizable string (the keyword URL) to Firefox's region.properties. The risk of regression is very low.
Attachment #230681 - Flags: approval1.8.1?
Whiteboard: [need-a][mustfix] → [mustfix][has patch][needs approval]
Attachment #230681 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [mustfix][has patch][needs approval] → [checkin needed (1.8 branch)][mustfix]
mozilla/browser/app/profile/firefox.js 	1.71.2.57
mozilla/browser/locales/en-US/chrome/browser-region/region.properties 	1.9.2.8
mozilla/docshell/base/nsDefaultURIFixup.cpp 	1.44.18.5
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)][mustfix] → [mustfix]
Gavin:

A question about what was done here. When you changed the keyword.url in firefox.js to be a chrome value, does the prefs code automatically know how to jump through the chrome to the region.properties? This is an area I don't understand well. I assume this is the case, because when I use about:config, I see the final URL, not the chrome URL.
Ben, did you try a new profile?

The code change that looks up the localized value first is https://bugzilla.mozilla.org/attachment.cgi?id=230681&action=diff#mozilla/docshell/base/nsDefaultURIFixup.cpp_sec2, one needs to get the complex value for a nsIPrefLocalizedString, http://developer.mozilla.org/en/docs/Code_snippets:Preferences#nsIPrefLocalizedString
(In reply to comment #21)
> A question about what was done here. When you changed the keyword.url in
> firefox.js to be a chrome value, does the prefs code automatically know how to
> jump through the chrome to the region.properties?

No, it doesn't "automatically" know - that's why I changed the code that retrieves the pref to first try a localized pref, and then fall back to a normal "char" pref. about:config has code that does something similar, so that you see the actual value rather than the chrome URI.
Depends on: 458501
You need to log in before you can comment on or make changes to this bug.