1481052 - Implement FileLink WebExtensions API

Status: RESOLVED FIXED

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

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

It would be nice if we can allow FileLink add-ons to be WebExtensions. The API is pretty plain and simple:

manifest.json:

>   "cloudfile": {
>     "name": "Sekrit File Transfer",
>     "service_url": "https://example.com",
>     "new_account_url": "https://example.com/signup",
>     "settings_url": "/content/settings.html",
>     "management_url": "/content/management.html"
>   },

API:

> browser.cloudfile.onUploadFile.addListener((fileInfo, abortSignal) => {
>   // fileInfo = {
>   //   id: 123,
>   //   name: "teh file.txt",
>   //   data: new ArrayBuffer(...)
>   // }
> 
>   return { url: "https://example.com/" + fileInfo.id };
> });
> 
> browser.cloudfile.onDeleteFile.addListener((fileId) => {
>   // ...
> });
> 
> 
> browser.cloudfile.setQuota({
>   uploadSizeLimit: -1,
>   spaceRemaining: -1,
>   spaceUsed: -1
> });
> 
> await browser.cloudfile.getQuota({ uploadSizeLimit: 123123 });


settings_url and management_url will be moz-extension:// pages that are shown in a (content) iframe. I'm not quite sure yet how to do any of the communication the current iframe does, there doesn't seem to be a comparable precedent in the browser. I guess we could use postMessage().

The following changes should be done on the filelink code:

