Closed Bug 862147 Opened 7 years ago Closed 2 years ago

remove window.external.addSearchEngine

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59

People

(Reporter: Gavin, Assigned: florian)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 1 obsolete file)

It's not specced, and has very little use these days now that window.external.AddSearchProvider exists and is supported across browsers.

I'm not sure what the compatibility impact of this will be, if any.
Could we add some telemetry for this before removing?
Perhaps compare usage of addSeachProvider to usage of addSearchEngine. (I guess both are used very rarely)
We are using window.sidebar, and window.sidebar.addPanel for the 'install' into the user's bookmarks.  FYI, Internet Explorer and Opera also have sidebar support.  Not sure about Chrome.  Please do not remove window.sidebar!

We are looking at your Social API, but it would presumably mean having to have very different code for different browsers as they do not support it?  This would make development of our service much harder.
You *can* still utilize the Firefox sidebar by creating an extension:
https://developer.mozilla.org/en-US/docs/Creating_a_Firefox_sidebar

I should mention that (and Social API) in the site compatibility document, BTW.
Yes, but creating our own extension is more development for us, more maintenance to keep up with Firefox changes, and possibly harder to get customers to install.  I did actually read that document on Friday, and download the "empty sidebar project".  However, the "empty sidebar project" was not compatible with Firefox 23.0.  I tried overriding the max version and it still would not install.  I probably could have fixed it with some more effort, but presumably the "Creating a Firefox sidebar" document is also somewhat out-of-date and I would need to merge in changes to add-on requirements that have been made since that document was written.  It would also be nice not to need a restart.  It is not clear if this example requires a restart since it was written before that functionality was introduced.
The document might be obsolete... but for Firefox 4 and above, extensions are usually maintenance free. And for Internet Explorer, you have to develop an add-on anyway, right?
I'll update the doc and sample extension later today.
The Internet Explorer sidebar is a form of registry controlled ActiveX control that comes with IE.  We did not really have to develop an add-on.  We just ask users to run a .reg file to 'install' and a different one to 'uninstall'.  Not ideal perhaps, but they can save the .reg file and check it over before running. It is only a few lines. Ref: http://msdn.microsoft.com/en-us/library/aa753590%28v=vs.85%29.aspx

Thanks Kohei, that would be useful.
Here we go: https://github.com/kyoshino/simple-sidebar
It's *restartless* and compatible with Firefox 17+. I'll update the docs soon.
Thanks Kohei.  Looks good, except I don't see how to access it via the menu.  I see you have a menu-label, etc., but if I press Alt to show the menu, I still can't see it anywhere.  Also, I would like a way for the user to be able to open the sidebar easily without having to remember a hotkey, probably a toolbar button.  Should I basically follow the instructions in https://developer.mozilla.org/en-US/docs/Code_snippets/Toolbar and the two tutorials it links to?
The menu is available under View > Sidebar. And yes, a toolbar button would be helpful for convenient access to the sidebar. I'll update my sample extension.

