862148 - stop supporting installation of Sherlock plugins from the web

Status: RESOLVED FIXED

(Firefox :: Search, defect, P3)

Description

[:Gavin Sharp [email: [e-mail]]]

This involves a couple of things:
- dropping support for the window.sidebar.* APIs (these are not specced in the HTML spec, bug 862147)
- drop support in the search service for parsing/loading sherlock plugins

Comment 1

[Kohei Yoshino [:kohei]]

Added this to the compatibility doc as an advance notice:
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_24

Comment 2

[Florian Quèze [:florian]]

Attachment: Part 1 - remove window.sidebar.addSearchEngine

Requesting review from smaug for the dom/webidl/External.webidl and from Drew for the rest of the patch.

Comment 3

[Florian Quèze [:florian]]

Attachment: Part 2 - stop installing Sherlock engines

This removes support for Sherlock engines on Services.search.addEngine

Comment 4

[Florian Quèze [:florian]]

Attachment: part 3 - remove Sherlock parsing code

Comment 5

[Florian Quèze [:florian]]

Attachment: Part 4 - remove the _parseAsOpenSearch method

Now that _parseAsSherlock is gone, the _parseAsOpenSearch method that just forwards makes even less sense, we should consolidate all the _parse methods.

Comment 6

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6f6c328ef8

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8663737 [details] [diff] [review]
Part 2 - stop installing Sherlock engines

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

::: netwerk/base/nsIBrowserSearchService.idl
@@ +118,5 @@
>    /**
>     * Supported search engine types.
>     */
>    const unsigned long TYPE_MOZSEARCH     = 1;
> +  const unsigned long TYPE_SHERLOCK      = 2; // deprecated

"Obsolete" or "unsupported" is more accurate since we're removing support entirely, right?  Actually shouldn't we just remove these consts altogether?

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8663740 [details] [diff] [review]
part 3 - remove Sherlock parsing code

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

Nice to remove all this!

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8663742 [details] [diff] [review]
Part 4 - remove the _parseAsOpenSearch method

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

::: toolkit/components/search/nsSearchService.js
@@ -2190,5 @@
>            this._iconUpdateURL = child.textContent;
>            break;
>          case "ExtensionID":
>            this._extensionID = child.textContent;
> -          breakk;

:-O

Comment 10

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #7)
> "Obsolete" or "unsupported" is more accurate since we're removing support
> entirely, right?  Actually shouldn't we just remove these consts altogether?

By "these consts" I meant the ones you're marking as deprecated, but maybe we could remove the remaining TYPE_* and DATA_* consts (one of each) too?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Same comment here as for the window.sidebar removal
https://bugzilla.mozilla.org/show_bug.cgi?id=862147#c23
Any data how often this stuff is being used?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8663735 [details] [diff] [review]
Part 1 - remove window.sidebar.addSearchEngine

So, I'd rather review this after we have some telemetry.
.addSearchEngine is ancient stuff so I wouldn't be surprised if the usage is higher than we'd hope.

Comment 13

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #7)
> Comment on attachment 8663737 [details] [diff] [review]
> Part 2 - stop installing Sherlock engines
> 
> Review of attachment 8663737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsIBrowserSearchService.idl
> @@ +118,5 @@
> >    /**
> >     * Supported search engine types.
> >     */
> >    const unsigned long TYPE_MOZSEARCH     = 1;
> > +  const unsigned long TYPE_SHERLOCK      = 2; // deprecated
> 
> "Obsolete" or "unsupported" is more accurate since we're removing support
> entirely, right?

Right, I'll change that to 'Obsolete'.

> Actually shouldn't we just remove these consts altogether?

I had initially removed the aDataType parameter completely, and then had some second thoughts about add-on compatibility. But thinking about this some more, removing the consts shouldn't actually break add-ons (I checked on mxr), it will just return some undefined values. I'll cleanup some more :).

Comment 14

[Florian Quèze [:florian]]

Attachment: Part 1, v2 - drop support for Sherlock plugins in window.sidebar.addSearchEngine

This keeps window.sidebar.addSearchEngine, and just makes the Sherlock part of it display an error message in the console. This should avoid issues with old browser-detection scripts.

Comment 15

[Florian Quèze [:florian]]

Attachment: Part 5 - remove the DATA_* and TYPE_* constants

