Closed Bug 1277967 Opened 8 years ago Closed 8 years ago

Do a better job of handling a failing search service

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

Details

(Whiteboard: [partner search] [fxsearch])

Attachments

(2 files)

Add some error checking in search.xml
3.63 KB, patch
Details | Diff | Splinter Review
Use defaultFavicon for image
750 bytes, patch
florian
: review+
Details | Diff | Splinter Review 
One of the requirements for enterprise has been to remove all default search engines. That used to be easy, but now the only real way to do it is to remove the resource://search-plugins associations.

When this happens without distribution searchplugins, things go awry. The icon is wrong on the search dropdown and although the + sign appears when you go to sites with opensearch, the  "Add search engine" doesn't appear.

We should be able to make the search service initialize without a default engine per say.

Search won't work, but it shouldn't break the UI. And if a search engine is added, things should just start working.

To be clear, enterprises add their own search engines so they don't see this except in testing, but we can do a better job of handling this case (and it might point to other edge cases within search).
So it looks like the search.xml was originally designed to handle this case:

http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#140

But code was added that didn't know that.

For the failing cases, I used code that checked searchbar.currentEngine instead of Services.search.currentEngine.

I'm not totally happy about the first check. I guess I could explicitly check for this.currentEngine.speculativeConnect?

After these changes, the searchbar works totally as expected. It's blank with no engines (but functional) and if you go to a website with opensearch, you can add the engine and once it is added, it works as the default engine.
Attachment #8760294 - Flags: review?(florian)
Comment on attachment 8760294 [details] [diff] [review]
Add some error checking in search.xml

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

I think we misunderstood each other on Friday when I said over IRC that we should investigate. I was talking specifically about the "search icon is 1/4 of a 32x32 generic URL icon" issue.

Services.search.currentEngine should never been null (see bug 438599).

If you start allowing initialization without any search engine, you'll have other pieces of the UI breaking (at least the context menu when some text is selected, and the search pane of the preferences).

::: browser/components/search/content/search.xml
@@ +474,5 @@
>        <handler event="focus">
>        <![CDATA[
>          // Speculatively connect to the current engine's search URI (and
>          // suggest URI, if different) to reduce request latency
> +        if (this.currentEngine.name) {

If currentEngine is null, this will throw an exception (like I assume the code did before the change), so I don't see how this check is fixing anything.

@@ +1092,5 @@
>  
>        <method name="updateHeader">
>          <body><![CDATA[
> +          let searchbar = document.getElementById("searchbar");
> +          if (searchbar.currentEngine && searchbar.currentEngine.iconURI) {

How is checking the searchbar better than using the search service? Is it to avoid triggering an initialization of the search service if it's not (successfully) initialized yet?
Attachment #8760294 - Flags: review?(florian)
The reason the icon is broke is because the search dropdown never finishes populating, so it doesn't properly remove the src attribute for the icon and the default icon is strange.

> Services.search.currentEngine should never been null (see bug 438599).

It can be null if there are no search engines at all which can happen if somehow search-plugins doesn't register.

> (at least the context menu when some text is selected, and the search pane of the preferences).

Search pane works fine. You are right that the context menu does some strange stuff. I'd love to fix that as well.

> If currentEngine is null, this will throw an exception (like I assume the code did before the change), so I don't see how this check is fixing anything.

currentEngine on the searchbar can never be null. See:

http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#140

> How is checking the searchbar better than using the search service? Is it to avoid triggering an initialization of 
the search service if it's not (successfully) initialized yet?

Because the searchbar is smart enough to handle a null engine from the search service. Again, see:

http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml#140
(In reply to Mike Kaply [:mkaply] from comment #3)

> the default icon is strange.

Is this something we can/should fix?

> > Services.search.currentEngine should never been null (see bug 438599).
> 
> It can be null if there are no search engines at all which can happen if
> somehow search-plugins doesn't register.

Would it make sense to fallback to a hardcoded chrome://browser/locale/searchplugins/ URL in that case?

> currentEngine on the searchbar can never be null. See:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/search/
> content/search.xml#140

Ah, I had never seen that getter.
> Would it make sense to fallback to a hardcoded chrome://browser/locale/searchplugins/ URL in that case?

I'd rather not because that would break the ability to remove the default plugins.

You are correct that other things break if there is a null currentengine. Worst is that searching in the URL bar crashes browser. I just thought it would be nice to at least make the searchbar code friendly to a null engine (since again, it was written to allow it to be that way).

I'll check on the icon.
(In reply to Mike Kaply [:mkaply] from comment #5)
> > Would it make sense to fallback to a hardcoded chrome://browser/locale/searchplugins/ URL in that case?
> 
> I'd rather not because that would break the ability to remove the default
> plugins.

Not really. It's possible to register resource://search-plugins pointing to a different folder. This is kind of a gray area, but letting people remove default plugins without replacing them seems more like a bug than like a feature.
Whiteboard: [partner search] [fxsearch]
> This is kind of a gray area, but letting people remove default plugins without replacing them seems more like a bug than like a feature.

They do replace them, either via the search service or distribution/searchplugins.
So it looks like the bad icon is a bug on Windows with higher than 100% DPI.

For 1.1 or higher DPI, we set the icon here:

http://searchfox.org/mozilla-central/source/browser/themes/windows/searchbar.css#81

  .searchbar-engine-image {
    list-style-image: url("chrome://mozapps/skin/places/defaultFavicon@2x.png");
  }

but for the searchbar-engine-image, we have this:


http://searchfox.org/mozilla-central/source/browser/themes/windows/searchbar.css#23

.searchbar-engine-image {
  height: 16px;
  width: 16px;
  list-style-image: url("chrome://global/skin/icons/folder-item.png");
  -moz-image-region: rect(0px, 16px, 16px, 0px);
}

So we're grabbing the top left of the defaultfavicon.

This begs the question "Why aren't we using chrome://mozapps/skin/places/defaultFavicon.png for the default icon"
This is separate from the other patch.

We should be using defaultFavicon consistent with the other platforms.
Attachment #8761274 - Flags: review?(florian)
Comment on attachment 8761274 [details] [diff] [review]
Use defaultFavicon for image

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

Thanks for debugging this icon issue :-).
Attachment #8761274 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/405bdbc110e1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: