Closed Bug 1188618 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

(Whiteboard: [ui][fxsearch])

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
I also noticed that when cycling through engines, the default engine is updated three times per engine change. This patch fixes all of this.
Attachment #8640138 - Flags: review?(adw)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Added a comment.
Attachment #8640138 - Attachment is obsolete: true
Attachment #8640138 - Flags: review?(adw)
Attachment #8640143 - Flags: review?(adw)
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.
Attached patch Patch v2Splinter Review
Use @2x image for retina displays. This also makes the UI use the @2x image for the placeholder icon in one-off buttons.
Attachment #8640143 - Attachment is obsolete: true
Attachment #8640143 - Flags: review?(adw)
Attachment #8640197 - Flags: review?(adw)
Comment on attachment 8640197 [details] [diff] [review]
Patch v2

Clearing review since I think you're working on a new patch.
Attachment #8640197 - Flags: review?(adw)
Comment on attachment 8640197 [details] [diff] [review]
Patch v2

Patch v2 is ready for review. :)
Attachment #8640197 - Flags: review?(adw)
Comment on attachment 8640197 [details] [diff] [review]
Patch v2

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

Sorry!  Looks fine.
Attachment #8640197 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/18f9c283b687
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.