By the way, I think the Social API is much easier to implement. Though I haven't looked into the API yet and the documentation is not ready, sites using this API don't have to be social services.
(In reply to Seb A from comment #2)
> FYI, Internet Explorer and Opera also have
> sidebar support.

At least Internet Explorer 11 dropped the support in IE 11 mode. Opera (Presto) died.

> Not sure about Chrome.

Chrome (Blink) doesn't support window.sidebar.
(In reply to Masatoshi Kimura [:emk] from comment #11)
> At least Internet Explorer 11 dropped the support in IE 11 mode. Opera

Even IE6 doesn't support window.sidebar. Even if we support the sidebar feature, we shouldn't expose it to content.
(In reply to Seb A from comment #2)
> We are using window.sidebar, and window.sidebar.addPanel

Also, windows.sidebar.addPanel was already removed.
(In reply to Masatoshi Kimura [:emk] from comment #12)
> (In reply to Masatoshi Kimura [:emk] from comment #11)
> > At least Internet Explorer 11 dropped the support in IE 11 mode. Opera
> 
> Even IE6 doesn't support window.sidebar. Even if we support the sidebar
> feature, we shouldn't expose it to content.

Sorry, but you are wrong. IE6 *does* support the equivalent to window.sidebar (http://msdn.microsoft.com/en-us/library/aa753590%28v=vs.85%29.aspx)!  So does IE7, IE8, IE9, IE10, and probably IE11 at least in Desktop mode (maybe not in Metro mode) although I haven't had a chance to test it yet. And why should we not expose it to content?
(In reply to Masatoshi Kimura [:emk] from comment #13)
> (In reply to Seb A from comment #2)
> > We are using window.sidebar, and window.sidebar.addPanel
> 
> Also, windows.sidebar.addPanel was already removed.

Yes, I know and it broke our installation procedure and I already wrote on that bug report when that version of Firefox was released.  However, that is not a reason to remove the feature entirely.
(In reply to Seb A from comment #14)
> IE6 *does* support the equivalent to
> window.sidebar
> (http://msdn.microsoft.com/en-us/library/aa753590%28v=vs.85%29.aspx)!

Is it exposed to Web pages? If not, I don't think it's equivalent to window.sidebar. Rather, it would be equivalent to the chrome-only sidebar API.
(In reply to Seb A from comment #15)
> (In reply to Masatoshi Kimura [:emk] from comment #13)
> Yes, I know and it broke our installation procedure and I already wrote on
> that bug report when that version of Firefox was released.

Bug number please? (I searched Bugzilla, but bug 686441 was the only bug you filed.)

> However, that is
> not a reason to remove the feature entirely.

Why? The only remaining function is addSearchEngine which has a standard alternative (window.external.AddSearchProvider).
(In reply to Masatoshi Kimura [:emk] from comment #16)
> (In reply to Seb A from comment #14)
> > IE6 *does* support the equivalent to
> > window.sidebar
> > (http://msdn.microsoft.com/en-us/library/aa753590%28v=vs.85%29.aspx)!
> 
> Is it exposed to Web pages? If not, I don't think it's equivalent to
> window.sidebar. Rather, it would be equivalent to the chrome-only sidebar
> API.

I'm not certain.  I didn't realise that Firefox window.sidebar was exposed to web pages.  Do you mean that abitrary web pages can modify the contents of the window.sidebar?
(In reply to Masatoshi Kimura [:emk] from comment #17)
> (In reply to Seb A from comment #15)
> > (In reply to Masatoshi Kimura [:emk] from comment #13)
> > Yes, I know and it broke our installation procedure and I already wrote on
> > that bug report when that version of Firefox was released.
> 
> Bug number please?

bug 691647.  Most of the conversation has been here though because I didn't fancy my chances on getting that reversed after it was released.

> > However, that is
> > not a reason to remove the feature entirely.
> 
> Why? The only remaining function is addSearchEngine which has a standard
> alternative (window.external.AddSearchProvider).

That's incorrect.  It is also available via the GUI (see http://www.syntec.co.uk/agentinstall/).  This is our current work-around for installing the sidebar for our users (after window.sidebar.addPanel was removed) so please don't remove that either!
(In reply to Kohei Yoshino [:kohei] from comment #10)
> The menu is available under View > Sidebar. And yes, a toolbar button would
> be helpful for convenient access to the sidebar. I'll update my sample
> extension.

Thanks, I had lost it.  And, thanks, that would be good.

> By the way, I think the Social API is much easier to implement. Though I
> haven't looked into the API yet and the documentation is not ready, sites
> using this API don't have to be social services.

The features look nice: however, the Social API is Firefox only.  This means that we would either have to ask all our clients to use Firefox (which we have been doing anyway, but sometimes you get resistance in large organizations), or we would need to fork our code completely depending on the browser.  The advantage of the sidebar approach is that: OK, the installation methods may be different for different browsers, but after that the same code runs in all the browsers as it is standard HTML and Javascript without non-standard API calls.
Summary: drop support for window.sidebar.addSearchEngine (and window.sidebar in general) → drop support for window.sidebar
Attached patch Patch (obsolete) — Splinter Review
I have a patch removing window.sidebar.addSearchEngine attached to bug 862148.

This patch removes what remains of window.sidebar.
Attachment #8663755 - Flags: review?(bugs)
Thanks, but no thanks flo. This is still a useful feature that we are actively using.
Do we have any data how often window.sidebar is used, or even just accessed? 
Remember, removing silly things like navigator.taintEnabled() may cause major breakage.
(navigator.taintEnabled() does nothing! bug 679971)

So, at least we need some telemetry here. Since bug 968923 we have now rather easy way to track
this kind of stuff.
IRC: froydnj: add a line in http://mxr.mozilla.org/mozilla-central/source/dom/base/UseCounters.conf#5 and a [UseCounter] annotation in the appropriate .webidl
Flags: needinfo?(florian)
Comment on attachment 8663755 [details] [diff] [review]
Patch

So, I'd rather review this after we have some telemetry.
window.sidebar is ancient stuff so I wouldn't be surprised if the usage is higher than we'd hope.
Attachment #8663755 - Flags: review?(bugs)
Flags: needinfo?(florian)
Attachment #8664204 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #23)
> Do we have any data how often window.sidebar is used, or even just accessed?

AFAIK we don't.

To be honest, I don't care too much about removing window.sidebar (or even window.sidebar.addSearchEngine), what I really care about (and would like to land soon) is the removal of the sherlock code from the search service. I attached a new patch in bug 862148 that keeps the addSearchEngine method and makes it error when it's used with a sherlock engine.
Comment on attachment 8664204 [details] [diff] [review]
Instrument usage of window.sidebar.addSearchEngine

Just because UseCounter is a new thing, I'd like nfroyd to take a look.
(but looks good)
Attachment #8664204 - Flags: review?(bugs) → review?(nfroyd)
Comment on attachment 8664204 [details] [diff] [review]
Instrument usage of window.sidebar.addSearchEngine

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

LGTM, thanks.
Attachment #8664204 - Flags: review?(nfroyd) → review+
One of the reasons for proposing removing window.sidebar was because it wasn't specced. Why not just spec it and announce it. Maybe then people will love the 'new' feature? I don't see the point in removing it when it works fine and people are using it. We recommend to all our customers that they use Firefox because it has this feature.
Why do you insist window.sidebar while window.sidebar.addPanel was removed long before? How can you actively use the removed feature? Are you even testing your code?
Because you can still create sidebars via ticking the box when you bookmark something. That's what we have asked customers to do since the removal of window.sidebar.addPanel and it works fine. So we have no code. (Well apart from the stuff the runs in the sidebar.)
(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8664204 [details] [diff] [review]
> Instrument usage of window.sidebar.addSearchEngine
> 
> Just because UseCounter is a new thing, I'd like nfroyd to take a look.
> (but looks good)

The WebIDL push hook on fx-team prevented me from pushing the patch with only r=froydnj in the commit message. I added you in the list of reviewers to push the patch, I assumed this was fine given you said the patch looks good.

Should Nathan be added in the list of accepted reviewers in the hook?
Flags: needinfo?(bugs)
Keywords: leave-open
r=me for the change. Nathan probably doesn't want to do too many .webidl reviews in general ;)
Flags: needinfo?(bugs)
addSearchEngine still works for OpenSearch plugin.
Um, I misread the patch in Bug 862148? I thought window.external.AddSearchProvider was the winner as documented:

https://www.fxsitecompat.com/en-US/docs/2015/sherlock-search-plugin-is-no-longer-supported/
(In reply to Kohei Yoshino [:kohei] from comment #41)
> Um, I misread the patch in Bug 862148? I thought
> window.external.AddSearchProvider was the winner as documented:
> 
> https://www.fxsitecompat.com/en-US/docs/2015/sherlock-search-plugin-is-no-
> longer-supported/

window.external.AddSearchProvider is the winner. But window.sidebar.addSearchEngine wasn't removed immediately, we only removed support for installing sherlock plugins. We added telemetry probes to know how much window.sidebar is used, so that we can make an informed decision the next time we think about removing it.
(In reply to Kohei Yoshino [:kohei] from comment #41)
> Um, I misread the patch in Bug 862148? I thought
> window.external.AddSearchProvider was the winner as documented:
> 
> https://www.fxsitecompat.com/en-US/docs/2015/sherlock-search-plugin-is-no-
> longer-supported/

addSearchEngine will be removed in the future, but
> The addPanel and addSearchEngine methods have been removed with Firefox 23 and 44 respectively
is clearly wrong. Firefox 44 still supports addSearchEngine as an alias of AddSearchProvider.
Thanks for clarification! Corrected the site compat docs.
(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8663755 [details] [diff] [review]
> Patch
> 
> So, I'd rather review this after we have some telemetry.
> window.sidebar is ancient stuff so I wouldn't be surprised if the usage is
> higher than we'd hope.

Can you make sense of the collected usage count from telemetry and decide if we should go ahead or wontfix this bug? I tried to look at the telemetry data today, but didn't really gained much understanding from it.
Flags: needinfo?(bugs)
Not sure why there is _PAGE and _DOCUMENT, but both look very high % for window.sidebar.
~1.4% and ~4%. Way too high to remove without some more investigation.

addSearchEngine doesn't seem to have any telemetry data.
Flags: needinfo?(bugs)
Telemetry data shows that window.sidebar is accessed by about 1.62% of page loads, that's too high to remove.

window.external.addSearchEngine has been touched 105 times out of 13,651,075,134 page loads on Firefox 57 beta. Let's remove it.
Summary: drop support for window.sidebar → remove window.external.addSearchEngine
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8663755 - Attachment is obsolete: true
Attachment #8939892 - Flags: review?(bugs) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95c9d645e6e3
remove window.external.addSearchEngine, r=smaug.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I have documented this by adding a note to the Window.sidebar page, and a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/docs/Web/API/Window/sidebar
https://developer.mozilla.org/en-US/Firefox/Releases/59#APIs_2

Let me know if this look OK. Thanks!
Flags: needinfo?(florian)
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #52)
> I have documented this [...] Let me know if this look OK. Thanks!

Looks good to me, thanks!
Flags: needinfo?(florian)
See Also: → 1503551
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.