477620 - Enable addition of search plugins

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

Sites make the availability of search plugins known (though not very obviously) in desktop Firefox by a glow on the search engine selector button, and then have an option in that menu to add the search engine in question.  Fennec should have some way of doing both these things, as well:  (1) making it known that there is a plugin to be had, and (2) allowing the user to add the plugin.

My suggested solution is in an extended site button (ie. where the site identity information lives (Larry)).  See linked image, below.  A weakness here is that the presence of the search plugin is not announced in primary chrome, but, with our minimal chrome (intentionally so) I think this is a fair trade here.  Users who want an engine can check to see if one exists.  We can think more about how to make this more obvious once this is in place, too.

http://people.mozilla.com/~madhava/files/mobile/2008-11-18/site_button.png

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Madhava, I'm using this as a reference : https://bug494218.bugzilla.mozilla.org/attachment.cgi?id=378896, but what should we display if the page offer more more than one search plugin?

Do we want to:
 * use the title of the search engine as the button label?
 * display multiples buttons?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-1

Lot of changes for a small feature but much of them are done to break the glue betweeen the IdentityHandler and the popup above the urlbar.

It miss a 'per tab' mechanism to this patch.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-2

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-3 - Updated on the trunk

I'm continue to call it a WIP cause I'm not sure at all I'm going in the right direction by adding a SiteSpecific JS Object.
In some manner it sounds logical since at the end we want to have a panel to control/give some info per site, but right now it seems overkilled.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: patch v0.1

smaller version (as requested by Mark on IRC)

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Behavior/style morph to be what have been said on IRC

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 396593 [details] [diff] [review]
Patch v0.2

>diff -r 8f0e9ca2c3a5 chrome/content/browser-ui.js

>+  _searchProvider: null,

Skip this. We'll handle it in the BrowserSearch

>+    else if (!this._searchProvider && /\bsearch\b/i(link.rel)) {
>+      var type = link.type && link.type.toLowerCase();
>+      type = type.replace(/^\s+|\s*(?:;.*)?$/g, "");
>+      if (type == "application/opensearchdescription+xml" && link.title && /^(?:https?|ftp):/i.test(link.href)) {
>+        var engine = { title: link.title, href: link.href };
>+        this._searchProvider = engine;
>+
>+        BrowserSearch.addPageSearchEngine(engine, link.ownerDocument);
>+      }
>+    }

Again, let's build the real array of all the engines, but we'll only show the first

>         this._favicon.src = "";
>         this._faviconLink = null;
>+        this._searchProvider = null;

Remove this too

>+const BrowserSearch = {

>+  updatePageSearchEngines: function() {
>+
>+    let buttons = container.getElementsByAttribute("class", "search-engine-button button-dark");
>+    for (let i=0; i<buttons.length; i++)
>+      buttons[i].hidden = true;

These we can really remove like your previous patch

>+    for (let i = 0; i<newEngines.length; i++) {

Here, we will limit to 1 and only 1 button :)  (for now)

>+      button.setAttribute("label", engine.engine.title);
>+      button.setAttribute("image", BrowserUI._favicon.src);

It would be better to use the engine.engine.iconURI.spec (if we have it)

>+  updateSearchButtons: function() {

>+    // Clean the previous search engines button
>+    var container = document.getElementById("search-buttons");
>+    while (container.hasChildNodes())
>+      container.removeChild(container.lastChild);

This was the place I wanted to avoid removing buttons, but we can file a followup bug for that, if needed. Let's try this as is for now.

>+.search-engine-button .button-icon {
>+  width: 16px;
>+  height: 16px;
>+}

Might want this to be bigger (yeah we upscale favicons on high-dpi systems)

Also, button text is already smaller than normal. No need to go 80% (at least to start)

>diff -r 8f0e9ca2c3a5 themes/wince/browser.css

>+.search-engine-button .button-icon {
>+  width: 16px;
>+  height: 16px;
>+}

Probably want this in browser-high (30px) and browser-low (16px)

Sorry for the work this patch is causing you. I swear, the next patch _will_ land

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

Address comments.

(In reply to comment #8)
> >+      button.setAttribute("label", engine.engine.title);
> >+      button.setAttribute("image", BrowserUI._favicon.src);
> 
> It would be better to use the engine.engine.iconURI.spec (if we have it)

Nope it's not already available.
 
> >+  updateSearchButtons: function() {
> 
> >+    // Clean the previous search engines button
> >+    var container = document.getElementById("search-buttons");
> >+    while (container.hasChildNodes())
> >+      container.removeChild(container.lastChild);
> 
> This was the place I wanted to avoid removing buttons, but we can file a
> followup bug for that, if needed. Let's try this as is for now.

I can provide an updated patch for that, but is it so perf killer?
We do a check at the start of this function, so the buttons are destroyed/created only if a new engines has been added.

> Also, button text is already smaller than normal. No need to go 80% (at least
> to start)

The css rule is not for the button, but for the 'Search Add:' text. should i show it bigger?


> Sorry for the work this patch is causing you. I swear, the next patch _will_
> land

hey! Sorry for the review this patch is causing you :p

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 396696 [details] [diff] [review]
Patch v0.3

>+  updatePageSearchEngines: function() {

>+    let container = document.getElementById('search-container');
>+    if (newEngines.length == 0) {
>+      container.hidden = true;
>+      return;
>+    }
>+
>+    let buttons = container.getElementsByAttribute("class", "search-engine-button button-dark");
>+    for (let i=0; i<buttons.length; i++)
>+      container.removeChild(buttons[i]);

Let's always delete the old buttons, so I'll move this above the empty engine check

>+    // XXX limit to the first search engine for now
>+    for (let i = 0; i<1; i++) {
>+      let button = button = document.createElement("button");

I'll remove the extra "button"

r+ and I can tweak the changes when I land it

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed: https://hg.mozilla.org/mobile-browser/rev/3d7a9711dea2

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

verified with 20090827 nightly winmo build.  There is a amo search that you can add in.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

in-litmus+

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9768