See URL for description.

Comment on attachment 368429 [details] [diff] [review] Strings >+sitesearch=%S Search I think we want just %s >+sitesearch_description=Search for items on %S siteSearchDescription=Search %S using Google with a localization note saying that the provider will need to be dependent on the pref.

Comment on attachment 368440 [details] [diff] [review] New strings We've decided we don't need the search.properties strings, so r=me on the region.properties string only.

This is a bad landing. Changes to region.properties need to go through l10n-drivers review, so this is not one string, but a localization change that needs a l10n policy and a process to implement it for all 70 locales.

Instead of kvetching, let's just deal with it now. Were you to review it, what comments would you have?

The problem is less the review of the en-US patch, but the review of the 70 patches for all localizations that we have to do now, including more in-depth checking if any landing to region.properties regressed previous settings. So my review comment would be "can we land this on mozilla-central at a time where the l10n-drivers team is not busy with shipping a release so that we can don't have to pull the manpower off of release stuff"

How about "don't translate this string" as a policy? I can help review changes to region.properties to enforce it.

Well, the policy sounds like an awkward choice for localizations that come with something non-google as default engine. http://l10n.mozilla.org/stage/pushes/?from=1237427286.973&length=50&path=browser-region%2Fregion.properti# is a query on my own pushlog impl on changes to region.properties that landed without bugs and review. Didn't go through all of them, but found a broken patch in the first I checked, http://hg.mozilla.org/releases/l10n-mozilla-1.9.1/fy-NL/diff/80cc590058b2/browser/chrome/browser-region/region.properties.

Well, other search providers may not support site-specific searches, and until l10n-drivers have resources in place to ensure that they do, a "no changes" policy seems simplest as a temporary measure. I can post to the l10n newsgroup and get in touch with the fy-NL team directly, if that would help. The it/id/fr/eo/ru changes all look fine.

Hey folks, can we change the premise for this from being based on a google search to being based on whatever the default search is? there are a number of non-default distributions that don't use google as a default, along with one default (ru), and using Google as default is not something I'm crazy about because of the potential impacts. I can provide more info as required.

It would be nice if we could, but I don't think we can use the same URLs for site search and normal search given that different search engines have different ways of specifying site-specific searches. "site:nhl.com test" does not work the same way for Google and Yandex.

The "hl" parameter in the query string for this URL is a language code, so "don't translate this" is a bad policy, too. http://www.google.com/search?hl=en&q=site%3Amozilla-europe.org+firefox http://www.google.com/search?hl=pl&q=site%3Amozilla-europe.org+firefox http://www.google.com/search?hl=ro&q=site%3Amozilla-europe.org+firefox

I'd request that we don't use google as a template, then, or we allow it to be localized/customized. it doesn't just affect Yandex, there are other distributions out there that are offered by other providers (i.e. Yahoo!). I'd like to ensure that customizations via distribution.js work properly as a requirement. the ability to use the pref is great, I'd just like it to be both localizable and supported via customizations properly

My suggestion would be to back this out, and land it once it's properly spec'ed out.