* A possibility to register/unregister filelink providers without xpcom
* Move the learn more link outside of the iframe
* Make iframe settings/management form validation result affect the main xul dialog's disabled states
* Adjust the extraArgs code to not apply to WebExtensions (they'll use their own storage mechanism)
* Make sure clicking on links in the iframe will open them in the browser, unless they are self moz-extension links.



Geoff, any comments on the API, or required changes?

Comment 1

[Geoff Lankow (:darktrojan)]

I not very familiar with FileLink or how it works, but this seems good. I presume abortSignal is a callback to cancel the upload if necessary? And that returning from the event will resolve a promise in our code (I've not seen that before).

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

abortSignal is https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal and can be passed directly to fetch(), which is convenient. The calling code can then just call signal.abort() and the fetch call will abort. I just need to expose it via Cu.importGlobalProperties.

browser.cloudfile.onUploadFile.addListener() should be able to return a promise, so you could also do:

browser.cloudfile.onUploadFile.addListener(async ({ id, name, data } , abortSignal) => {
  let resp = await fetch("http://example.com/myservice, { body: data, signal: abortSignal });
  let rdata = await resp.json();
  return { url: rdata.url }
});

The returning in comment 0 will just return the value, but I think if you await a non-promise you just get the value. I'll make sure the code accepts both. The returning in this comment will return a promise resolving with the url object, given the function is async.

Comment 3

[Geoff Lankow (:darktrojan)]

Ah, I remembered I meant to say: if you use File instead of ArrayBuffer, you can specify the file name on the object instead of doing it separately. You might want an ArrayBuffer though, so just an idea.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Some relevant discussion:

<&Fallen> darktrojan: different thing, I'm looking into the wx cloudfile api and still don't have a smart idea how the wx iframe for example in the add cloudfile account dialog should tell its parent that the form is valid. Right now the parent reaches into the iframe and checks form validity, but from the perspective of a WebExtension that could be black magic. Any ideas?
1:45 PM <&Fallen> The current WebExtension APIs seem to just use global APIs to set status, but that doesn't seem right to me from a separation of concerns standpoint
1:46 PM <&Fallen> The only other option I see is using postMessage and documenting it, but then having the parent check form validity doesn't seem so bad
1:47 PM <%darktrojan> point me to some code, Fallen ?
1:49 PM <&Fallen> https://searchfox.org/comm-central/source/mail/components/cloudfile/content/addAccountDialog.js#143 and line 284
1:50 PM <%darktrojan> thanks
2:00 PM <%darktrojan> and not a single filelink extension is compatible with 60
2:01 PM <&Fallen> The WeTransfer one will be :) It even works as a WebExtension.
2:01 PM <&Fallen> And box and hightail are built in
2:02 PM <%darktrojan> I only see box and it doesn't have a form
2:07 PM <&Fallen> darktrojan: https://searchfox.org/comm-central/source/mail/components/cloudfile/content/Hightail/settings.xhtml#19
2:10 PM <%darktrojan> I wonder if that dialog should be reworked
2:11 PM <%darktrojan> I mean, if you choose Box, you get a link which says "Get a Box account" that opens a browser, and a button that says "Set up account", which opens another dialog with a webpage
2:11 PM <%darktrojan> that's confusing to begin with
2:13 PM <%darktrojan> also an API shouldn't be dependent on how our UI works
2:15 PM <&Fallen> Whatever it looks like, there needs to be something at account setup time that allows the provider to determine what settings and prerequisites are required to set up the account. Are you suggesting limiting what the UI can look like by providing just a few field options like the former options ui for legacy add-ons?
2:16 PM <&Fallen> The OAuth dialog won't fit into the new cloudfile provider account iframe, I guess that is another reason why it is separate.
2:19 PM <%darktrojan> what if we consider a provider "incomplete" until the webextension tells us otherwise, and just use the main window UI for everything except picking a provider?
2:19 PM <%darktrojan> or even for picking a provider somehow
2:20 PM <%darktrojan> you already use the preferences tab for a modify settings iframe, no?
2:29 PM <%darktrojan> fyi this link is a 404 https://searchfox.org/comm-central/source/mail/components/cloudfile/content/Hightail/settings.xhtml#23


I could imagine an enable() / disable() method that the extension could call once all information is provided, and it would be disabled by default. Then we could indeed use the currbent prefs dialog iframe for both setup and management. We'd still need a "add account" type button under the list of cloudfile providers, or a [ + ] [ - ] button set, which has a dropdown with the list of providers that can be added.

Is this what you had in mind? This is a lot more than I expected and it might be difficult to justify backporting this to tb60. I do think it would be cleaner though and allow for less modal dialogs.

One caveat, with a single global enable/disable, we are setting in stone that there will only be one cloudfile account per extension. If you for example have a Filelink for WebDAV provider, then you could only ever use one WebDAV share unless you duplicate the add-on with a different id.

Comment 5

[Magnus Melin [:mkmelin]]

That seems unfortunate (to limit it to one).

Comment 6

[Geoff Lankow (:darktrojan)]

That's more or less what I had in mind. You could do multiple accounts per extension by passing around some sort of identifier. I guess how you do it depends on whether account information is stored by Thunderbird or by the extension.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ok, different approach. Also renamed onUploadFile -> onFileUploaded because it seems more consistent with naming that Firefox uses.

manifest.json:

>   "cloudfile": {
>     "name": "Sekrit File Transfer",
>     "service_url": "https://example.com",
>     "new_account_url": "https://example.com/signup",
>     "config_url": "/content/config.html"
>   },

API:

> browser.cloudfile.onFileUploaded.addListener((account, fileInfo, abortSignal) => {
>   // account is a cloudfile account object
> 
>   // fileInfo = {
>   //   id: 123,
>   //   name: "teh file.txt",
>   //   data: new ArrayBuffer(...)
>   // }
> 
>   return { url: "https://example.com/" + fileInfo.id };
> });
> 
> browser.cloudfile.onFileDeleted.addListener((account, fileId) => {
>   // ...
> });
> 
> browser.cloudfile.onAccountAdded.addListener(async (account) => {
>   // TODO we could also consider adding manifest defaults for the upload size limit
>   account.uploadSizeLimit = 123;
>   await browser.cloudfile.update(account);
> });
> 
> browser.cloudfile.onAccountDeleted.addListener((account) => {
>   // ...
> });
> 
> // cloudfile account object:
> {
>   id: 123,
>   configured: true, // set true if ready to use
> 
>   // default to manifest entry if provided
>   name: "Sekrit File Transfer", // TODO could make this readonly?
>   icons: { 32: "/icon32.png" }
> 
>   // quotas. Check if spaceRemaining/spaceUsed is really necessary.
>   uploadSizeLimit: -1,
>   spaceRemaining: -1,
>   spaceUsed: -1,
> 
>   // default to manifest entry if provided.
>   // Maybe double check if we really need a service and new account url
>   // with the new layout or if that can be part of the html page the 
>   // provider controls.
>   serviceUrl: "https://...", 
>   newAccountUrl: "https://...",
>   configUrl: "/content/config.html"
> }
> 
> (async function() {
>   // These will not return accounts for other extensions
>   var account123 = await browser.cloudfile.get(123123);
> 
>   var accounts = await browser.cloudfile.getAll();
> 
>   account123.uploadSizeLimit = 123;
>   await browser.cloudfile.update(account123);
> })();


There is no create/delete method for accounts, because I'd like to avoid providers creating their own accounts in the background without user action. I could be convinced to add a delete method.

I'd appreciate feedback on the TODO parts above, and of course general feedback if this is a good approach.

Comment 8

[Geoff Lankow (:darktrojan)]

Where are account credentials stored in this scenario? I think the best place for them is in Thunderbird's password manager, although we'd have to be sure that we only hand them out to the right extension.


I imagine a UI something like this:

On the left, the list of accounts as we do currently, followed by [ New Foo Account… ][ New Bar Account… ] buttons.
Clicking on an existing account loads config.html?id=123123 on the right, which can load what it needs from await browser.cloudfile.get(123123)
Clicking on a new account button creates a new unconfigured account 456456 and loads config.html?id=456456. (Could be a different page, I guess, but config.html could work out what to do from the "configured" field on the account object.)

I don't think there's a need for service_url or new_account_url, or onAccountAdded/onAccountDeleted events. If Thunderbird's storing account credentials all the extension needs to do is communicate with the service's APIs.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Geoff Lankow (:darktrojan) from comment #8)
> Where are account credentials stored in this scenario? I think the best
> place for them is in Thunderbird's password manager, although we'd have to
> be sure that we only hand them out to the right extension.

I think this should be up to the WebExtension, they can use the logins API if it ever lands (bug 1324919). For now the password would likely be in browser.storage.


> 
> 
> I imagine a UI something like this:
> 
> On the left, the list of accounts as we do currently, followed by [ New Foo
> Account… ][ New Bar Account… ] buttons.
> Clicking on an existing account loads config.html?id=123123 on the right,
> which can load what it needs from await browser.cloudfile.get(123123)
> Clicking on a new account button creates a new unconfigured account 456456
> and loads config.html?id=456456. (Could be a different page, I guess, but
> config.html could work out what to do from the "configured" field on the
> account object.)
Yes, this is also what I had in mind.

> 
> I don't think there's a need for service_url or new_account_url, or
> onAccountAdded/onAccountDeleted events. If Thunderbird's storing account
> credentials all the extension needs to do is communicate with the service's
> APIs.
I can see service_url/new_account_url going away, but we should have an event when the account is added/removed. This would allow extensions to do some initial default config, or clean up storage if they need to.

Looks like we are pretty much in consensus here, let me know if you have final thoughts on the account events.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-api-1.diff

This is mostly your code Philipp, so it probably should have another reviewer, but let's see what you think first.

Comment 11

[Geoff Lankow (:darktrojan)]

By the way, I think this namespace should be cloudFile, and the manifest entry cloud_file. For consistency.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9023511 [details] [diff] [review]
1481052-cloudfile-api-1.diff

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

My original patch has some other code I am missing, e.g. making sure the iframe used in the settings is a content iframe. Did I write that after I sent you the code, or did it go missing? I'll upload my latest patch for reference, feel free to merge things together.

This is a great start, but I think we are not quite there yet.

::: mail/components/extensions/parent/ext-cloudfile.js
@@ +117,5 @@
> +
> +  cancelFileUpload(file) {
> +    this.emit("uploadAborted", {
> +      id: this._fileIds.get(file.path),
> +    });

Not quite sure why you moved the abort logic into a new event handler and having the client deal with the AbortController? I thought this was pretty elegant and saved us from another event handler.

@@ +163,5 @@
> +    let contractID = "@mozilla.org/mail/cloudfile;1?type=" + this.extension.id.replace(/@/g, "-");
> +    let self = this;
> +
> +    // unregisterFactory does not clear the contract id from Components.classes, therefore re-use
> +    // the class id from the unregistered factory

Registering via xpcom is fickle. I have a patch that adds code to the cloudfile provider so we can dynamically register providers, without XPCOM. Will upload in a sec.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Dynamically register cloudfile (defunct) - v1

This patch was meant to allow both the xpcom way and the new way, but I think in the end we should probably just get rid of the xpcom way and require add-ons to update.

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Philipp's latest patch, for reference only - v1

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9023511 [details] [diff] [review]
1481052-cloudfile-api-1.diff

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

::: mail/components/extensions/parent/ext-cloudfile.js
@@ +21,5 @@
> +    reader.readAsArrayBuffer(blob);
> +  });
> +}
> +
> +class CloudFileProvider extends EventEmitter {

add 
@implements {nsIMsgCloudFileProvider}

@@ +27,5 @@
> +    super();
> +
> +    this.extension = extension;
> +    this.accountKey = false;
> +    this.lastError = "";

don't see this used anywhere

::: mail/components/extensions/test/xpcshell/test_ext_cloudfile.js
@@ +30,5 @@
> +  Assert.ok(Cc[contract].createInstance(Ci.nsIMsgCloudFileProvider));
> +  await extension.unload();
> +  Assert.throws(
> +    () => Cc[contract].createInstance(Ci.nsIMsgCloudFileProvider),
> +    /NS_ERROR_XPC_CI_RETURNED_FAILURE/

?

Comment 16

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #12)
> Not quite sure why you moved the abort logic into a new event handler and
> having the client deal with the AbortController? I thought this was pretty
> elegant and saved us from another event handler.

It would be elegant but it doesn't work. The extension receives a copy of the object, so calling abort on the original does nothing. It might be different if we had a real AbortController, but we can't do that, so I don't know.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

The mock AbortController needs to go, that is true. Last time I tried to add Abort{Controller,Signal} to Cu.importGlobalProperties, it seemed to work without. Do we really not have the real AbortController available? I think there were options to ensure that some parameters are not copied but transferred raw.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Geoff, for the record, I've started to update the patch I referenced to the API from comment 7. It seems this can be done without the UI changes for now. ping me if you want my current state.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9023595 [details] [diff] [review]
Dynamically register cloudfile (defunct) - v1

And this patch is totally wrong, it registers instances not classes. Probably needs a rethinking.

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-api-2.diff

Here's what I currently have. It's capable of multiple accounts per provider, but just wired to one for now.

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-register-1.diff

This is the current non-XPCOM registration patch. I've added observer service notifications so the UI can be fixed.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9024647 [details] [diff] [review]
1481052-cloudfile-api-2.diff

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

::: mail/components/extensions/parent/ext-cloudfile.js
@@ +28,5 @@
> +    super();
> +
> +    this.extension = extension;
> +    this.accountKey = false;
> +    this.lastError = "";

unused?

@@ +104,5 @@
> +      }
> +      return;
> +    }
> +
> +    if (results && results.length) {

prefer comaping to numbers, i.e. length > 0

@@ +146,5 @@
> +    } catch (ex) {
> +      callback.onStopRequest(null, null, Cr.NS_ERROR_FAILURE);
> +    }
> +
> +    if (results && results.length) {

> 0

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9024655 [details] [diff] [review]
1481052-cloudfile-register-1.diff

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

::: mail/components/cloudfile/cloudFileAccounts.js
@@ +80,5 @@
> +   * Register a cloudfile provider, e.g. from a bootstrapped add-on. Registering can be done in two
> +   * ways, either implicitly through using the "cloud-files" XPCOM category, or explicitly using
> +   * this function.
> +   *
> +   * @param {Object} aImplementation        The nsIMsgCloudFileProvider implementation to register

@param {nsIMsgCloudFileProvider}

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Philipp's new API patch - v1

I was looking into this as well, which I probably should not have without chatting with you. Here is a patch that pretty much brings it in line with the API in comment 7, applies on top of attachment 9023596 [details] [diff] [review]. Maybe there is something useful you can salvage from it.

I also looked into how the AbortController thing could work, see the big comment in the patch.

I'll stop now, let me know if you need my feedback.

Comment 25

[Geoff Lankow (:darktrojan)]

Okay, there's some good stuff in there. I don't understand why you're so keen on passing abort signals around. What's wrong with having an ordinary event and letting the extension deal with it?

I'll work this into what I've got and then hopefully we're good to go. People are starting to get impatient.

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I guess part of it is wanting to figure out how to make it work. The other part is that from experience writing WebExtensions, I always found it annoying when I manually have to keep a Map to handle API calls and responses that are somewhat related. A while ago openerTabId was not implemented, so when I needed that property I had to manually remember what the opener was to get this property, implementing most of the tab event handlers.

The other beauty is that when using AbortSignal, it can directly be passed to fetch(), which most if not all FileLink providers will be using.

Anyway, I am ok with not using AbortSignal until I figure out how it could work. Just keep in mind that at some point we may hav to deal with backwards compatibility if we are making API changes. You could also consider checking if we can pass a Promise instead of an AbortSignal for now, so you could do:

onFileUpload.addListener(async (acc, info, abortPromise) => {
  let controller = new AbortController();
  abortPromise.catch(() => controller.abort());

  await fetch({ signal: controller.signal, ... });
});

This would also save having to keep a map of AbortControllers and likely won't require using a child script. Or, maybe even just ignore the cancel event for now and add it later. What do you think?

Comment 27

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-api-3.diff

Some things I haven't mentioned yet:

* Dynamically registered providers aren't true XPCOM objects, so they will QI to nsIMsgCloudFileProvider, but not instanceof it.
* On a cancelled upload the extension can return { aborted: true } or throw the error that fetch throws when using abortSignal.
* uploadCanceled is still spelled wrong.

Outstanding things that I can remember right now:

* The settings and management pages don't have access to "browser". I've tried to fix this for the preferences tab, but to do so it needs to be loaded in a chrome browser, which causes other things to break.
* Many of the things in the schema could do with a description so the generated documentation is better. (Also the documentation generator could be better.)
* My comment 11.

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9026345 [details] [diff] [review]
1481052-cloudfile-api-3.diff

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

I'm fine with the renaming you mentioned in comment 11. Please also make sure to file followups for the remaining items as necessary, and a followup to de-xpcom cloudfile accounts completely. 

r+ with these issues considered:

::: mail/components/cloudfile/cloudFileAccounts.js
@@ +86,5 @@
> +   * this function.
> +   *
> +   * @param {nsIMsgCloudFileProvider} The implementation to register
> +   */
> +  registerProvider(aImplementation) {

I think we should clarify what aImplementation means and maybe rename the variable. Right now it really seems like this means instance, not the class implementation.

That aside, and I know this code comes from me in part, should we be registering an instance or a class? If we in the future support more than one account per provider, then a class may make more sense? Not sure how this would fit in with the current wx implementation though.

::: mail/components/cloudfile/content/addAccountDialog.js
@@ +16,5 @@
>    QueryInterface: ChromeUtils.generateQI([Ci.nsIRequestObserver]),
>    onStartRequest(aRequest, aContext) {},
>    onStopRequest(aRequest, aContext, aStatusCode) {
>      if (aStatusCode == Cr.NS_OK
> +        && aContext.QueryInterface(Ci.nsIMsgCloudFileProvider)) {

If aContext does not QI to nsIMsgCloudFileProvider, this will throw.

Re your comment, I always thought that once it QIs it will also be instanceof. I guess that only worked with nsIClassInfo filled out, which m-c is breaking anyway. For Lightning we have https://searchfox.org/comm-central/source/calendar/base/modules/calUtils.jsm#363

@@ +66,5 @@
>      this.removeTitleMenuItem();
>  
> +    // Hook up the default "Learn More..." link to the appropriate link. We do
> +    // this even before adding account types because emptySettings.xhtml will
> +    // also mainly this.

Maybe you can fix my botched sentence here. emptySettings.xhtml is the main consumer of this, iirc.

::: mail/components/extensions/parent/ext-cloudfile.js
@@ +256,5 @@
> +          context,
> +          name: "cloudfile.onFileDeleted",
> +          register: fire => {
> +            let listener = (event, { id }) => {
> +              let account = convertAccount(self.provider);

m-c caches the converted objects, can you file a followup to do this as well?

@@ +305,5 @@
> +            };
> +          },
> +        }).api(),
> +
> +        async getAccount(accountId) {

Other WX apis only use .get() .getAll() .update(). Any reason you changed this to add Account? Are you anticipating getting other things in the cloud file API?

Comment 29

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #28)
> ::: mail/components/cloudfile/cloudFileAccounts.js
> @@ +86,5 @@
> > +   * this function.
> > +   *
> > +   * @param {nsIMsgCloudFileProvider} The implementation to register
> > +   */
> > +  registerProvider(aImplementation) {
> 
> I think we should clarify what aImplementation means and maybe rename the
> variable. Right now it really seems like this means instance, not the class
> implementation.

I've renamed it to aProvider.

> That aside, and I know this code comes from me in part, should we be
> registering an instance or a class? If we in the future support more than
> one account per provider, then a class may make more sense? Not sure how
> this would fit in with the current wx implementation though.

Not totally sure what you're asking here, but in the interests of not changing too much (since we want to take these changes all the way through to ESR), let's stick with what we've got. Once extensions are using the WebExt API, we can change whatever we like underneath it.

> ::: mail/components/cloudfile/content/addAccountDialog.js
> @@ +16,5 @@
> >    QueryInterface: ChromeUtils.generateQI([Ci.nsIRequestObserver]),
> >    onStartRequest(aRequest, aContext) {},
> >    onStopRequest(aRequest, aContext, aStatusCode) {
> >      if (aStatusCode == Cr.NS_OK
> > +        && aContext.QueryInterface(Ci.nsIMsgCloudFileProvider)) {
> 
> If aContext does not QI to nsIMsgCloudFileProvider, this will throw.
> 
> Re your comment, I always thought that once it QIs it will also be
> instanceof. I guess that only worked with nsIClassInfo filled out, which m-c
> is breaking anyway. For Lightning we have
> https://searchfox.org/comm-central/source/calendar/base/modules/calUtils.
> jsm#363

I think that might've been the case, but you're probably right about class info.

> @@ +66,5 @@
> >      this.removeTitleMenuItem();
> >  
> > +    // Hook up the default "Learn More..." link to the appropriate link. We do
> > +    // this even before adding account types because emptySettings.xhtml will
> > +    // also mainly this.
> 
> Maybe you can fix my botched sentence here. emptySettings.xhtml is the main
> consumer of this, iirc.

I can't actually work out what you're trying to say!

> ::: mail/components/extensions/parent/ext-cloudfile.js
> @@ +256,5 @@
> > +          context,
> > +          name: "cloudfile.onFileDeleted",
> > +          register: fire => {
> > +            let listener = (event, { id }) => {
> > +              let account = convertAccount(self.provider);
> 
> m-c caches the converted objects, can you file a followup to do this as well?

I don't think it's necessary, convertAccount just assigns some variables into a new object. It's probably as fast as a Map lookup or something like it, and compared to the WebExt overhead is probably negligible.

> @@ +305,5 @@
> > +            };
> > +          },
> > +        }).api(),
> > +
> > +        async getAccount(accountId) {
> 
> Other WX apis only use .get() .getAll() .update(). Any reason you changed
> this to add Account? Are you anticipating getting other things in the cloud
> file API?

browser.windows.getAll() gets all windows, browser.tabs.update() updates a tab, browser.cookies.get() gets a cookie, but browser.cloudFile.get() doesn't get a cloudFile, it gets a cloudFile account. That we called the events onAccountAdded and onAccountDeleted instead of onAdded and onDeleted implies we should name the methods getAccount, getAllAccounts and updateAccount.

Comment 30

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-api-4.diff

Comment 31

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-api-interdiff.diff

This is an interdiff between the last two patches. To prevent blowing interdiff's little mind, I undid the renaming of some files (cloudfile back to cloudFile).

It's not very interesting, but I did change the test to use a second file for the upload abort, as it was causing problems "uploading" the same file twice.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8709d04aee92
FileLink WebExtensions API; r=Fallen

Comment 33

[Jorg K (CEST = GMT+2)]

For the record: This seems to have caused:
TEST-UNEXPECTED-FAIL | [snip]/mozmill/cloudfile/test-cloudfile-add-account-dialog.js | test-cloudfile-add-account-dialog.js::test_accept_enabled_on_form_validation

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/06599026d496
Disable failing part of test-cloudfile-add-account-dialog.js. rs=bustage-fix

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/70a161f6a58b
Work around test that causes Mozmill to die a horrible death; rs=bustage-fix

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 9027742 [details] [diff] [review]
1481052-cloudfile-api-4.diff

I guess we want this in the next beta so it can go into TB 60.4 ESR.

Comment 37

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/4088f33deefaba669e3faae2bbd57f71fcca860b
https://hg.mozilla.org/releases/comm-beta/rev/9398d440a92d2e9a4f9126cd7287abb0ba5397ff
https://hg.mozilla.org/releases/comm-beta/rev/5e00c85f4d23b0187b26dca3c33439b6a1c60b89

Comment 38

[Geoff Lankow (:darktrojan)]

I've rebased this for ESR, just waiting for TryServer to tell me I did something wrong.

Comment 39

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-esr-1.diff

TryServer says go! https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=10c5e56d050c1c6290407f9e67aad060a52c61ed

Comment 40

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Note EventManager invocation was changed between ESR60 and now:

https://hg.mozilla.org/releases/mozilla-esr60/file/tip//browser/components/extensions/ext-tabs.js#l138

Comment 41

[Geoff Lankow (:darktrojan)]

It was. That would've failed on Try if I'd actually added the test to the manifests. :/

Comment 42

[Geoff Lankow (:darktrojan)]

Attachment: 1481052-cloudfile-esr-2.diff

Comment 43

[Jorg K (CEST = GMT+2)]

Comment on attachment 9029539 [details] [diff] [review]
1481052-cloudfile-esr-2.diff

OK, we'll take it for TB 60.4.

Comment 44

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/076be5ca12a94eaf19618618977ed1f5f2101be4

Comment 45

[John Crisp]

Is there any idea on timescale for releasing this?

We heavily depend on filelink/DL and cannot upgrade without it working.

We are aware of the security implications on not upgrading, but we can't do it without this, and no amount of brow beating will change that. We cannot lose the functionality as we use it extensively to transfer lots of large files which our business depends on.

It puts us in an invidious and dangerous position, and we really don't understand why these hooks were never implemented by the Mozilla team prior to trying to force everyone to upgrade. There were some breakages we could live without, but not this.

We fully appreciate the work of the developer who has added patches for this, and congratulate him on his persistence.

Comment 46

[Geoff Lankow (:darktrojan)]

Upgrade to what, John? The old FileLink functionality is still there in all current versions of Thunderbird.

Comment 47

[wavexx]

AFAIK, not a single one of the old FileLink addons is working today. This is because several related APIs got broken in TB60, and there's no way to rebuild without the legacy SDK:

https://bugzilla.mozilla.org/show_bug.cgi?id=1493528

if you have additional information on that front, I would really appreciate it.

Comment 48

[Magnus Melin [:mkmelin]]

Pretty sure the one we ship (box.com) is still working.

Comment 49

[John Crisp]

(In reply to Magnus Melin [:mkmelin] from comment #48)
> Pretty sure the one we ship (box.com) is still working.

It's absolutely no use if you can't use it for legal reasons (and we can't)


(In reply to Geoff Lankow (:darktrojan) from comment #46)
> Upgrade to what, John? The old FileLink functionality is still there in all
> current versions of Thunderbird.

This gives a succint summary of the issue.

https://www.mail-archive.com/[e-mail]/msg00459.html

Enough said.

So the question remains as to when will 60.4 be released?

We are stuck with our current version until we can use an upgraded plugin.

Comment 50

[Magnus Melin [:mkmelin]]

We hope to get 60.4 released soon, probably within the next 2 weeks.
The api won't of course magically make any add-ons work again.

Comment 51

[John Crisp]

(In reply to Magnus Melin [:mkmelin] from comment #50)
> We hope to get 60.4 released soon, probably within the next 2 weeks.

OK thanks

> The api won't of course magically make any add-ons work again.

Of course not. But without the code no one can update their add-ons can they?

Comment 52

[Magnus Melin [:mkmelin]]

Sure they can update without it too. The old api is still available for 60.

Comment 53

[John Crisp]

(In reply to Magnus Melin [:mkmelin] from comment #52)
> Sure they can update without it too. The old api is still available for 60.

Err - I am no dev but as far as I understand it add-on devs are expected to use the webextensions API, and this bug is for file links via web extensions, which until 60.4 (now) is missing. So until this is released they can't properly build an updated add on as per Mozilla recommendations. Or am I missing something?

I'll see what happens on this and https://bugzilla.mozilla.org/show_bug.cgi?id=1493528 and hope that something comes out of the woodwork. (we've rewritten one add-on we use for webextensions, but the filelink is a bit beyond us currently)

Comment 54

[Jorg K (CEST = GMT+2)]

You can get a preview of TB 60.4.0 for Windows here:
https://queue.taskcluster.net/v1/task/Bo0ta66JQkWWFikH-rXBuA/runs/0/artifacts/public/build/target.zip
https://queue.taskcluster.net/v1/task/Bo0ta66JQkWWFikH-rXBuA/runs/0/artifacts/public/build/install/sea/target.installer.exe

Upload to box.com still works, I tested that. I doubt that all the file like add-ons are broken, here are a few that claim to be compatible with TB 60:
https://addons.thunderbird.net/en-GB/thunderbird/search/?q=filelink&appver=&platform=
NextCloud:
https://addons.thunderbird.net/en-GB/thunderbird/addon/nextcloud-filelink/?src=search
and 
Mega:
https://addons.thunderbird.net/en-GB/thunderbird/addon/megabird/?src=search
should work.

https://www.mail-archive.com/[e-mail]/msg00459.html is deliberately misleading. *All* "legacy" add-ons that have been made compatible with TB 60 still work. There is information on what needed to be done:
https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57

Comment 55

[Magnus Melin [:mkmelin]]

Yes, you're missing the point that all the old providers were add-ons using the (old) internal api, which is still available. The add-on may need other adjustments of course. (In 60 we still support old style add-ons.)

Comment 56

[John Crisp]

(In reply to Jorg K (GMT+1) from comment #54)
> You can get a preview of TB 60.4.0 for Windows here:

Don't use Windows anywhere. Haven't for a decade or more..... :-)

> Upload to box.com still works, I tested that. I doubt that all the file like

As per my previous, I can't upload files to a 3rd party service. Yes, that may be a bit of shock, but that is the way of the world. We only upload them to our own server only (GDPR & all that jazz). Hence DL is very good for us as we control what is going on, where, and when.

> 
> https://www.mail-archive.com/[e-mail]/msg00459.html is
> deliberately misleading. *All* "legacy" add-ons that have been made
> compatible with TB 60 still work. There is information on what needed to be
> done:
> https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57

I'll leave you to argue that point with the add-on developer as per his #c47 (I think that is the the developer) and his bug https://bugzilla.mozilla.org/show_bug.cgi?id=1493528

All I know is when we upgraded the add-on failed to work, and it appeared to be for the reasons he describes. The only solution for us was to downgrade.

Comment 57

[wavexx]

Well, I wanted to fix my old add-on to support TB60. It's true that the old API is still there, but the add-on broken in TB60 due to *other* API changes. That's the reason most old add-ons broke as well. I don't think I said anything misleading there. At the time of writing, *no* other filelink addon was supported on TB60.

I simply wanted to fix the legacy one, but as I wrote, the tools to do so seems to be missing in TB60. I'm lost here.

If you can point me now to some info to rebuild the xpi for TB60 I would be glad, so I can restore the functionality while waiting for the new webext interface to be available.

Comment 58

[Jorg K (CEST = GMT+2)]

John, you can have TB 60.4.0 preview for any platform you want.

Wavexx, as per bug 1493528 comment #10, your add-on can be made to work in TB 60.

Comment 59

[John Crisp]

(In reply to Jorg K (GMT+1) from comment #58)
> John, you can have TB 60.4.0 preview for any platform you want.
> 

OK thank you

> Wavexx, as per bug 1493528 comment #10, your add-on can be made to work in
> TB 60.

I'll comment on the other bug - I'd be happy to get my PFY involved as he knows a bit more than me but with Christmas et al he won't be available for the next 3+ weeks

Comment 60

[wavexx]

So I'm giving a stab at updating the addon to the webext API.

In the "settings" page which is used to setup an account, I don't see a way to perform validation before the account is added.

The onAccountAdded event is emitted too late.

Form validation seems to be somewhat working as required fields seem to prevent submission, but I don't want to perform account validation at each onchange event. Hooking up to onsubmit doesn't work, and returning false doesn't prevent submission.

Ideas?

Comment 61

[Geoff Lankow (:darktrojan)]

The simple answer, and not really what you want, is that the settings page is about to be removed completely. In hindsight we shouldn't have even put it in the API, but we didn't realise that at the time.

What you should do instead, is put all your configuration in the "management" page, and set the "configured" flag on the account when you're ready. Thunderbird 60 doesn't currently do anything with the flag, but I will change that in an upcoming release.

I'm going to update the docs now, as they don't reflect what I've just said.

Comment 62

[wavexx]

The "Box" provider though does prevent submission until authentication is provided (cannot say what happens next, as I don't have a box account).

I guess I shouldn't even attempt this if the page is going to be removed? In this case, should I just put an empty page?

When will the single-instance limit be lifted? I have several cases where I'd like to select which server I want to use.

Comment 63

[Geoff Lankow (:darktrojan)]

I guess I shouldn't even attempt this if the page is going to be removed? In this case, should I just put an empty page?

That's what I've been doing. For up-to-date Thunderbird versions you don't even need to do that.

When will the single-instance limit be lifted? I have several cases where I'd like to select which server I want to use.

Thunderbird 68. You could test in a beta version already. Technically it should be possible in TB60, but the UI is horribly broken, so don't even try it.

Comment 64

[Diego Casorran [:diegocr]]

This new API is really limited.

While it's very handy just providing the whole file loaded into memory as an ArrayBuffer, this is at least causing two issues:

1. Providers will be unable to let users upload files of GBs...
2. Some providers (such as MEGA) does need the file modification time.

Also, the way the file is loaded into memory is bogus:

+++ b/mail/components/extensions/parent/ext-cloudFile.js
@@ -0,0 +1,353 @@
+...
+async function promiseFileRead(nsifile) {
+  let blob = await File.createFromNsIFile(nsifile);
+
+  return new Promise((resolve, reject) => {
+    let reader = new FileReader();
+    reader.addEventListener("loadend", () => {
+      resolve(reader.result);
+    });
+    reader.addEventListener("onerror", reject);

I.e. any error loading the file won't be handled properly.

So, I would strongly advise to improve this further, e.g. by providing a File instance to the Filelink extension...

Comment 65

[John Crisp]

Regrettably this won't fix the DL FileLink extension as per previous comments here and elsewhere.

https://addons.thunderbird.net/en-US/thunderbird/addon/dl-for-thunderbird/

Still stuck on 52.9.x and will be until we decide to change email program (and I can see why many have dumped things like Thunderbird and gone with web mail etc - none of this nonsense) or I can get someone to update the extension. I have offered money but even that doesn't seem an incentive.

Sometimes I despair.