Keyword URL Needs to be localized

RESOLVED FIXED in Firefox 2 beta2

Status

()

defect
P1
normal
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: rebron, Assigned: Gavin)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 beta2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mustfix])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

14 years ago
This setting needs to be localizable for Firefox 2.0.

Comment 1

14 years ago
*** Bug 323803 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Assignee: nobody → l10n
Posted 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)
Reporter

Updated

14 years ago
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");

Comment 4

14 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

14 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.
(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

14 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

14 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

14 years ago
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)

Comment 13

13 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
(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]
Posted 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]

Updated

13 years ago
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+
Posted 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: 13 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]

Updated

13 years ago
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]

Comment 21

12 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.
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.

Updated

11 years ago
Depends on: 458501
You need to log in before you can comment on or make changes to this bug.