Closed
Bug 862148
Opened 12 years ago
Closed 10 years ago
stop supporting installation of Sherlock plugins from the web
Categories
(Firefox :: Search, defect, P3)
Firefox
Search
Tracking
()
RESOLVED
FIXED
Firefox 44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: Gavin, Assigned: florian)
References
Details
(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [fxsearch][bugday-20151007])
Attachments
(6 files)
13.48 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
33.79 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
20.04 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
3.78 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
12.69 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
60.69 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Priority: -- → P3
Whiteboard: [fxsearch]
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
This removes support for Sherlock engines on Services.search.addEngine
Attachment #8663737 -
Flags: review?(adw)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8663740 -
Flags: review?(adw)
Assignee | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
Keywords: addon-compat
Updated•10 years ago
|
Attachment #8663735 -
Flags: review?(adw) → review+
Comment 7•10 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•10 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•10 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•10 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?
Comment 11•10 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?
Comment 12•10 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.
Attachment #8663735 -
Flags: review?(bugs)
Assignee | ||
Comment 13•10 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•10 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.
Attachment #8664202 -
Flags: review?(adw)
Assignee | ||
Updated•10 years ago
|
Attachment #8664202 -
Attachment is patch: true
Assignee | ||
Comment 15•10 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.
Attachment #8664203 -
Flags: review?(adw)
Assignee | ||
Comment 16•10 years ago
|
||
Updated•10 years ago
|
Attachment #8664202 -
Flags: review?(adw) → review+
Comment 17•10 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•10 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•10 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•10 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•10 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•10 years ago
|
||
Oops, no, I forgot that was in an earlier patch.
Comment 23•10 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
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Comment 25•10 years ago
|
||
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•10 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]
Reporter | ||
Comment 28•10 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!
Flags: needinfo?(gavin.sharp) → needinfo?(florian)
Assignee | ||
Comment 29•10 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.
Description
•