(In reply to comment #17) > I'd request that we don't use google as a template, then, or we allow it to be > localized/customized. We are allowing it to be customized; that's why we landed it in region.properties. My comment 11 was a specific short-term proposal to address Axel's concern that l10n-drivers are currently overburdened and wouldn't be able to deal with reviewing cutomized URLs for each locale.

(In reply to comment #18) > My suggestion would be to back this out, and land it once it's properly spec'ed > out. What kind of spec are you looking for? I don't understand what details you think need to be sorted out.

No problem with it being in region.properties, I just want to ensure that if the pref is defined in <appdir>/distribution/distribution.ini, it'll trump what's in the defaults. this is the first I've heard about this, so apologize for being late t the party.

Being spec'ed out means that folks like partners and l10n-drivers know what to do about it in the non-google case, and have the right message to send out, and time to do so. Just having a string in a file that localizers can write to is not sufficient.

(In reply to comment #21) > No problem with it being in region.properties, I just want to ensure that if > the pref is defined in <appdir>/distribution/distribution.ini, it'll trump > what's in the defaults. this is the first I've heard about this, so apologize > for being late t the party. I don't remember as much about distribution stuff as I wish I would, but I think that should already work correctly. This pref is treated the same as any others in that file, which are already being changed by distribution builds.

(In reply to comment #22) > Being spec'ed out means that folks like partners and l10n-drivers know what to > do about it in the non-google case, and have the right message to send out, and > time to do so. The URL should return site-specific search results (i.e. results limited to the domain specified by {moz:domain}). The search engine chosen should be consistent with the locale defaults. If the locale's default search engine does not provide that functionality, you should either fall back to Google (if the conflict is deemed acceptable) or use an empty pref value (which the code can detect and avoid offering the feature). How is that for a spec? I'm not sure what you mean by "right message to send out", but I'm willing to help out with "time to do so" if I can.

The URL that landed in attachment 368454 [details] [diff] [review] hard-codes the "en" locale, which prevents Google from doing automatic language detection. This is bad, because the en-US builds are also used outside the US. Here's a patch to fix that and to unify the URL in en-US with what we asked the localizers to use in the localized builds. This should land asap as a bustage fix to the strings that have already landed. The sooner it lands, the fewer locales will copy the wrong URL.

Comment on attachment 370223 [details] [diff] [review] strings bustage fix You've copied the keyword.URL value which does an "I'm feeling lucky" search. We always want results to be returned for this feature.

I've tested it and it always gives me the results page.

try it with single word keywords like "ford". keyword.URL doesn't do a "I'm feeling lucky" search, it performs a browse by name search. (see http://www.squarefree.com/2004/09/09/googles-browse-by-name-in-firefox/) make sure gfns=1 is not in there.

(In reply to comment #28) Tested it without the gfns=1 http://www.google.com/search?ie=UTF-8&oe=UTF-8&sourceid=navclient&q=site%3Amozilla-europe.org+firefox and I get all the results for "en".

A better patch.

Comment on attachment 370230 [details] [diff] [review] strings bustage fix, without gfns=1 (checked in) Looks good as long as sourceid=navclient is the right thing to be using here.

Comment on attachment 370230 [details] [diff] [review] strings bustage fix, without gfns=1 (checked in) a191=beltzner

Comment on attachment 370230 [details] [diff] [review] strings bustage fix, without gfns=1 (checked in) https://hg.mozilla.org/mozilla-central/rev/c213ae76317f https://hg.mozilla.org/releases/mozilla-1.9.1/rev/70ea489c0644

Ryan, could you create a tryserver build so I could run a test with this patch while we are waiting for review?

(In reply to comment #36) > Ryan, could you create a tryserver build so I could run a test with this patch > while we are waiting for review? https://build.mozilla.org/tryserver-builds/2009-04-14_22:27-rflint@ryanflint.com-sitesearch2/ Looks like there's an enormous backlog for non-windows builds.

Thanks Ryan! I did a testrun with this build on OS X and Windows and it looks great! The autodiscovery is eliminated and the menu entry is always visible. As what I can see we have three different situations here: 1. OpenSearch plugin is available. That means we add the search engine to the list of search engines. 2. No OpenSearch plugin is available but we have a keyworded search on the web page. We add a search engine too. 3. No OpenSearch and keyworded search available. We add a search engine which uses Google to search on the website. Ryan, for point 2 I see a inconsistency between the implementation and the spec. Probably the latter one is a bit unclear. So when a user wants to add a search engine we don't ask him for a keyword? We just add it as a search engine and the keyword field has to be filled-out manually afterward. Now after further testing I can see another problem. Please open the following web page: http://dict.leo.org/ende?lp=ende&lang=de&searchLoc=0&cmpType=relaxed&sectHdr=on&spellToler=on&chinese=both&pinyin=diacritic&search=fix&relink=on Try to add the provided search engine which is called "Leo Eng-Deu". When done open the context menu and inspect the added search. This search is called "Leo Deu-Eng" and the search the user wanted to add is still listed beneath. You can try to add it several times but it will not be removed from the context menu.

(In reply to comment #38) > Try to add the provided search engine which is called "Leo Eng-Deu". When done > open the context menu and inspect the added search. This search is called "Leo > Deu-Eng" and the search the user wanted to add is still listed beneath. You can > try to add it several times but it will not be removed from the context menu. This is definitely another issue which also happens in the current state. You know an already existing bug?

Sounds like bug 351441.

Still kind of disappointed we didn't get this reviewed in time for beta 4, especially since we landed all the l10n aspects - will we be confident enough to take this post-b4? If so, what will make us thusly-confident?

Where are we on this bug?

(In reply to comment #42) > Where are we on this bug? We are still waiting for Gavin's review for attachment 369440 [details] [diff] [review].

We'll have to get this for 1.9.2 ... (sniff)

Gavin, can you give an ETA when you will be able to review this patch?

Any update for this project? Will it even not make into Firefox 3.6?

Gavin: can we get someone else to review this if you're too busy? I'd like to not miss another release for it, but wouldn't block on it. Definitely need to fix before alpha 1 of Firefox 3.7, though.

doh.

Comment on attachment 403343 [details] [diff] [review] Actually updated >diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml >+ <method name="getIconURL"> >+ let iconURI = iconSvc.getFaviconForPage(gBrowser.mCurrentBrowser >+ .currentURI); change this to use gBrowser.currentURI while you're at it? >+ <method name="discoverNewEngines"> >+ <body><![CDATA[ >+ let browser = gBrowser.mCurrentBrowser; >+ if (!browser.currentURI.schemeIs("http") && !browser.currentURI.schemeIs("https")) >+ return; and use .selectedBrowser here (and a temporary for currentURI?) >+ for (let i = 0; i < links.length; i++) { >+ let link = links[i]; >+ let type = link.type && link.type.toLowerCase(); >+ type = type.replace(/^\s+|\s*(?:;.*)?$/g, ""); while you're here, type.trim().replace(/;.*$/, "") is clearer and equivalent (I think) >+ // No OpenSearch description was found; generate a site-specific engine >+ let title = browser.currentURI.host; >+ try { >+ var url = gPrefService.getComplexValue("browser.search.siteSearchURL", >+ Components.interfaces.nsIPrefLocalizedString).data; use Ci? >+ url = url.replace(/\{moz:domain\}/g, title); "title" as a variable name here is rather confusing, title: host below would be clear enough. > <handler event="command"><![CDATA[ >+ if (target.hasAttribute("sitesearch")) { Just set a JS property on the element rather than setting attribute? >+ // We only detect OpenSearch files >+ let type = Components.interfaces.nsISearchEngine.DATA_XML; Ci here too I noticed a bug here while testing. If you open the popup while the page is loading, i.e. before it has a <link rel=search>, and then again after, you'll see both entries in the popup. That's because we add the sitesearch entry to .engines and then never remove it. Is there a reason to keep the engines on the browser at all given that we're re-walking the DOM on each open? r- because of that, but this looks OK otherwise.

(In reply to comment #51) > >+ for (let i = 0; i < links.length; i++) { > >+ let link = links[i]; > >+ let type = link.type && link.type.toLowerCase(); > >+ type = type.replace(/^\s+|\s*(?:;.*)?$/g, ""); > > while you're here, type.trim().replace(/;.*$/, "") is clearer and equivalent (I > think) type.replace(/;.*$/, "").trim() is equivalent.

(In reply to comment #51) > Is there a reason to keep the engines on the > browser at all given that we're re-walking the DOM on each open? Nope. I think we were just later in the release cycle when the patch was first written and were more averse to change at that time.

Comment on attachment 403449 [details] [diff] [review] Patch v2 >+ url = url.replace(/\{moz:domain\}/g, host); This doesn't need to be a regexp... url = url.replace("{moz:domain}", host, "g");

Comment on attachment 403449 [details] [diff] [review] Patch v2 >diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml >+ <method name="discoverNewEngines"> >+ let browser = gBrowser.selectedBrowser; >+ let browserURI = browser.currentURI; >+ if (!browserURI.schemeIs("http") && !browserURI.schemeIs("https")) >+ return; I thought this would cause a "function does not always return a value" strict warning, but it doesn't seem to for some reason. Wouldn't hurt to fix anyways (return null?). >+ for (let i = 0; i < links.length; i++) { >+ let link = links[i]; >+ let type = link.type && link.type.toLowerCase(); >+ type = type.replace(/;.*$/, "").trim(); This problem existed before, but if type can be null, this will throw. If it can't, the check is redundant. Looks to me like it's "" even if omitted from the element, so the check is unneeded. >+ if (type == "application/opensearchdescription+xml" && link.title && >+ /^(?:https?|ftp):/i.test(link.href)) { >+ if (engines.some(function (e) e.title == link.title)) >+ continue; >+ >+ // XXX This will need to be changed when engines are identified by URL; >+ // see bug 335102. >+ if (enginesHidden || this.searchService.getEngineByName(link.title)) { >+ enginesHidden = true; >+ continue; This doesn't look right... once we find a duplicate, we stop adding engines entirely? Seems like we should avoid checking enginesHidden here. Or better yet, just add a "foundValidEngine" boolean and set it unconditionally before checking for an existing engine, and then use that single check below: >+ if (engines.length || enginesHidden) >+ return engines; Do we have a plan for offering a better title than just the hostname in a followup bug? Just the hostname is kind of crappy.

(In reply to comment #55) > >+ for (let i = 0; i < links.length; i++) { > >+ let link = links[i]; > >+ let type = link.type && link.type.toLowerCase(); > >+ type = type.replace(/;.*$/, "").trim(); > > This problem existed before, but if type can be null, this will throw. If it > can't, the check is redundant. Looks to me like it's "" even if omitted from > the element, so the check is unneeded. It's undefined when the attribute isn't present. > Do we have a plan for offering a better title than just the hostname in a > followup bug? Just the hostname is kind of crappy. The alternatives aren't much better and the hostname is what was settled on when it was discussed before the 3.5 string freeze. I can certainly file a bug to revive the discussion, though.

(In reply to comment #56) > It's undefined when the attribute isn't present. That's not what I saw in my test. Looking at the implementation, it just ends up calling GetAttr (ignoring its return value via NS_IMPL_STRING_ATTR->GetAttrHelper), which returns a truncated string: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#4526

Ryan, Gavin, what's the status in this bug?

Status is that neither I or Ryan have had time to work on it. I'm not sure whether that's going to change in the near future.

We didn't hold the last release for this, and won't hold this one either. The patch seems like it's mostly there though - would take it if it's completed in time.

Wow. AFAICT, a pre-landed string for 3.5 and the bug isn't fixed still. First thing after fx4, we should either WONTFIX this and back the string out, or actually fix this.

Gavin, can we please get an assessment if we want this feature or not? Firefox 4 has now been released and this patch is still waiting. Thanks.

Comment on attachment 403594 [details] [diff] [review] Patch v2.1 The patch no longer applies. We probably do still want to do something like this, but need someone to work on it.

Gavin, can we rip this string out again? bug 633773 wants to change it to https, and that's gonna cause way more trouble than plain removing it.

Yes, feel free to remove it.

I removed the landed string: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef2d2c3b9d0