[API Request] Extend windows API to open webpages in a browser
Categories
(Thunderbird :: Add-Ons: Extensions API, enhancement)
Tracking
(thunderbird_esr78 fixed, thunderbird84 fixed)
People
(Reporter: TbSync, Assigned: TbSync)
References
Details
(Whiteboard: webextension-api-request)
Attachments
(1 file, 5 obsolete files)
5.19 KB,
patch
|
darktrojan
:
review+
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
Consider the following code in a background script:
messenger.windows.create({
url:"http://www.google.de",
type: "normal",
});
This will create an entire new mail3pane window with the url loaded in a second tab. This feels wrong.
Is this the intended behavior?
Are we able to get rid of the additional mail3pane tab?
Should we change the API to open the URL in the default browser, if type normal is used?
- if opening in a tab is meant to happen, there should be a way to open in a normal browser as well. IMHO, we would want both.
- the url bar cannot be edited, that feels wrong as well
Comment 2•4 years ago
|
||
(In reply to John Bieling (:TbSync) from comment #0)
This will create an entire new mail3pane window with the url loaded in a second tab. This feels wrong.
Is this the intended behavior?
Are we able to get rid of the additional mail3pane tab?
This is a limitation of the current window/tab system - somewhere around there's a bug about not being able to open a new main window without a 3-pane tab. Unfortunately to fix that has always been a lot of work.
Should we change the API to open the URL in the default browser, if type normal is used?
I think if we haven't got an API to open in the default browser we should get one, but I don't think that we should change this API to do that. An add-on or something might want to be able to open in a new window in future.
(In reply to klaus from comment #1)
- the url bar cannot be edited, that feels wrong as well
Thunderbird never intended to allow general browsing - to do so would mean that we would have to include all the security indications and probably quite few other things from Firefox... The main case for having web pages in Thunderbird was for the what's new page etc - i.e. sites we'd be reasonably confident of being "safe".
to open in a tab or a external browser window: we can do that from legacy easily.
Why don't we tie an api function into the old window handling functions? I need to look at my code, but for external browser, I think there even is a dedicated function for that (and that would also solve Mark's security concern because the url is then under the care of the external browser, virus app, etc.)
Assignee | ||
Comment 4•4 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #2)
I think if we haven't got an API to open in the default browser we should get one, but I don't think that we should change this API to do that. An add-on or something might want to be able to open in a new window in future.
Is the windows API the entirely wrong place? For example defining a new type "external" or "browser" for windows.create()? Otherwise it would end up being a very simple API as I do not see much other things related to it, which could be part of that API.
as a matter of fact, if we want this, it is as easy as this:
openLinkExternally(url) {
let uri = url;
if (!(uri instanceof Ci.nsIURI)) {
uri = Services.io.newURI(url);
}
Cc["@mozilla.org/uriloader/external-protocol-service;1"]
.getService(Ci.nsIExternalProtocolService)
.loadURI(uri);
},
I got this from searchfox, I think, but don't quite remember.
Comment 6•4 years ago
|
||
(In reply to John Bieling (:TbSync) from comment #4)
(In reply to Mark Banner (:standard8) from comment #2)
I think if we haven't got an API to open in the default browser we should get one, but I don't think that we should change this API to do that. An add-on or something might want to be able to open in a new window in future.
Is the windows API the entirely wrong place?
I think I was referring to browser.windows.create()
specifically.
For example defining a new type "external" or "browser" for windows.create()?
I don't think we should re-use that, since the return value is a Window
object that we're not going to be able to supply. Overloading the one API with slightly different behaviour is likely to be strange.
Otherwise it would end up being a very simple API as I do not see much other things related to it, which could be part of that API.
Klaus' browser.windows.openLinkExternally()
suggestion or maybe a slightly shorter browser.windows.openExternally()
would be clearer to me as to what it is doing.
Assignee | ||
Comment 7•4 years ago
|
||
I will get this going now. Let us settle on a name. Votes?
browser.windows.openExternally()
browser.windows.launchBrowser()
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
Patch to add launchBrowser.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 10•4 years ago
|
||
Fixed spelling error in description.
Comment 11•4 years ago
|
||
Comment on attachment 9184724 [details] [diff] [review] bug1664708.patch Review of attachment 9184724 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/extensions/parent/ext-windows.js @@ +340,5 @@ > + uri = Services.io.newURI(url); > + } catch (e) { > + Cu.reportError(e); > + return; > + } Would it make sense to propagate an error instead, if the url is bad. @@ +345,5 @@ > + > + let extProtocolSvc = Cc[ > + "@mozilla.org/uriloader/external-protocol-service;1" > + ].getService(Ci.nsIExternalProtocolService); > + if (extProtocolSvc) { I think we can rely on this service being available
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
Would it make sense to propagate an error instead, if the url is bad.
I currently catch and log the error. Do you mean it would be better to re-throw?
Comment 13•4 years ago
|
||
That was what I was suggesting. As an API user I'd expect a failure if what I was trying to do failed.
Assignee | ||
Comment 14•4 years ago
|
||
Updated according to reviewers comments.
Assignee | ||
Comment 15•4 years ago
|
||
Removed extra commit from patch.
Assignee | ||
Comment 16•4 years ago
|
||
Finally decided on a name.
Assignee | ||
Comment 17•3 years ago
|
||
Comment 18•3 years ago
|
||
Comment on attachment 9189131 [details] [diff] [review] bug1664708_Extend_windows_API_to_open_webpages_in_default_system_browser.patch Review of attachment 9189131 [details] [diff] [review]: ----------------------------------------------------------------- Yes, okay, but this needs a test. To test this you replace the built-in external protocol service with your own. Here's an example you can use for inspiration, although there's no need for it to be a mochitest: https://searchfox.org/comm-central/source/mail/base/test/browser/browser_webSearchTelemetry.js ::: mail/components/extensions/parent/ext-windows.js @@ +341,5 @@ > + } catch (e) { > + throw new ExtensionError(`Url "${url}" seems to be malformed.`); > + } > + if ( > + uri.scheme.toLowerCase() != "http" && uri.schemeIs("http") etc. ::: mail/components/extensions/schemas/windows.json @@ +438,5 @@ > + { > + "name": "openDefaultBrowser", > + "type": "function", > + "description": "Opens the provided URL in the default system browser.", > + "async": "false", Just don't specify async if it's not. (In fact I think putting quotes around the false makes it async, in a weird way. Strings mean something different than booleans here.)
Assignee | ||
Comment 19•3 years ago
|
||
While writing the test I realized that WebExtensions fail to catch errors thrown by non-async WebExtension API functions. A try..catch in backgrounds.js is just not going into the catch. The test environment is actually returning a Promise when a non-async functions throws, resulting in an uncaught promise, which is what made me wonder.
Standard8 found this: "Since extensions can run in a child process, any API function that is implemented (either partially or completely) in the parent process must be asynchronous."
(https://firefox-source-docs.mozilla.org/toolkit/components/extensions/webextensions/functions.html#declaring-a-function-in-the-api-schema)
I therefore made the function async. Test works as expected.
Comment 20•3 years ago
|
||
Comment on attachment 9189699 [details] [diff] [review] bug1664708_Extend_windows_API_to_open_webpages_in_default_system_browser.patch Review of attachment 9189699 [details] [diff] [review]: ----------------------------------------------------------------- This is fine but I have a (incomplete, unchecked) suggestion for the test. I'll let you decide whether to take it or leave it. ::: mail/components/extensions/test/browser/browser_ext_windows.js @@ +60,5 @@ > + expected, > + `Checking result for browser.windows.openDefaultBrowser(${url})` > + ); > + } > + browser.test.sendMessage("ready", urls); I think you've over-complicated this. No need to get too fancy for a test. try { await browser.windows.openDefaultBrowser("http://www.google.de/"); await browser.windows.openDefaultBrowser("https://www.google.de/"); browser.test.assertThrows( () => browser.windows.openDefaultBrowser("ftp://www.google.de/"), /Url scheme "ftp" is not supported./ ); browser.test.notifyPass("passed"); } catch (ex) { browser.test.notifyFail("unexpected exception"); }
Assignee | ||
Comment 21•3 years ago
|
||
I played around with assertThrows
but it seems it does not catch the error (but assertRejects
does). My test code may be a bit old school, but it does work, so I keep it for now to get this of my head. I will get used to the other methods eventually.
Comment 22•3 years ago
|
||
I've never quite understood when to use assertThrows
and when to use assertRejects
, so fair enough.
Comment 23•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/3d2772720a88
Extend windows API to open webpages in default system browser. r=darktrojan
Assignee | ||
Comment 24•3 years ago
|
||
Comment on attachment 9189699 [details] [diff] [review]
bug1664708_Extend_windows_API_to_open_webpages_in_default_system_browser.patch
[Approval Request Comment]
Regression caused by (bug #):
Testing completed (on c-c, etc.):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=67f898de403c7f24acdb8feec254cd1097c7c70c
User impact if declined:
WebExtensions cannot use the new features on ESR
Risk to taking this patch (and alternatives if risky):
I hope none. This adds a new function and does not change any existing logic.
Comment 25•3 years ago
|
||
Comment on attachment 9189699 [details] [diff] [review]
bug1664708_Extend_windows_API_to_open_webpages_in_default_system_browser.patch
[Triage Comment]
Approved for beta
Comment 26•3 years ago
|
||
bugherder uplift |
Thunderbird 84.0b3
https://hg.mozilla.org/releases/comm-beta/rev/6045fb572011
Comment 27•3 years ago
|
||
Comment on attachment 9189699 [details] [diff] [review]
bug1664708_Extend_windows_API_to_open_webpages_in_default_system_browser.patch
[Triage Comment]
Approved for esr78
Comment 28•3 years ago
|
||
bugherder uplift |
Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/3c8f7193a7f8
Description
•