:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Description • 11 years ago

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

	Kohei Yoshino [:kohei]
	Comment 1 • 11 years ago

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

	Florian Quèze [:florian] Assignee
	Comment 2 • 9 years ago

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

	Florian Quèze [:florian] Assignee
	Comment 3 • 9 years ago

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

	Florian Quèze [:florian] Assignee
	Comment 5 • 9 years ago

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

	Florian Quèze [:florian] Assignee
	Comment 6 • 9 years ago

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

	Drew Willcoxon :adw
	Comment 7 • 9 years ago

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?

	Drew Willcoxon :adw
	Comment 8 • 9 years ago

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!

	Drew Willcoxon :adw
	Comment 9 • 9 years ago

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

	Drew Willcoxon :adw
	Comment 10 • 9 years ago

(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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 9 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 9 years ago

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.

	Florian Quèze [:florian] Assignee
	Comment 13 • 9 years ago

(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 :).

	Florian Quèze [:florian] Assignee
	Comment 14 • 9 years ago

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.

	Florian Quèze [:florian] Assignee
	Comment 15 • 9 years ago

(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.

	Florian Quèze [:florian] Assignee
	Comment 16 • 9 years ago

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

	Drew Willcoxon :adw
	Comment 17 • 9 years ago

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?

	Florian Quèze [:florian] Assignee
	Comment 18 • 9 years ago

(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 :).

	Drew Willcoxon :adw
	Comment 19 • 9 years ago

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.)

	Florian Quèze [:florian] Assignee
	Comment 20 • 9 years ago

(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?

	Florian Quèze [:florian] Assignee
	Comment 21 • 9 years ago

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

	Drew Willcoxon :adw
	Comment 22 • 9 years ago

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

	Pulsebot
	Comment 23 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 9 years ago

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

	Kohei Yoshino [:kohei]
	Comment 25 • 9 years ago

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

	Saurabh Nair [:jsx] (not reading bugmail. ni? if you need me)
	Comment 26 • 9 years ago

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.

	alex_mayorga
	Comment 27 • 9 years ago

¡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!

	:Gavin Sharp [email: gavin@gavinsharp.com] Reporter
	Comment 28 • 9 years ago

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!

	Florian Quèze [:florian] Assignee
	Comment 29 • 9 years ago

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, ...)