1188618 - Default engine icon for engines without a favicon is broken in the in-content search UI

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

I also noticed that when cycling through engines, the default engine is updated three times per engine change. This patch fixes all of this.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v1.1

Added a comment.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8640143 [details] [diff] [review]
Patch v1.1

Review of attachment 8640143 [details] [diff] [review]:
-----------------------------------------------------------------

Some explanation of what I'm doing in this patch.

::: browser/base/content/contentSearchUI.js
@@ -451,5 @@
> -        icon: button.firstChild.getAttribute("src"),
> -      };
> -    }
> -    this._sendMsg("SetCurrentEngine", engine.name);
> -    this.defaultEngine = engine;

I originally had this to prevent a possible delay between keyboard input and a visible change in the UI, but the delay is really unnoticeable and removing this means we update the header one less time.

@@ -758,5 @@
>      let header = document.createElementNS(HTML_NS, "td");
>      headerRow.setAttribute("class", "contentSearchHeaderRow");
>      header.setAttribute("class", "contentSearchHeader");
> -    let img = document.createElementNS(HTML_NS, "img");
> -    img.setAttribute("src", "chrome://browser/skin/search-engine-placeholder.png");

This line is really unnecessary, since at this point we don't even have an engine name. We can set the icon later when we have more info.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch v2

Use @2x image for retina displays. This also makes the UI use the @2x image for the placeholder icon in one-off buttons.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8640197 [details] [diff] [review]
Patch v2

Clearing review since I think you're working on a new patch.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8640197 [details] [diff] [review]
Patch v2

Patch v2 is ready for review. :)

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8640197 [details] [diff] [review]
Patch v2

Review of attachment 8640197 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry!  Looks fine.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/18f9c283b687

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18f9c283b687