stop supporting installation of Sherlock plugins from the web

RESOLVED FIXED in Firefox 44

Status

()

Firefox
Search
P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Gavin, Assigned: florian)

Tracking

(Depends on: 1 bug, {addon-compat, dev-doc-complete, site-compat})

unspecified
Firefox 44
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [fxsearch][bugday-20151007])

Attachments

(6 attachments)

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
Added this to the compatibility doc as an advance notice:
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_24
Keywords: dev-doc-needed, site-compat
(Assignee)

Updated

2 years ago
Priority: -- → P3
Whiteboard: [fxsearch]
(Assignee)

Comment 2

2 years ago
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.
Assignee: nobody → florian
Attachment #8663735 - Flags: review?(bugs)
Attachment #8663735 - Flags: review?(adw)
(Assignee)

Comment 3

2 years ago
Created attachment 8663737 [details] [diff] [review]
Part 2 - stop installing Sherlock engines

This removes support for Sherlock engines on Services.search.addEngine
Attachment #8663737 - Flags: review?(adw)
(Assignee)

Comment 4

2 years ago
Created attachment 8663740 [details] [diff] [review]
part 3 - remove Sherlock parsing code
Attachment #8663740 - Flags: review?(adw)
(Assignee)

Comment 5

2 years ago
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.
Attachment #8663742 - Flags: review?(adw)
(Assignee)

Comment 6

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

Updated

2 years ago
Keywords: addon-compat

Updated

2 years ago
Attachment #8663735 - Flags: review?(adw) → review+

Comment 7

2 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?
Attachment #8663737 - Flags: review?(adw) → review+

Comment 8

2 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!
Attachment #8663740 - Flags: review?(adw) → review+

Comment 9

2 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
Attachment #8663742 - Flags: review?(adw) → review+

Comment 10

2 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?
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 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.
Attachment #8663735 - Flags: review?(bugs)
(Assignee)

Comment 13

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

Comment 14

2 years ago
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.
Attachment #8664202 - Flags: review?(adw)
(Assignee)

Updated

2 years ago
Attachment #8664202 - Attachment is patch: true
(Assignee)

Comment 15

2 years ago
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.
Attachment #8664203 - Flags: review?(adw)
(Assignee)

Comment 16

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

Updated

2 years ago
Attachment #8664202 - Flags: review?(adw) → review+

Comment 17

2 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?
(Assignee)

Comment 18

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

Comment 19

2 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.)
Attachment #8664203 - Flags: review?(adw) → review+
(Assignee)

Comment 20

2 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?
(Assignee)

Comment 21

2 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

Comment 22

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

Comment 23

2 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
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/sherlock-search-plugin-is-no-longer-supported/
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.
Keywords: dev-doc-needed → dev-doc-complete

Comment 27

2 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!
Flags: needinfo?(gavin.sharp)
Whiteboard: [fxsearch] → [fxsearch][bugday-20151007]
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!
Flags: needinfo?(gavin.sharp) → needinfo?(florian)
(Assignee)

Comment 29

2 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, ...)
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.