Always provide the ability to add an engine for a site

NEW
Unassigned

Status

()

defect
P3
normal
Rank:
35
10 years ago
8 months ago

People

(Reporter: rflint, Unassigned)

Tracking

(Blocks 2 bugs)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.6 -
wanted-firefox3.6 +

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [fxsearch], URL)

Attachments

(3 attachments, 8 obsolete attachments)

1.50 KB, patch
mconnor
: approval1.9.1+
Details | Diff | Splinter Review
1.34 KB, patch
rflint
: review+
Gavin
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
47.42 KB, patch
Details | Diff | Splinter Review
Posted patch WIP (obsolete) — Splinter Review
See URL for description.
Posted patch Strings (obsolete) — Splinter Review
Attachment #368429 - Flags: ui-review?(beltzner)
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.
Attachment #368429 - Flags: ui-review?(beltzner) → ui-review-
Posted patch New strings (obsolete) — Splinter Review
Attachment #368429 - Attachment is obsolete: true
Attachment #368440 - Flags: ui-review?(beltzner)
Attachment #368440 - Flags: review?(gavin.sharp)
Attachment #368440 - Flags: ui-review?(beltzner) → ui-review+
Attachment #368440 - Flags: review?(gavin.sharp) → review+
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.
Attachment #368454 - Flags: approval1.9.1+
http://hg.mozilla.org/mozilla-central/rev/451c1f1753f0
Keywords: checkin-needed
Whiteboard: [strings landed on 1.9.1] → [strings landed]

Comment 8

10 years ago
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.
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.
Posted patch Patch (obsolete) — Splinter Review
Attachment #366284 - Attachment is obsolete: true
Attachment #369440 - Flags: review?(gavin.sharp)
Attachment #368454 - Attachment description: patch → strings (checked in)
Posted patch strings bustage fix (obsolete) — Splinter Review
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.
Attachment #370223 - Flags: review?(l10n)
Attachment #370223 - Flags: review?(gavin.sharp)
Attachment #370223 - Flags: approval1.9.1?

Updated

10 years ago
Attachment #370223 - Flags: review?(l10n) → review+
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.
Attachment #370223 - Flags: review?(gavin.sharp)
Attachment #370223 - Flags: review-
Attachment #370223 - Flags: approval1.9.1?
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.

Comment 30

10 years ago
(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.
Attachment #370223 - Attachment is obsolete: true
Attachment #370230 - Flags: review?(rflint)
Attachment #370230 - Flags: review?(gavin.sharp)
Attachment #370230 - Flags: review?(rflint) → review+
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.
Attachment #370230 - Flags: review?(gavin.sharp) → review+
Attachment #370230 - Flags: approval1.9.1?
Comment on attachment 370230 [details] [diff] [review]
strings bustage fix, without gfns=1 (checked in)

a191=beltzner
Attachment #370230 - Flags: approval1.9.1? → approval1.9.1+
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
Attachment #370230 - Attachment description: strings bustage fix, without gfns=1 → strings bustage fix, without gfns=1 (checked in)
No longer blocks: 356128
Duplicate of this bug: 356128
Ryan, could you create a tryserver build so I could run a test with this patch while we are waiting for review?
Whiteboard: [strings landed] → [strings landed][has patch][needs review gavin]
(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?
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].
Flags: wanted1.9.0.x?
We'll have to get this for 1.9.2 ... (sniff)
Flags: blocking-firefox3.6?
Blocks: 495226
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?
Duplicate of this bug: 417717
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.
blocking2.0: --- → alpha1
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
Posted patch Updated (obsolete) — Splinter Review
Attachment #369440 - Attachment is obsolete: true
Attachment #403342 - Flags: review?(gavin.sharp)
Attachment #369440 - Flags: review?(gavin.sharp)
Posted patch Actually updated (obsolete) — Splinter Review
doh.
Attachment #403342 - Attachment is obsolete: true
Attachment #403343 - Flags: review?(gavin.sharp)
Attachment #403342 - Flags: review?(gavin.sharp)
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.
Attachment #403343 - Flags: review?(gavin.sharp) → review-
Whiteboard: [strings landed][has patch][needs review gavin] → [strings landed][has patch][needs new patch]
(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.
Posted patch Patch v2 (obsolete) — Splinter Review
(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.
Attachment #403343 - Attachment is obsolete: true
Attachment #403449 - Flags: review?(gavin.sharp)
Whiteboard: [strings landed][has patch][needs new patch] → [strings landed][has patch][needs review gavin]
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");
Keywords: late-l10n
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.
Posted patch Patch v2.1Splinter Review
(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.
Attachment #403449 - Attachment is obsolete: true
Attachment #403594 - Flags: review?(gavin.sharp)
Attachment #403449 - Flags: review?(gavin.sharp)
(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
blocking2.0: alpha1 → ?
Target Milestone: Firefox 3.6a1 → Firefox 3.7
Blocks: 415958
Blocks: 495745
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.
blocking2.0: ? → -
No longer blocks: 415958

Updated

9 years ago
Duplicate of this bug: 597916

Updated

9 years ago
Duplicate of this bug: 614807
Flags: wanted1.9.0.x?
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.
Whiteboard: [strings landed][has patch][needs review gavin] → [strings pre-landed for 3.5][has patch][needs review gavin]
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.
Attachment #403594 - Flags: review?(gavin.sharp)
Whiteboard: [strings pre-landed for 3.5][has patch][needs review gavin] → [strings pre-landed for 3.5][has patch]
No longer blocks: 412322
No longer blocks: 425806
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
Assignee: rflint → nobody
Whiteboard: [strings pre-landed for 3.5][has patch]
Target Milestone: Firefox 4.0 → ---
https://hg.mozilla.org/mozilla-central/rev/4ef2d2c3b9d0
Assignee: nobody → gavin.sharp
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Assignee: gavin.sharp → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 19 → ---
No longer blocks: 356128, 405443, 495226, 495745
Status: REOPENED → NEW
Priority: -- → P3
Whiteboard: [fxsearch]

Updated

4 years ago
Rank: 35
You need to log in before you can comment on or make changes to this bug.