(In reply to Drew Willcoxon :adw from comment #10)

> By "these consts" I meant the ones you're marking as deprecated, but maybe
> we could remove the remaining TYPE_* and DATA_* consts (one of each) too?

Yes. The change was large enough that I made it as a separate patch for easier review.

Comment 16

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bbccd4f5186

Comment 17

[Drew Willcoxon :adw]

Comment on attachment 8664203 [details] [diff] [review]
Part 5 - remove the DATA_* and TYPE_* constants

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

Er, actually I didn't notice that there are two remaining TYPE_ consts, MOZSEARCH and OPENSEARCH.  Embarrassing question: what's the difference between the two?  This new patch removes both.  Is that really OK?

Also, nsIBrowserSearchService.addEngine now has a useless second parameter, right?  Ideally if we're removing the DATA consts we're removing that parameter too.  Or if we're worried about add-on compatibility, do you want to mark addEngine as deprecated and introduce a new method without that param?

Comment 18

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #17)

> Er, actually I didn't notice that there are two remaining TYPE_ consts,
> MOZSEARCH and OPENSEARCH.  Embarrassing question: what's the difference
> between the two?

OpenSearch is a standard to define search plugins. It's the kind of files we expect to find on the web.

MozSearch is our Mozilla-specific non-standard version. We serialize engines into this format when installing them into the user's profile. We expect to find such files mostly in the user's profile, but I don't think we would reject a MozSearch plugin found on the web.

These 2 formats are close enough that we use the same parsing code for both. They are actually so close that Gavin filed a bug a while ago to make us serialize to OpenSearch in the profile instead of MozSearch (bug 862550).

> This new patch removes both.  Is that really OK?

Yesterday while working on this bug I thought I should file a new bug to remove the differences between the two, but today when working on this patch, I noticed (unless I missed something) that there's nothing we do with the type information, except for storing it. It doesn't seem to be used anywhere.

> Also, nsIBrowserSearchService.addEngine now has a useless second parameter,
> right?

Yes, I kept it for add-on compatibility.

> do you want
> to mark addEngine as deprecated and introduce a new method without that
> param?

I absolutely don't want to do that, it would add more junk that we wouldn't be able to cleanup soon. I think we can live with the null parameter. Someday I may want to cleanup the API by making all parameters but the first optional, and making the second parameter an object that can contain the other values. But that's offtopic for this bug :).

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 8664203 [details] [diff] [review]
Part 5 - remove the DATA_* and TYPE_* constants

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

OK, thanks for explaining.  In that case please correct addEngine's dataType comment.  I would say something about dataType no longer being used and callers should pass in 0.  (0 is better than null IMO since the param's type is long.  But I don't mean that you should modify your patch to replace all your nulls with zeroes.)

Comment 20

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #19)
> Comment on attachment 8664203 [details] [diff] [review]
> Part 5 - remove the DATA_* and TYPE_* constants
> 
> Review of attachment 8664203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, thanks for explaining.  In that case please correct addEngine's dataType
> comment.

The comment currently says "Obsolete, the value is ignored." (I replaced 'Deprecated' with 'Obsolete' after reading your previous review comments; I haven't attached that updated patch for a single word changed). Is that not correct?

Comment 21

[Florian Quèze [:florian]]

Looks like I forgot to remove from the idl file:
  /**
   * The search engine type.
   */
  readonly attribute long type;

and (in the addEngine method):
@throws NS_ERROR_FAILURE if the type is invalid

Comment 22

[Drew Willcoxon :adw]

Oops, no, I forgot that was in an earlier patch.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/923c13d8a0cf
https://hg.mozilla.org/integration/fx-team/rev/ce4065567b88
https://hg.mozilla.org/integration/fx-team/rev/fe5773db8478
https://hg.mozilla.org/integration/fx-team/rev/e5a6f8ea2ff4
https://hg.mozilla.org/integration/fx-team/rev/b2fd64120dab

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/923c13d8a0cf
https://hg.mozilla.org/mozilla-central/rev/ce4065567b88
https://hg.mozilla.org/mozilla-central/rev/fe5773db8478
https://hg.mozilla.org/mozilla-central/rev/e5a6f8ea2ff4
https://hg.mozilla.org/mozilla-central/rev/b2fd64120dab

Comment 25

[Kohei Yoshino [:kohei]]

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/sherlock-search-plugin-is-no-longer-supported/

Comment 26

[Saurabh Nair [:jsx] (not reading bugmail. ni? if you need me)]

Mentioned drop of support for window.sidebar.addSearchEngine at:
https://developer.mozilla.org/en-US/docs/Adding_search_engines_from_web_pages
https://developer.mozilla.org/en-US/Firefox/Releases/44

Will mark dev-doc-complete when I have permission to do that.

Comment 27

[alex_mayorga]

¡Hola Gavin!

How is this to be verified?

I'm struggling to even find traces of Sherlock plugins on the web to test out.

¡Gracias!

Comment 28

[:Gavin Sharp [email: [e-mail]]]

There are still some Sherlock plugins available on MyCroft. Looks for the ones with the Apple icon (rather than the "A9" icon), e.g. on this page:

http://mycroftproject.com/search-engines.html?category=29

Florian is a better contact for asking about verification details, though!

Comment 29

[Florian Quèze [:florian]]

What Gavin said is good. The only thing I have to add is that when verifying, checking that Sherlock plugins aren't supported anymore isn't as important as checking that we didn't break anything else (eg. installing regular open search plugins, ...)