1664708 - [API Request] Extend windows API to open webpages in a browser

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[John Bieling (:TbSync)]

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?

Comment 1

[klaus]

1. 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.
2. the url bar cannot be edited, that feels wrong as well

Comment 2

[Mark Banner (:standard8)]

(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)

2. 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".

Comment 3

[klaus]

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

Comment 4

[John Bieling (:TbSync)]

(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.

Comment 5

[klaus]

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

[Mark Banner (:standard8)]

(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.

Comment 7

[John Bieling (:TbSync)]

I will get this going now. Let us settle on a name. Votes?

browser.windows.openExternally()
browser.windows.launchBrowser()

Comment 9

[John Bieling (:TbSync)]

Attachment: bug1664708.patch

Patch to add launchBrowser.

Comment 10

[John Bieling (:TbSync)]

Attachment: bug1664708.patch

Fixed spelling error in description.

Comment 11

[Magnus Melin [:mkmelin]]

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

Comment 12

[John Bieling (:TbSync)]

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

[Magnus Melin [:mkmelin]]

That was what I was suggesting. As an API user I'd expect a failure if what I was trying to do failed.

Comment 14

[John Bieling (:TbSync)]

Attachment: bug1664708.patch

Updated according to reviewers comments.

Comment 15

[John Bieling (:TbSync)]

Attachment: bug1664708.patch

Removed extra commit from patch.

Comment 16

[John Bieling (:TbSync)]

Attachment: bug1664708_Extend_windows_API_to_open_webpages_in_default_system_browser.patch

Finally decided on a name.

Comment 17

[John Bieling (:TbSync)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=67f898de403c7f24acdb8feec254cd1097c7c70c

Comment 18

[Geoff Lankow (:darktrojan)]

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

Comment 19

[John Bieling (:TbSync)]

Attachment: bug1664708_Extend_windows_API_to_open_webpages_in_default_system_browser.patch

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

[Geoff Lankow (:darktrojan)]

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");
}

Comment 21

[John Bieling (:TbSync)]

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

[Geoff Lankow (:darktrojan)]

I've never quite understood when to use assertThrows and when to use assertRejects, so fair enough.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3d2772720a88
Extend windows API to open webpages in default system browser. r=darktrojan

Comment 24

[John Bieling (:TbSync)]

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

[Wayne Mery (:wsmwk)]

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

[Dave Miller [:justdave]]

Thunderbird 84.0b3
https://hg.mozilla.org/releases/comm-beta/rev/6045fb572011

Comment 27

[Wayne Mery (:wsmwk)]

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

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/3c8f7193a7f8