Last Comment Bug 862148 - stop supporting installation of Sherlock plugins from the web
: stop supporting installation of Sherlock plugins from the web
Status: RESOLVED FIXED
[fxsearch][bugday-20151007]
: addon-compat, dev-doc-complete, site-compat
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 44
Assigned To: Florian Quèze [:florian] [:flo]
:
Mentors:
Depends on: 862147
Blocks: 862137
  Show dependency treegraph
 
Reported: 2013-04-15 17:34 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2015-10-09 04:02 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - remove window.sidebar.addSearchEngine (13.48 KB, patch)
2015-09-21 10:01 PDT, Florian Quèze [:florian] [:flo]
adw: review+
Details | Diff | Review
Part 2 - stop installing Sherlock engines (33.79 KB, patch)
2015-09-21 10:02 PDT, Florian Quèze [:florian] [:flo]
adw: review+
Details | Diff | Review
part 3 - remove Sherlock parsing code (20.04 KB, patch)
2015-09-21 10:03 PDT, Florian Quèze [:florian] [:flo]
adw: review+
Details | Diff | Review
Part 4 - remove the _parseAsOpenSearch method (3.78 KB, patch)
2015-09-21 10:05 PDT, Florian Quèze [:florian] [:flo]
adw: review+
Details | Diff | Review
Part 1, v2 - drop support for Sherlock plugins in window.sidebar.addSearchEngine (12.69 KB, patch)
2015-09-22 06:34 PDT, Florian Quèze [:florian] [:flo]
adw: review+
Details | Diff | Review
Part 5 - remove the DATA_* and TYPE_* constants (60.69 KB, patch)
2015-09-22 06:36 PDT, Florian Quèze [:florian] [:flo]
adw: review+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-15 17:34:06 PDT
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] 2013-06-29 01:03:03 PDT
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] [:flo] 2015-09-21 10:01:26 PDT
Created attachment 8663735 [details] [diff] [review]
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] [:flo] 2015-09-21 10:02:53 PDT
Created attachment 8663737 [details] [diff] [review]
Part 2 - stop installing Sherlock engines

This removes support for Sherlock engines on Services.search.addEngine
Comment 4 Florian Quèze [:florian] [:flo] 2015-09-21 10:03:42 PDT
Created attachment 8663740 [details] [diff] [review]
part 3 - remove Sherlock parsing code
Comment 5 Florian Quèze [:florian] [:flo] 2015-09-21 10:05:44 PDT
Created attachment 8663742 [details] [diff] [review]
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] [:flo] 2015-09-21 10:06:44 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6f6c328ef8
Comment 7 Drew Willcoxon :adw 2015-09-21 11:22:36 PDT
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 2015-09-21 11:23:38 PDT
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 2015-09-21 11:24:24 PDT
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 2015-09-21 11:26:11 PDT
(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] (high review load, please consider other reviewers) 2015-09-21 12:09:47 PDT
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] (high review load, please consider other reviewers) 2015-09-21 12:21:53 PDT
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] [:flo] 2015-09-21 12:30:29 PDT
(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] [:flo] 2015-09-22 06:34:10 PDT
Created attachment 8664202 [details] [diff] [review]
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] [:flo] 2015-09-22 06:36:43 PDT
Created attachment 8664203 [details] [diff] [review]
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] [:flo] 2015-09-22 06:41:54 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bbccd4f5186
Comment 17 Drew Willcoxon :adw 2015-09-22 10:43:07 PDT
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] [:flo] 2015-09-22 10:54:05 PDT
(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 2015-09-22 11:03:40 PDT
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] [:flo] 2015-09-22 11:32:06 PDT
(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] [:flo] 2015-09-22 11:37:34 PDT
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 2015-09-22 11:50:36 PDT
Oops, no, I forgot that was in an earlier patch.
Comment 25 Kohei Yoshino [:kohei] 2015-09-23 20:04:35 PDT
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 all bugmail. ni? if you need me) 2015-10-07 12:19:28 PDT
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 2015-10-08 09:11:09 PDT
¡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: gavin@gavinsharp.com] 2015-10-08 10:07:51 PDT
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] [:flo] 2015-10-09 04:02:42 PDT
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, ...)

Note You need to log in before you can comment on or make changes to this bug.