Closed
Bug 323798
Opened 19 years ago
Closed 18 years ago
Keyword URL Needs to be localized
Categories
(Firefox :: Search, defect, P1)
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)
(deleted),
patch
|
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
This setting needs to be localizable for Firefox 2.0.
Comment 1•19 years ago
|
||
*** Bug 323803 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Assignee: nobody → l10n
Assignee | ||
Comment 2•19 years ago
|
||
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)
Reporter | ||
Updated•19 years ago
|
Flags: blocking-firefox2?
Assignee | ||
Updated•19 years ago
|
Attachment #208771 -
Flags: ui-review?(darin) → superreview?(darin)
Updated•19 years ago
|
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Firefox 2
Version: unspecified → 1.5 Branch
Assignee | ||
Comment 3•19 years ago
|
||
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");
Comment 4•19 years ago
|
||
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.
Comment 5•19 years ago
|
||
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.
Assignee | ||
Comment 6•19 years ago
|
||
(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
Comment 7•19 years ago
|
||
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 8•19 years ago
|
||
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-
Updated•19 years ago
|
Attachment #208771 -
Flags: superreview?(darin)
Updated•19 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: Firefox 2 → Firefox 2 beta1
Version: 1.5.0.x Branch → 2.0 Branch
Assignee | ||
Comment 9•19 years ago
|
||
(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?
Assignee | ||
Updated•19 years ago
|
Priority: -- → P4
Comment 10•18 years ago
|
||
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.
Comment 11•18 years ago
|
||
pushing out non-critical-path bugs to b2
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
Comment 12•18 years ago
|
||
> (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)
Comment 13•18 years ago
|
||
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
Assignee | ||
Comment 14•18 years ago
|
||
(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.
Updated•18 years ago
|
Whiteboard: [mustfix]
Assignee | ||
Comment 15•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Whiteboard: [mustfix] → [patch-r?][mustfix]
Updated•18 years ago
|
Attachment #230541 -
Flags: review?(l10n) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #230541 -
Flags: superreview?(darin)
Assignee | ||
Updated•18 years ago
|
Attachment #230541 -
Flags: superreview?(darin) → superreview?(bzbarsky)
Comment 16•18 years ago
|
||
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+
Assignee | ||
Comment 17•18 years ago
|
||
Attachment #230541 -
Attachment is obsolete: true
Assignee | ||
Comment 18•18 years ago
|
||
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]
Assignee | ||
Comment 19•18 years ago
|
||
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?
Updated•18 years ago
|
Whiteboard: [need-a][mustfix] → [mustfix][has patch][needs approval]
Updated•18 years ago
|
Attachment #230681 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [mustfix][has patch][needs approval] → [checkin needed (1.8 branch)][mustfix]
Assignee | ||
Comment 20•18 years ago
|
||
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]
Comment 21•17 years ago
|
||
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.
Comment 22•17 years ago
|
||
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
Assignee | ||
Comment 23•17 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•