Make FileLink work in Instantbird

NEW
Unassigned

Status

defect
5 years ago
4 years ago

People

(Reporter: sawrubh, Unassigned)

Tracking

(Blocks 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

8.03 KB, patch
clokep
: feedback+
Details | Diff | Splinter Review
180.94 KB, patch
clokep
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
This bug is for tracking the work done to get FileLink support in Instantbird. This will be used as a fallback when we lack protocol support (or implementation). This will consist of two patches.

a) Reuse FileLink code, by either hacking the build stuff or copying the code. I'll start by copying the code and in the meanwhile take a look at how to get it done by hacking the build stuff.
b) Use the FileLink code to implement a drag and drop UI.
Duplicate of this bug: 955501
(Reporter)

Comment 2

5 years ago
Posted patch Patch1(v1) (obsolete) — Splinter Review
This patch basically "moves" cloudfile to chat/ and does the changes required for building it.

Next version : Possibly include such 'moving' changes from the Patch2 to this patch for clarity.
(Reporter)

Comment 3

5 years ago
Posted patch Patch2(v1) (obsolete) — Splinter Review
Note : This patch is just for a quick glance. There are a lot of improvements I have in mind which are listed below. Please suggest some more if I haven't mentioned them already.
This patch :
*moves a few things to get cloudfile working (need to move such changes to Patch1)
*adds ondragover, ondragenter and ondrop handlers to implement the drag and drop UI
*calls the addAccountDialog if you're not already signed into some FileLink Provider, if you're then it uploads to one of them (currently hardcoded)

What works : Uploading to Box, Hightail works without any flaws (in a very crude form though)

Improvements :
* Move the changes pertaining to 'getting cloudfile to build' to Patch 1
* Move the methods (ondrop, checkDrag) I've added to another file for clarity
* Figure out a UX to inform the user when the upload is complete (a popup or progress bar or the FX Download Panel) and give them the option to include the link in their message when they want to, instead of randomly sending the message as soon as it's uploaded
* Add more error handling in the switch case statement in onStopRequest
* Fix indentation and organization of code
Comment on attachment 8429369 [details] [diff] [review]
Patch2(v1)

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

This looks like it's on the right track, but it needs a lot more work!

Try to ensure that the changes that just move cloudfile stuff is done in the same patch (e.g. the logos).

I gave some feedback below, if some of it doesn't make sense anymore because you've moved / changed something, that's fine. We'll deal with it in the next version. Thanks!

::: chat/cloudfile/cloudFileAccounts.js
@@ +273,5 @@
>      else
>        Services.logins.addLogin(newLoginInfo);
>    },
>  
> +  fixIterator: function(aEnum, aIface) {

This entire fixIterator function is probably not necessary. We should see what parts of it are ACTUALLY necessary and inline that.

::: chat/cloudfile/content/addAccountDialog.js
@@ +195,5 @@
>    },
>  
>    accountTypeSelected: function AAD_accountTypeSelected() {
> +    if (!this._accountType)
> +      this._accountType = document.getElementById("accountType");

Is this change necessary? Why?

::: chat/cloudfile/content/dropFile.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This is an Instantbird specific file, so it should be under im/content.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource:///modules/cloudFileAccounts.js");
> +
> +function uploadListener(aFile, aCloudProvider)

Is this used or the inlined one inside of conversation.xml? :-S

::: chat/cloudfile/jar.mn
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  messenger.jar:
> +% content messenger %content/messenger/

I'm unsure if this should go here or in the chat jar.mn or in the im/ one, hopefully Florian can clarify.

::: chat/cloudfile/log4moz.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Florian mentioned that this was added to toolkit as Log.jsm (https://mxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/Log.jsm), it looks like it was only KIND of added, the object name is slightly different, etc. We should use this instead though so log4moz doesn't need to be added to Instantbird. (To be clear, this will involve modifying the nsHightail.js and nsBox.js to use Log.jsm instaed of log4moz.js.)

::: chat/locales/en-US/messenger.dtd
@@ +1,1 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

I doubt this entire file is needed. We should only include the entities we need (this goes for messenger.properties too!)

::: chat/themes/linux/preferences/preferences.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Are these preferences files actually used?

::: im/app/profile/all-instantbird.js
@@ +96,5 @@
>  
>  pref("messenger.proxies", "");
>  pref("messenger.globalProxy", "none");
>  pref("messenger.warnOnQuit", true);
> +pref("mail.cloud_files.learn_more_url", "https://support.mozillamessaging.com/kb/filelink-large-attachments");

I wonder what our thoughts are on include this preference here. Florian/aleth?

::: im/content/conversation.xml
@@ +136,5 @@
> +        <![CDATA[
> +          return aEvent.dataTransfer.types.contains("application/x-moz-file");
> +        ]]>
> +        </body>
> +      </method>

Why exactly is this function needed? Please include a comment in the code. (And give it a better function name.) Thanks.

@@ +138,5 @@
> +        ]]>
> +        </body>
> +      </method>
> +
> +      <method name="onDrop">

Please give this a better function name. It looks like most of the contents wants to be a separate file (as you've mentioned) in im/content.

@@ +144,5 @@
> +        <body>
> +        <![CDATA[
> +        Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +        Components.utils.import("resource:///modules/cloudFileAccounts.js");
> +        let listener = function(aFile, aCloudProvider, outerThis) {

Please choose a better name than listener. For reference this is mostly a copy of: https://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1183
Attachment #8429369 - Flags: feedback+
(In reply to Patrick Cloke [:clokep] from comment #4)
> Comment on attachment 8429369 [details] [diff] [review]

> ::: chat/cloudfile/jar.mn

> >  messenger.jar:
> > +% content messenger %content/messenger/
> 
> I'm unsure if this should go here or in the chat jar.mn or in the im/ one,
> hopefully Florian can clarify.

Thunderbird has it in mail/, so if we want it, it should go in im/.


> ::: im/app/profile/all-instantbird.js

> > +pref("mail.cloud_files.learn_more_url", "https://support.mozillamessaging.com/kb/filelink-large-attachments");
> 
> I wonder what our thoughts are on include this preference here.
> Florian/aleth?

We probably don't want to point Instantbird users to this url. Is the code doing something sane if the pref isn't set?
(Reporter)

Comment 6

5 years ago
Posted patch Patch1(v2) (obsolete) — Splinter Review
This patch now has everything that's done to get FileLink integrated in Instantbird, be it copying cloudfile to chat/, copying other files (png's), making sure the panel in the Preference Pane works.

What needs to be done in this :
* Make sure the paper-clip icon shown in the 'FileLink' tab in the Preference Window works on Mac and Windows. This will involve making changes to im/themes/{preferences-pinstripe, preferences-winstripe}/preferences.css so that the appropriate image is picked up instead of the sprite that's being used there currently.

What this version does differently (in a positive sense):
* For Linux, this patch doesn't copy preferences.css to chat/themes/linux/preferences/ and instead copies the CSS rules required to im/themes/preferences-gnomestripe/preferences.css.
* Added a working tab in the Preference Window (oh boy, it took some effort)
Attachment #8429357 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
Posted patch Patch2(v2) (obsolete) — Splinter Review
This patch basically adds the parts required to get the drag and drop UI to work with the FileLink integration that Patch1 does.

What needs to be done :
* Need to move and then rename the methods I've added (onDrop and checkDrag) to another file.
Attachment #8429369 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
(In reply to Florian Quèze [:florian] [:flo] (Away until June 2nd) from comment #5)
> We probably don't want to point Instantbird users to this url. Is the code
> doing something sane if the pref isn't set?
It doesn't do much error handling. I could try to catch it if the error bubbles up but since we need to point to some sane url sometime later anyway, I thought why not keep this pointed to the Thunderbird support article for now and change this later when we have something specific for IB users. I've filed a followup bug 1017034 for this.
(Reporter)

Comment 9

5 years ago
(In reply to Patrick Cloke [:clokep] from comment #4)
> This entire fixIterator function is probably not necessary. We should see
> what parts of it are ACTUALLY necessary and inline that.
I've kept the last case since that seems to be the one being called, after checking.

> >    accountTypeSelected: function AAD_accountTypeSelected() {
> > +    if (!this._accountType)
> > +      this._accountType = document.getElementById("accountType");
> 
> Is this change necessary? Why?
This change seems to be necessary as explained here : http://log.bezut.info/instantbird/140527/#m608 I tried removing this change and testing. Everything works (as far as I tested) just that I get an error in the error console when I'm adding account, which doesn't seem to affect anything. I've kept this change since I don't think this is harmful. If you want I can add a TODO or something here to keep this in mind later on?

Comment 10

5 years ago
Comment on attachment 8431877 [details] [diff] [review]
Patch1(v2)

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

::: chat/cloudfile/content/Box/management.xhtml
@@ +15,5 @@
>      <script type="text/javascript;version=1.8"
>              src="chrome://messenger/content/cloudfile/Box/management.js"/>
>      <link rel="stylesheet"
>          type="text/css"
> +        href="chrome://instantbird/skin/preferences/preferences.css" />

You can't include chrome://instantbird files here because that won't work for TB. 

Is it actually necessary to include the preferences.css here? If I understand correctly, this XUL is inserted in the Preferences pane which should already include that stylesheet.

If it *is* necessary to include it, the required CSS rules will have to go in a new file in chat/.

::: chat/cloudfile/content/addAccountDialog.js
@@ +195,5 @@
>    },
>  
>    accountTypeSelected: function AAD_accountTypeSelected() {
> +    if (!this._accountType)
> +      this._accountType = document.getElementById("accountType");

Why wasn't this needed before?
Attachment #8431877 - Flags: feedback?(clokep)
Attachment #8431882 - Flags: feedback?(clokep)
Comment on attachment 8431877 [details] [diff] [review]
Patch1(v2)

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

This looks like a great start! I've got a bunch of comments below, plus aleth's comments. Can't wait to try the next version!

::: chat/cloudfile/cloudFileAccounts.js
@@ +273,5 @@
>      else
>        Services.logins.addLogin(newLoginInfo);
>    },
>  
> +  fixIterator: function(aEnum, aIface) {

enumerateCategory returns an nsISimpleEnumerator: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICategoryManager#enumerateCategory%28%29

To this code can be simplified a bit more.

::: chat/cloudfile/content/Hightail/management.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  function onLoadProvider(provider) {
> +  //let messenger = Components.classes["@mozilla.org/messenger;1"]
> +  //                          .createInstance(Components.interfaces.nsIMessenger);

I think you're working on the formatFileSize so I won't comment too much about this. If not, please switch to a TODO comment instead of just commenting out code.

::: chat/locales/en-US/messenger.dtd
@@ +1,1 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

It looks like you never answered my question last time of what is actually necessary from messenger.dtd and messenger.properties? If they're not necessary, please hg rm them.

::: chat/locales/en-US/preferences.dtd
@@ +1,1 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

This seems wrong to be in chat/ since it's a UI thing. Is this even used? If so, this probably needs to be part of http://dxr.mozilla.org/comm-central/source/im/locales/en-US/chrome/instantbird/preferences/preferences.dtd. (Sames goes for preferences.properties.)

::: chat/themes/windows/preferences/preferences.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

chat/ does not have preferences, im/ and mail/ has it so...this is not necessary in someway or another. (For both the macos and windows.)

::: im/content/jar.mn
@@ +90,5 @@
>  	content/instantbird/preferences/privacy.js              (preferences/privacy.js)
>  	content/instantbird/preferences/themes.xul              (preferences/themes.xul)
>  	content/instantbird/preferences/themes.js               (preferences/themes.js)
> +*	content/instantbird/preferences/cloudfile.xul           (preferences/cloudfile.xul)
> +*	content/instantbird/preferences/cloudfile.js            (preferences/cloudfile.js)

I think we renamed this pane file transfer. Let's rename the files too. (Goes for the dtd/properties files too.)

::: im/content/preferences/cloudfile.js
@@ +1,1 @@
> +/*

I have not exhaustively gone over this file yet, first I want to ask:
1. It looks like you didn't hg cp this, that would probably make the diff nicer to read. (And easily show us what was modified.)
2. Did you check that all the code was still necessary? Did you remove the bits that are not?

::: im/content/preferences/cloudfile.xul
@@ +49,5 @@
> +    <tabbox id="attachmentPrefs" flex="1" onselect="gApplicationsTabController.tabSelectionChanged();">
> +      <tabs id="attachmentPrefsTabs">
> +        <tab id="attachmentsInTab" label="&attachments.incoming.label;"/>
> +        <tab id="attachmentsOutTab" label="&attachments.outgoing.label;"/>
> +      </tabs>

I thought we were getting rid of these tabs? Doesn't all the FileLink stuff go in outgoing?

@@ +80,5 @@
> +
> +          <separator class="thin"/>
> +
> +          <script type="application/javascript"
> +                  src="chrome://messenger/content/preferences/downloads.js"/>

This file probably doesn't exist in Instantbird.

@@ +119,5 @@
> +          <vbox flex="1">
> +            <hbox id="cloudFileToggleAndThreshold" align="center">
> +              <checkbox id="enableThreshold"
> +                        label="&enableCloudFileAccountOffer.label;"
> +                        preference="mail.compose.big_attachments.notify"

Where else is this preference used? If it's just used in a UI layer, it needs to be renamed to things under "messenger.*": messenger.cloudfile.notify, most likely here.

@@ +123,5 @@
> +                        preference="mail.compose.big_attachments.notify"
> +                        oncommand="gCloudFileTab.updateThreshold();"/>
> +              <textbox id="cloudFileThreshold" type="number" increment="1"
> +                       maxlength="4" size="1"
> +                       preference="mail.compose.big_attachments.threshold_kb"

I think we said we're deleting this preference for now, if not rename it to messenger.cloudfile.threshold_kb.

::: im/locales/en-US/chrome/instantbird/preferences/cloudfile.dtd
@@ +8,5 @@
> +<!ENTITY  actionColumn2.label         "Action">
> +<!ENTITY  actionColumn2.accesskey     "A">
> +
> +<!ENTITY  focusSearch1.key            "f">
> +<!ENTITY  focusSearch2.key            "k">

These two don't seem to be used. Are they?

::: im/locales/en-US/chrome/instantbird/preferences/preferences.dtd
@@ +16,5 @@
>  <!ENTITY  paneContent.title       "Content">
>  <!ENTITY  paneApplications.title  "Applications">
>  <!ENTITY  panePrivacy.title       "Privacy">
>  <!ENTITY  paneThemes.title        "Themes">
> +<!ENTITY  paneCloudfile.title     "FileLink">

I think we decided to rename this "File transfer", right? (It's possible I told you this after you uploaded the patch and am misremembering timing. :))

::: im/themes/jar.mn
@@ +53,5 @@
> +	skin/classic/instantbird/preferences/alwaysAsk.png        (preferences-pinstripe/alwaysAsk.png)
> +	skin/classic/instantbird/preferences/application.png      (preferences-pinstripe/application.png)
> +	skin/classic/instantbird/preferences/applications.css     (preferences-pinstripe/applications.css)
> +	skin/classic/instantbird/preferences/mail-options.png     (preferences-pinstripe/mail-options.png)
> +	skin/classic/instantbird/preferences/mail-options@2x.png  (preferences-pinstripe/mail-options@2x.png)

Why do we use mail-options.png for Mac and attachments.png for Linux/Windows? I.e. why is one a sprite and not the others? Is this how it is in Thunderbird? If we're only pulling out a single image from here, maybe we should modify the image.
Attachment #8431877 - Flags: feedback?(clokep) → feedback+
Comment on attachment 8431882 [details] [diff] [review]
Patch2(v2)

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

::: im/content/conversation.xml
@@ +25,5 @@
>            <xul:notificationbox class="conv-messages" anonid="convNotificationBox" flex="1" xbl:inherits="chat">
>              <xul:vbox flex="1">
>                <xul:browser anonid="browser" type="content-conversation" flex="1"
> +                           xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autoscrollpopup"
> +                           ondragenter="return checkDrag(event);" ondragover="return checkDrag(event);" ondrop="onDrop(event);"/>

Does this work now? I know you had some issues getting things working on the browsers.

@@ +136,5 @@
> +        <![CDATA[
> +          return aEvent.dataTransfer.types.contains("application/x-moz-file");
> +        ]]>
> +        </body>
> +      </method>

I'm still not quite 100% sure why this is necessary. Were you just checking if things worked or is this still required?

@@ +144,5 @@
> +        <body>
> +        <![CDATA[
> +        Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +        Components.utils.import("resource:///modules/cloudFileAccounts.js");
> +        let listener = function(aFile, aCloudProvider, outerThis) {

I believe I asked you to give this a better name.

The third parameter should probably be aConv. and saved as this.conv.

@@ +158,5 @@
> +           if (Components.isSuccessCode(aStatusCode)) {
> +              Components.utils.reportError("Link is " + this.cloudProvider.urlForFile(this.file) + "\n");
> +              this._this._conv.sendMsg("Uploaded on " + this.cloudProvider.displayName +
> +                                           " with link as " + this.cloudProvider.urlForFile(this.file) +
> +                                           "\n");

This needs to be a translatable string! You also shouldn't need a \n at the end. (This string also doesn't make sense, something like "You've been sent a file: '<filename>' at <link>." or something. Maybe someone has a better idea, brainstorm in #instantbird.

@@ +163,5 @@
> +           }
> +           else {
> +             let title;
> +             let msg;
> +             //let displayName = cloudFileAccounts.getDisplayName(this.cloudProvider);

Why does this not work?

@@ +180,5 @@
> +               break;
> +             default:
> +               break;
> +             }
> +             }

This is supposed to give feedback to the user, I assume. Please do something, even if it's only throwing errors to the error console for now.
Attachment #8431882 - Flags: feedback?(clokep) → feedback+
(Reporter)

Comment 13

5 years ago
Posted patch Patch1(v3) (obsolete) — Splinter Review
This version addresses most of the comments given earlier.

Things to be done:
* Check if TB builds and works
* Use the bundle for fetching the title of the preference pane, in the preffing off code
* Fix the onselect listener issue in addAccountDialog
Attachment #8431877 - Attachment is obsolete: true
Attachment #8435337 - Flags: review?(clokep)
(Reporter)

Comment 14

5 years ago
Posted patch Patch2(v3) (obsolete) — Splinter Review
This addresses most of the comment.

Things to be done :
* Pref off the handler addition to browser and textbox
Attachment #8431882 - Attachment is obsolete: true
Attachment #8435338 - Flags: review?(clokep)

Comment 15

5 years ago
Comment on attachment 8435337 [details] [diff] [review]
Patch1(v3)

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

Drive-by comments:

::: chat/cloudfile/content/addAccountDialog.js
@@ +77,5 @@
>  
> +/*    this._accountType.addEventListener("onselect", function(e) {
> +      return addAccountDialog.accountTypeSelected();
> +    });
> +*/

What's going on here? What happened to the accountType = null bug?

::: im/content/preferences/handlers.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +richlistitem.requireBinding {

Can we give this class a more meaningful name?
Efficient CSS is to put the class selector first.
Comment on attachment 8435337 [details] [diff] [review]
Patch1(v3)

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

Looks like we're getting there! A few nits, a few minor changes and some questions!

::: chat/cloudfile/cloudFileAccounts.js
@@ +146,1 @@
>                                    Ci.nsISupportsCString)) {

Nit: Spacing. Although I actually think we should just inline this function.

::: chat/cloudfile/content/Box/management.js
@@ +7,2 @@
>    let fileSpaceUsed = document.getElementById("file-space-used");
> +  fileSpaceUsed.textContent = DownloadUtils.convertByteUnits(provider.fileSpaceUsed).join(" ");

These should all use a localized string like http://dxr.mozilla.org/comm-central/source/mozilla/browser/locales/en-US/chrome/browser/pageInfo.properties#51 (Maybe make a function which does this?)

::: chat/cloudfile/content/Box/management.xhtml
@@ +15,5 @@
>      <script type="text/javascript;version=1.8"
>              src="chrome://messenger/content/cloudfile/Box/management.js"/>
>      <link rel="stylesheet"
>          type="text/css"
> +        href="chrome://messenger/skin/preferences/filetransfers.css" />

This should be called cloudfileManagement.css or something like that. The code is called cloudfile, we're calling it "File transfers" in Instantbird and Thunderbird calls this "FileLink", I know it's confusing. Generally though: things inside of chat/ should all refer to "cloudfile", things in im/ "File transfers", mail/ "FileLink" (or cloudfile).

::: chat/cloudfile/content/Hightail/management.js
@@ +7,2 @@
>    let fileSpaceUsed = document.getElementById("file-space-used");
> +  fileSpaceUsed.textContent = DownloadUtils.convertByteUnits(provider.fileSpaceUsed).join(" ");

Same comment as before about localizing this.

::: chat/cloudfile/content/addAccountDialog.js
@@ +77,5 @@
>  
> +/*    this._accountType.addEventListener("onselect", function(e) {
> +      return addAccountDialog.accountTypeSelected();
> +    });
> +*/

Why is this commented out?

::: chat/locales/en-US/cloudfile.properties
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

These used to be in messenger.properties, correct? Did you remove them from messenger.properties?

::: chat/themes/linux/preferences/filetransfers.css
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I think we discussed this on IRC, but this needs to be named cloudfile.css or something "filetransfers" is an Instantbird term. Also, this CSS needs to be removed from wherever it came from so it's not duplicated.

::: im/app/profile/all-instantbird.js
@@ +44,5 @@
>  // Timespan (in seconds) that a MUC nick is marked active after speaking.
>  // -1 = keep active forever
>  pref("messenger.conversations.nickActiveTimespan", 3600);
>  
> +pref("messenger.cloudfile.enabled", true);

Default to false.

::: im/content/jar.mn
@@ +90,5 @@
>  	content/instantbird/preferences/privacy.js              (preferences/privacy.js)
>  	content/instantbird/preferences/themes.xul              (preferences/themes.xul)
>  	content/instantbird/preferences/themes.js               (preferences/themes.js)
> +*	content/instantbird/preferences/filetransfers.xul       (preferences/filetransfers.xul)
> +*	content/instantbird/preferences/filetransfers.js        (preferences/filetransfers.js)

Let's make these all the filetransfers.* files fileTransfers.* (i.e. camel-case).

::: im/content/preferences/applications.js
@@ +497,5 @@
>  
>      for each (let visibleType in visibleTypes) {
>        let item = document.createElement("richlistitem");
>        item.setAttribute("type", visibleType.type);
> +      item.setAttribute("class", "requireBinding");

Can we come up with a better class name than this? I'm also a bit confused at why this is required. I know you guys discussed it on IRC yesterday, but can you give me a summary (in the bug)?

::: im/content/preferences/filetransfers.xul
@@ +43,5 @@
>  
> +    <tabpanels flex="1">
> +      <tabpanel orient="vertical">
> +        <vbox flex="1">
> +          <hbox flex="1">

Does this really need both a vbox and hbox flex="1" here? That seems a bit funky. I'd think you could get rid of one (or both) of these.

::: im/content/preferences/main.js
@@ +16,5 @@
>      this._pane = document.getElementById("paneMain");
> +    if (Services.prefs.getBoolPref("messenger.cloudfile.enabled")) {
> +      let mainWindow = document.getElementById("BrowserPreferences");
> +      let paneFT = document.createElement("prefpane");
> +      let bundle = Services.strings.createBundle("chrome://instantbird/locale/preferences/preferences.dtd");

This looks unused!

@@ +19,5 @@
> +      let paneFT = document.createElement("prefpane");
> +      let bundle = Services.strings.createBundle("chrome://instantbird/locale/preferences/preferences.dtd");
> +      paneFT.setAttribute("id", "paneFileTransfers");
> +      // TODO Use bundle for fetching the localized title
> +      paneFT.setAttribute("label", "File Transfers");

Does it work if you set it to the entity name? I kind of doubt it...

::: im/themes/preferences-gnomestripe/preferences.css
@@ +192,5 @@
> +  overflow: hidden;
> +}
> +
> +
> +/* Loading panel */

What does this mean? Also, nit: the two blank lines.

@@ +199,5 @@
> +  padding-top: 150px;
> +}
> +
> +/**
> + * Used by the outgoing attachment manager

This comment doesn't make much sense anymore.
Attachment #8435337 - Flags: review?(clokep) → review-
Comment on attachment 8435338 [details] [diff] [review]
Patch2(v3)

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

Getting there, but needs a bit more!

::: im/content/conversation.xml
@@ +25,5 @@
>            <xul:notificationbox class="conv-messages" anonid="convNotificationBox" flex="1" xbl:inherits="chat">
>              <xul:vbox flex="1">
>                <xul:browser anonid="browser" type="content-conversation" flex="1"
> +                           xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autoscrollpopup"
> +                           ondragenter="event.stopPropagation(); event.preventDefault();" ondragover="event.stopPropagation(); event.preventDefault();" ondrop="handleDroppedFiles(event);"/>

We should probably have a comment about the stopPropagation/preventDefault stuff here. You use those couple lines a lot (I see six times), maybe combine them into a function with a comment above it.

@@ +136,5 @@
> +        <![CDATA[
> +          Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +          Components.utils.import("resource://gre/modules/Services.jsm");
> +          Components.utils.import("resource:///modules/cloudFileAccounts.js");
> +          let uploadListener = function (aConv, aFile, aCloudProvider) {

Nit: no space after function

@@ +146,5 @@
> +          uploadListener.prototype = {
> +            onStartRequest: function (aRequest, aContext) {
> +              this.conv.systemMessage(this.bundle.GetStringFromName("uploadStartedNotif"));
> +            },
> +          

Nit: Trailing white space!!!

@@ +183,5 @@
> +                                                         [displayName,
> +                                                          this.file.leafName], 2);
> +                  break;
> +                case this.cloudProvider.uploadCanceled:
> +                  break;

No message in this situation, but we still try to print one below?

@@ +194,5 @@
> +              }
> +            },
> +
> +            QueryInterface: XPCOMUtils.generateQI([Ci.nsIRequestObserver,
> +                                                   Ci.nsISupportsWeakReference])

Are you sure Ci is defined anywhere?

@@ +198,5 @@
> +                                                   Ci.nsISupportsWeakReference])
> +          };
> +          let dataTransfer = aEvent.dataTransfer;
> +          let types = dataTransfer.types;
> +          if (!types.contains("application/x-moz-file"))

Types is only used once, inline it.

@@ +201,5 @@
> +          let types = dataTransfer.types;
> +          if (!types.contains("application/x-moz-file"))
> +            return;
> +          aEvent.preventDefault();
> +          aEvent.stopPropagation();

Didn't you inline these above in the ondrop attribute?

@@ +204,5 @@
> +          aEvent.preventDefault();
> +          aEvent.stopPropagation();
> +          if (cloudFileAccounts.accounts.length == 0) {
> +            cloudFileAccounts.addAccountDialog();
> +          }

Nit: No { } around a single line.

@@ +206,5 @@
> +          if (cloudFileAccounts.accounts.length == 0) {
> +            cloudFileAccounts.addAccountDialog();
> +          }
> +          else {
> +            let account = cloudFileAccounts.accounts[1];

[1] not [0]? Why?

@@ +207,5 @@
> +            cloudFileAccounts.addAccountDialog();
> +          }
> +          else {
> +            let account = cloudFileAccounts.accounts[1];
> +            for (var i = 0; i < dataTransfer.mozItemCount; ++i) {

Nit: let not var.

@@ +209,5 @@
> +          else {
> +            let account = cloudFileAccounts.accounts[1];
> +            for (var i = 0; i < dataTransfer.mozItemCount; ++i) {
> +              let file = dataTransfer.mozGetDataAt("application/x-moz-file", i);
> +              file = file.QueryInterface(Ci.nsIFile);

Again, are you sure Ci is defined?
Attachment #8435338 - Flags: review?(clokep) → review-
(Reporter)

Comment 18

5 years ago
(In reply to Patrick Cloke [:clokep] from comment #16)

> Nit: Spacing. Although I actually think we should just inline this function.
I'll inline it.

> These should all use a localized string like
A localized string bundle is already used by convertByteUnits(http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#78) which gets the localized units from http://dxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties#66 so I think we're good.

> This should be called cloudfileManagement.css or something like that. The
I'll name it cloudfile.css.

> Why is this commented out?
Fixed.

> These used to be in messenger.properties, correct? Did you remove them from
> messenger.properties?
No, I hadn't removed them. I'll remove them now.

> I think we discussed this on IRC, but this needs to be named cloudfile.css
> or something "filetransfers" is an Instantbird term. Also, this CSS needs to
> be removed from wherever it came from so it's not duplicated.
I'll name it cloudfile.css (as I said above) and remove it from where it came.

> Can we come up with a better class name than this? I'm also a bit confused
> at why this is required. I know you guys discussed it on IRC yesterday, but
> can you give me a summary (in the bug)?
So in filetransfers.xul, the richlistitem's (each FileLink provider in the provider listing) gets styled by http://mxr.mozilla.org/comm-central/source/im/content/preferences/handlers.css#5 which calls gApplicationsPane(http://mxr.mozilla.org/comm-central/source/im/content/preferences/handlers.xml#45) which we don't have defined in filetransfers.js. It was required in TB because it used to have a tab similar to our Applications pane which needed these bindings.

> Does this really need both a vbox and hbox flex="1" here? That seems a bit
> funky. I'd think you could get rid of one (or both) of these.
I'll trim the useless parts more.

> What does this mean? Also, nit: the two blank lines.
I'm not sure but something related to the placeholder where the iframe is loaded probably?

> This comment doesn't make much sense anymore.
Removed.
(In reply to Saurabh Anand [:sawrubh] from comment #18)
> (In reply to Patrick Cloke [:clokep] from comment #16)
> > These should all use a localized string like
> A localized string bundle is already used by
> convertByteUnits(http://mxr.mozilla.org/mozilla-central/source/toolkit/
> mozapps/downloads/DownloadUtils.jsm#78) which gets the localized units from
> http://dxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/
> chrome/mozapps/downloads/downloads.properties#66 so I think we're good.
No, the space you're hard coding between items has to be localized, see: https://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties#101 (as aleth pointed out, you can actually just use this properties file!)

> > Can we come up with a better class name than this? I'm also a bit confused
> > at why this is required. I know you guys discussed it on IRC yesterday, but
> > can you give me a summary (in the bug)?
> So in filetransfers.xul, the richlistitem's (each FileLink provider in the
> provider listing) gets styled by
> http://mxr.mozilla.org/comm-central/source/im/content/preferences/handlers.
> css#5 which calls
> gApplicationsPane(http://mxr.mozilla.org/comm-central/source/im/content/
> preferences/handlers.xml#45) which we don't have defined in
> filetransfers.js. It was required in TB because it used to have a tab
> similar to our Applications pane which needed these bindings.
I'm not sure I entirely understand this, but aleth seems to and if he says it makes sense than that's fine with me.

> > This comment doesn't make much sense anymore.
> Removed.
If it is useful to have, update it, don't just remove it. If it wasn't useful, remove it.
Status: NEW → ASSIGNED
(Reporter)

Comment 20

5 years ago
Attachment #8435337 - Attachment is obsolete: true
Attachment #8436380 - Flags: feedback?(clokep)
(Reporter)

Comment 21

5 years ago
Attachment #8435338 - Attachment is obsolete: true
Attachment #8436381 - Flags: feedback?(clokep)
(Reporter)

Comment 22

5 years ago
I hadn't removed the entries, for the files I had moved, from the jar.mn's which is fixed in this patch. I've tested building TB and it builds without any issues. I triggered a try run with this setting : 'try: -b o -e -p linux,linux64 -u mozmill[Ubuntu] -t none' however I got some error related to mozconfig : https://tbpl.mozilla.org/?tree=Try&rev=28b186f94c07

Seems I had/have to do something with the mozconfig's before pushing, so leaving that for now.
Attachment #8436380 - Attachment is obsolete: true
Attachment #8436380 - Flags: feedback?(clokep)
Attachment #8436394 - Flags: feedback?(clokep)
Comment on attachment 8436394 [details] [diff] [review]
Move cloudfile to chat/ and other changes to make it work (v5)

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

I think we're getting closer! It would probably be worth doing a Thunderbird Try run now, as I gave you the link on IRC.

::: chat/cloudfile/cloudFileAccounts.js
@@ +141,5 @@
>    },
>  
>    enumerateProviders: function() {
>      let providerList = [];
> +    let face = Ci.nsISupportsCString || Ci.nsISupports;

You know that this is nsISupportsString, inline this directly below.

@@ +153,2 @@
>        let provider = this.getProviderForType(entry.data);
>        yield [entry.data, provider];

I don't think you need to define the iterator function, just directly iterate the enumerator. Something like:

let simpleEnum = categoryManager.enumerateCategory(CATEGORY);
while (simpleEnum.hasMoreElements()) {
  let entry = simpleEnum.getNext().QueryInterface(face);
  let provider = this.getProviderForType(entry.data);
  yield [entry.data, provider];
}

::: chat/cloudfile/content/Box/management.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +Components.utils.import("resource://gre/modules/DownloadUtils.jsm");

Nit: Add a blank line after the license comment.

Any other comments for this file also apply to the othe rmanagement.js file.

@@ +8,5 @@
> +  const downloadProperties =
> +    "chrome://messenger/locale/cloudfile.properties";
> +  let bundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
> +                         .getService(Components.interfaces.nsIStringBundleService)
> +                         .createBundle(downloadProperties);

I think I suggested making a function, which should clean this code up a bit:

function fileSpaceUsed(aSize) {
 return bundle.formatStringFromName(
		"spaceDetails", DownloadUtils.convertByteUnits(provider.fileSpaceUsed), 2);
}

::: chat/cloudfile/content/addAccountDialog.js
@@ +76,5 @@
>      });
>  
> +    this._accountType.addEventListener("select", function(e) {
> +      return addAccountDialog.accountTypeSelected();
> +    });

Why is this change necessary? Wasn't this commented out in the previous patch? I assumed this meant it'd be deleted.

::: chat/cloudfile/content/addAccountDialog.xul
@@ +32,5 @@
>  #endif
>    <description id="createAccountText">&addAccountDialog.createAccountText2;</description>
>    <description id="noAccountText" hidden="true">&addAccountDialog.noAccountText;</description>
>  
> +  <menulist id="accountType" class="indent">

Why is this change necessary?

::: chat/locales/en-US/cloudfile.properties
@@ +9,5 @@
> +# or the username should appear
> +passwordPrompt=Enter your password for %1$S on %2$S:
> +
> +uploadStartedNotif=File upload started
> +uploadCompleteMsg=You've been sent a file: '%1$S' at %2$S

These are Instantbird specific, right? They should not be in cloudfile.properties.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ -441,5 @@
> -## @loc None
> -# LOCALIZATION NOTE (passwordPrompt): Do not translate the word %S below.
> -# Place the word "%S" in your translation where the email address
> -# or the username should appear
> -passwordPrompt=Enter your password for %1$S on %2$S:

Are you positive this is ONLY used by filelink?

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ -355,5 @@
>  ## LOCALIZATION NOTE(errorSavingMsg): Do not translate the word %S. It
>  ## will be replaced with the name of the folder the message is being saved to.
>  errorSavingMsg=There was an error saving the message to %S. Retry?
>  
> -errorCloudFileAuth.title=Authentication Error

It doesn't look like you updated any consumers of this properties file to use the new cloudfile.properties file.

Are we sure we want these same strings in Instantbird, by the way? Where are they used? We need to talk a bit of UI here.
Attachment #8436394 - Flags: feedback?(clokep) → feedback+
Comment on attachment 8436381 [details] [diff] [review]
Make FileLink work on drag and drop in Instantbird (v4)

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

Looks like it's making progress!

::: im/content/conversation.xml
@@ +50,5 @@
>          </xul:hbox>
>          <xul:splitter class="splitter" anonid="splitter-bottom"/>
>          <xul:vbox anonid="conv-bottom" class="conv-bottom">
>            <xul:textbox anonid="inputBox" class="conv-textbox" multiline="true" flex="1"
> +                       onclick="delete document.getBindingParent(this)._completions;"/>

Generally, don't change arbitrary chunks outside of what you're doing, but this is fine here.

@@ +138,5 @@
>          ]]>
>          </body>
>        </method>
>  
> +      <!-- Cancel the given event -->

Nit: Full sentences with periods at the end.

@@ +157,5 @@
> +          this.cancelEvent(aEvent);
> +          const {interfaces: Ci, utils: Cu} = Components;
> +          Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +          Cu.import("resource://gre/modules/Services.jsm");
> +          Cu.import("resource:///modules/cloudFileAccounts.js");

cloudFileAccounts isn't a module, does this actually work? It looks to me like it's a component. See how this is done in the compose code.

@@ +162,5 @@
> +          let uploadListener = function(aConv, aFile, aCloudProvider) {
> +            this.conv = aConv;
> +            this.file = aFile;
> +            this.cloudProvider = aCloudProvider;
> +            this.bundle = Services.strings.createBundle("chrome://messenger/locale/cloudfile.properties");

This should probably be made a lazy getter.

@@ +215,5 @@
> +                  break;
> +                }
> +
> +                if (displayError)
> +                  Cu.reportError(msg);

We should probably do something better with this error, did you have a plan for this? What happened to adding a notification bar to this patch?

@@ +224,5 @@
> +                                                   Ci.nsISupportsWeakReference])
> +          };
> +          let dataTransfer = aEvent.dataTransfer;
> +          if (!dataTransfer.types.contains("application/x-moz-file"))
> +            return;

This should be checked earlier up in the function.

Please add some comments (in general) to this function.
Attachment #8436381 - Flags: feedback?(clokep) → feedback+

Comment 25

5 years ago
Comment on attachment 8436381 [details] [diff] [review]
Make FileLink work on drag and drop in Instantbird (v4)

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

::: im/content/conversation.xml
@@ +91,5 @@
>            browser.addEventListener("keypress", this.browserKeyPress);
>            browser.addEventListener("dblclick", this.browserDblClick.bind(this));
>            Services.obs.addObserver(this, "conversation-loaded", false);
>  
> +          if (Services.prefs.getBoolPref("messenger.cloudfile.enabled")) {

Isn't this an IM-specific pref? Probably shouldn't be called messenger.* in that case.

@@ +144,5 @@
> +        <parameter name="aEvent"/>
> +        <body>
> +        <![CDATA[
> +          aEvent.preventDefault();
> +          aEvent.stopPropagation();

Shouldn't this also return true or false?

Comment 26

5 years ago
Comment on attachment 8436394 [details] [diff] [review]
Move cloudfile to chat/ and other changes to make it work (v5)

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

::: chat/cloudfile/content/addAccountDialog.js
@@ +74,5 @@
>      this._settings.addEventListener("overflow", function(e) {
>        addAccountDialog.fitIFrame();
>      });
>  
> +    this._accountType.addEventListener("select", function(e) {

You also have to call addAcountDialog.accountTypeSelected explicitly at the end of onInit, since you prevented the menulist constructor from doing so. Please be careful about details like this in the future.
(Reporter)

Comment 27

5 years ago
(In reply to Patrick Cloke [:clokep] from comment #23)
> Why is this change necessary? Wasn't this commented out in the previous
> patch? I assumed this meant it'd be deleted.

This was commented out previously because I couldn't get it to work last time (I was adding the eventListener for the wrong event). This is required btw because sometimes addAccountDialog.accountTypeSelected is called before addAccountDialog.onInit, so we decided to remove the inline event handler from the xul and add it in onInit instead, since that way we won't get 'this._accountType is null' error.

> Are you positive this is ONLY used by filelink?

I'm quite positive from the files shown in http://dxr.mozilla.org/comm-central/search?q=passwordPrompt&case=true that this is not used elsewhere.

> It doesn't look like you updated any consumers of this properties file to
> use the new cloudfile.properties file.
> 
> Are we sure we want these same strings in Instantbird, by the way? Where are
> they used? We need to talk a bit of UI here.

I'll be adding the notificationbox in the next patch.
(Reporter)

Comment 28

5 years ago
(In reply to Patrick Cloke [:clokep] from comment #24)
> cloudFileAccounts isn't a module, does this actually work? It looks to me
> like it's a component. See how this is done in the compose code.

I've tested this and it works probably because of http://dxr.mozilla.org/comm-central/source/mail/components/cloudfile/moz.build#19

> We should probably do something better with this error, did you have a plan
> for this? What happened to adding a notification bar to this patch?

Adding it in the upcoming version of the patch.
(Reporter)

Comment 29

5 years ago
Addressed comments.
Attachment #8436394 - Attachment is obsolete: true
Attachment #8436940 - Flags: feedback?(clokep)
(Reporter)

Updated

5 years ago
Attachment #8436940 - Flags: feedback?(florian)
Comment on attachment 8436940 [details] [diff] [review]
Move cloudfile to chat/ and other changes to make it work (v6)

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

Besides the small changes to management.js and me not understanding the changes to addAccountDialog, this looks ready. I think the next version we'll be ready to do a real review of instead of feedback! :)

::: chat/cloudfile/content/Box/management.js
@@ +7,5 @@
> +const downloadProperties =
> +  "chrome://messenger/locale/cloudfile.properties";
> +const bundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
> +                         .getService(Components.interfaces.nsIStringBundleService)
> +                         .createBundle(downloadProperties);

Inline downloadProperties. It's probably worth import Services.jsm and using Services.strings.createBundle.

@@ +9,5 @@
> +const bundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
> +                         .getService(Components.interfaces.nsIStringBundleService)
> +                         .createBundle(downloadProperties);
> +
> +function spaceUsed(aSize) {

Any reason you didn't use my suggested name of fileSpaceUsed? It probably makes more sense to call this "formatFileSize" though, since that's what it is actually doing.

@@ +12,5 @@
> +
> +function spaceUsed(aSize) {
> +  return bundle.formatStringFromName(
> +    "spaceDetails", DownloadUtils.convertByteUnits(aSize), 2);
> +}

I dislike the amount of duplication here between Box and Hightail, but I can't really think of a way to simplify this. Anyway, apply the same to Hightail's management.js. Sorry for the churn on this file.

::: chat/cloudfile/content/addAccountDialog.js
@@ +91,5 @@
>      // listener, so we'll call the function here manually.
>      this.onIFrameLoaded(null);
>  
>      addAccountDialog.fitIFrame();
> +    addAccountDialog.accountTypeSelected();

I'm still a bit confused about why these changes (+ the one in addAccountDialog.xul and handlers.css) are necessary. I think we need to discuss this when I'm able to concentrate more.
Attachment #8436940 - Flags: feedback?(clokep) → feedback+
(Reporter)

Comment 32

5 years ago
(In reply to Patrick Cloke [:clokep] from comment #31)
> Any reason you didn't use my suggested name of fileSpaceUsed? It probably
> makes more sense to call this "formatFileSize" though, since that's what it
> is actually doing.

There is already a variable called fileSpaceUsed and if I had done it, the line would have become
fileSpaceUsed.textContent = fileSpaceUsed(provider.fileSpaceUsed);
(just too many fileSpaceUsed's on a single line ;) )

I'll rename it to formatFileSize now.

> Hightail's management.js. Sorry for the churn on this file.

What does churn mean, just curious.

> I'm still a bit confused about why these changes (+ the one in
> addAccountDialog.xul and handlers.css) are necessary. I think we need to
> discuss this when I'm able to concentrate more.

Regarding the handlers.css changes :
So we have a richlistbox where richlistitems (the FileLink providers) are added. Since in TB, we had the 'Applications' *tab* along side FileLink *tab* in the Attachments pane, and gApplicationsPane was defined in the applications.js (which contained the code for both Applications and FileLink tabs), with the CSS that was there previously we didn't have any issues. Now when I moved this to IB, I removed the 'Applications' tab code from fileTransfers.js but still the same CSS bindings were being applied to the FileLink tab which cause a problem of missing gApplicationsPane. Actually we didn't require the bindings for FileLink, so I simply changed the css in handlers.xml to not apply to all richlistitems and instead just apply on the IB's 'Applications' pane richlistitems (since that's where this binding is still required).

Regarding the changes in addAccountDialog.{js, xul} :
The menulist constructors fired the select event and called accountTypeSelected and all this before the onInit method was called. So we needed to move this handling to *after* the onInit was called (for the first time, for the rest we planned to just add the event listener and everything would be similar to the setting before), so we removed the handler from the menulist, added the handler for select event in onInit and then called accountTypeSelected explicitly (this last explicit call was required to make sure we compensated for the first call to accountTypeSelected that would have happened had we not removed the inlined handler on the menulist). Does this make sense or do you want to discuss on IRC?

Comment 33

5 years ago
Have you checked existing TB Filelink add-ons (e.g. the Dropbox one) still work in TB after your changes are made?
(Reporter)

Comment 34

5 years ago
So I investigated and it seems everything(Box and Hightail providers) works in TB functionality wise, including the Dropbox addon. However the move of the management.xhtml's styling code from preferences.css inside mail's themes directory to cloudfile.css inside chat's themes directory broke Dropbox's preference pane styling. There are two options :
*we keep the styling in the preferences.css in mail's themes directory, if we do that then *everything* (Dropbox, Box, Hightail) in TB would work but *nothing* in IB would work since IB won't be able to access it
*if we don't do that and keep the situation as it is now, Dropbox addon will break in both TB and IB, however the rest of the providers will work in *both* TB and IB.

This is how it looks broken : http://i.imgur.com/BdULpr8.png

So the plan is to land this patch for now, break Dropbox and then fix it by sending a PR to change https://github.com/mikeconley/thunderbird-filelink-dropbox/blob/master/content/chrome/management.xhtml#L19
(In reply to Saurabh Anand [:sawrubh] from comment #34)
> *we keep the styling in the preferences.css in mail's themes directory, if
> we do that then *everything* (Dropbox, Box, Hightail) in TB would work but
> *nothing* in IB would work since IB won't be able to access it
This isn't really true, we'd just have to add this file in IB too (or add it to our preferences file). I do not like this solution.

> *if we don't do that and keep the situation as it is now, Dropbox addon will
> break in both TB and IB, however the rest of the providers will work in
> *both* TB and IB.

I prefer this plan, but I'll probably defer to mconley on this. We'll get his review on the next patch iteration.

> So the plan is to land this patch for now, break Dropbox and then fix it by
> sending a PR to change
> https://github.com/mikeconley/thunderbird-filelink-dropbox/blob/master/
> content/chrome/management.xhtml#L19

Last I checked most of the providers were open source, so it'd probably only take an hour to fix this for all of them. :)
(Reporter)

Comment 36

5 years ago
Addresses everything. Everything works.
Attachment #8436940 - Attachment is obsolete: true
Attachment #8436940 - Flags: feedback?(florian)
Attachment #8439199 - Flags: review?(clokep)
Comment on attachment 8439199 [details] [diff] [review]
Move cloudfile to chat/ and other changes to make it work (v7)

This is really close to r+, the r- is mostly because the spacing was changed throughout jar.mn. Please don't do that. I think it's appropriate to have both Florian and Mike to look over this also now.

I have a couple of questions that need to be answered:
- Looking through some of the cloudfile code I saw hard coded instances to "Thunderbird", I'm thinking we should use Services.appinfo.name (see [1]) instead. I'm OK fixing these in a follow-up for now.
- We probably need our own API keys in Instantbird for these apps, we'll need to ensure that they're read from the preferences or something as we do for Twitter [2].
- FileLink is going to save accounts under mail.cloud_file, I believe, are we OK with that? (Obviously it has to stay that way for Thunderbird.)

Mike, you probably don't need to re-read this whole bug; but there's a few changes I'd like to note below:
- log4moz is only available in Thunderbird, it was added to toolkit as Log.jsm
- Instantbird does not have iteratorUtils.jsm, we inlined the change to handle an nsISimpleEnumerator since it was easy enough.
- Instantbird does not have nsIMessenger, so we've switched to using DownloadUtils.jsm to format the file sizes.
- Some of these changes split things out of TB specific files into files that can be shared (e.g. removing some CSS from preferences.css and putting it into it's own cloudfile.css).
- There is ONE change (the one mentioned above) that COULD break extension's management pane, if they included preferences.css to get the styling for management.xhtml. sawrubh will try to update all the open source ones (after this lands) and I'll email the others. If you don't think this is reasonable, let me know. (Additionally, the DownloadUtils.jsm change + modifying install.rdf should be made to each to make them compatible w/ Instantbird!)

Thanks so much for taking the time review this, I know it's a pretty big patch!

[1] https://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#1627
[2] https://mxr.mozilla.org/comm-central/source/chat/protocols/twitter/twitter.js#379 / https://mxr.mozilla.org/comm-central/source/im/app/profile/all-instantbird.js#307 / https://mxr.mozilla.org/comm-central/source/mail/components/im/all-im.js#10
Attachment #8439199 - Flags: review?(mconley)
Attachment #8439199 - Flags: review?(florian)
Attachment #8439199 - Flags: review?(clokep)
Attachment #8439199 - Flags: review-
Comment on attachment 8439199 [details] [diff] [review]
Move cloudfile to chat/ and other changes to make it work (v7)

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

This all looks reasonable.

This has my r+ given the following proviso's:

1) A try push that shows the cloudfile Mozmill tests passing (we have pretty comprehensive cloudfile tests)
2) Verification from manual testing that Filelink still indeed works on TB


We also need a plan on how to deal with l10n if this does indeed need to get uplifted to comm-aurora / comm-beta to fix bug 1021684.

::: chat/cloudfile/content/Box/management.js
@@ +4,5 @@
>  
> +Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +const bundle = Services.strings.createBundle("chrome://messenger/locale/cloudfile.properties");

Let's call this gBundle, since it's a global in this file.

::: chat/cloudfile/content/Hightail/management.js
@@ +4,5 @@
>  
> +Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +const bundle = Services.strings.createBundle("chrome://messenger/locale/cloudfile.properties");

gBundle instead of bundle.

All of this duplication makes me sad. That's not your fault though - the implementation was already duping a ton of stuff across management.js scripts. *sigh*

::: chat/locales/en-US/cloudfile.properties
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

So, this is problematic if we're hoping to uplift these fixes to Earlybird and Beta...

Is this just a straight-copy? We might be able to do some magic during the l10n pack...

::: chat/themes/jar.mn
@@ +44,3 @@
>  #endif
> +
> +classic.jar:

Why is this going into a separate jar?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +79,5 @@
>  var gCharsetTitle;
>  var gCharsetConvertManager;
>  var _gComposeBundle;
> +var _gCloudfileBundle;
> +function getCloudfileBundle() {

Might as well just use a lazyGetter, from XPCOMUtils, no?
Attachment #8439199 - Flags: review?(mconley) → review+
(Reporter)

Comment 39

5 years ago
(In reply to Mike Conley (:mconley) from comment #38)
> 1) A try push that shows the cloudfile Mozmill tests passing (we have pretty
> comprehensive cloudfile tests)

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7b0fd820e09d

> 2) Verification from manual testing that Filelink still indeed works on TB

I've done manual testing and everything works.

> We also need a plan on how to deal with l10n if this does indeed need to get
> uplifted to comm-aurora / comm-beta to fix bug 1021684.

We had a discussion(http://log.bezut.info/instantbird/140614/#m216) and seems like we don't need to uplift.

> Why is this going into a separate jar?

If it goes in the chat.jar, it's not accessible to mail hence had to keep it this way.

> Might as well just use a lazyGetter, from XPCOMUtils, no?

Can't use lazyGetter due to https://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#83 so instead of writing a custom getter, I'm not simply doing |let bundle = document.getElementById("bundle_cloudfile");|
(Reporter)

Comment 40

5 years ago
Addresses mconley's comments.
Attachment #8439199 - Attachment is obsolete: true
Attachment #8439199 - Flags: review?(florian)
Attachment #8440350 - Flags: review?(florian)

Comment 41

5 years ago
(In reply to Saurabh Anand [:sawrubh] from comment #39)
> > Why is this going into a separate jar?
> 
> If it goes in the chat.jar, it's not accessible to mail hence had to keep it
> this way.

Can you explain this? I thought chat.jar is shared?
(Reporter)

Comment 42

5 years ago
(In reply to aleth [:aleth] from comment #41)
> Can you explain this? I thought chat.jar is shared?

So I had tried to register these files by making them go in chat.jar itself but I faced issues that TB was trying to access 'chrome/classic/skin/classic/messenger/icons/' while the files(CSS and icons) were being packaged in 'chrome/chat/skin/classic/messenger/icons/'. I enquired on irc and as Mic suggested (http://log.bezut.info/instantbird/140611/#m711) it worked changing it to what it's right now.
I have to say that the idea of moveing filelink under chat/ sounds really odd to me. It's clearly NOT primarily a chat feature at all. 

I know nothing of how instantbird builds but if instead of doing really crazy moves it should just do whatever it needs to access filelink from where it's now.
(In reply to Magnus Melin from comment #43)
> I have to say that the idea of moveing filelink under chat/ sounds really
> odd to me.

We all agree this is unfortunate, but there's no obviously better solution.

If you consider "chat/" as "the folder containing files shared between "mail/" and "im/" it starts being slightly less odd though.

> I know nothing of how instantbird builds but if instead of doing really
> crazy moves it should just do whatever it needs to access filelink from
> where it's now.

Having non-Thunderbird applications depend on the content of mail/ would be even worse than the move.
That can be seen many ways, if non-thunderbird applications want to use unforked thunderbird features it's not unreasonable to "depend on mail/".

Having thunderbird code disoriented to support non-thunderbird applications just isn't right. And like comment 0 says it's just to be used as a fallback and most often there should be better ways to send files in instant messaging. What's next, should we just move all of mailnews and mail under chat/ also so instantbird can send mail as fallback when chat protocols are not working?
(In reply to Magnus Melin from comment #43)
> I know nothing of how instantbird builds but if instead of doing really
> crazy moves it should just do whatever it needs to access filelink from
> where it's now.
I think you're biasing the conversation by saying "really crazy moves", I don't really understand what you're trying to imply by this. It's just moving a file to an area that is shared by multiple applications. It would be the same if SeaMonkey wanted to use a TB only feature, it would get moved from mail/ to mailnews/. (Minus UI changes, which we've forked.) This seems to be pretty standard.

(In reply to Magnus Melin from comment #45)
> That can be seen many ways, if non-thunderbird applications want to use
> unforked thunderbird features it's not unreasonable to "depend on mail/".
I think it is. This was discussed at length during one (or more) of the Thunderbird status meetings and at the Mozilla Summit between the people that were in Toronto. It was deemed the most reasonable path forward was to move the code under chat/ (as this is shared TB and IB code).

> Having thunderbird code disoriented to support non-thunderbird applications
> just isn't right.
Again I think you're biasing the conversation via your word choice. We're not "disorienting" the code, we're generalizing it. This is a pretty common thing to do in software in order to support larger feature sets. If you look at the patches you'll see that the UI bits are forked also, which is pretty standard among the applications in c-c. Additionally, there really aren't that many changes that are involved in the cloudfile code. They're summarized in comment #37.

> And like comment 0 says it's just to be used as a fallback
> and most often there should be better ways to send files in instant
> messaging.
It's to be used in a fallback in that we will try the protocol specific file transfer mechanism to work. This does NOT imply that it will not be used often. In fact, we have little faith in the built-in file transfer mechanisms due to NAT traversal issues (many of which did not exist when these protocols were "designed") and expect FileLink to be used a majority of the time. It also isn't possible to share a file in a chat room on most protocols, so FileLink would be instrumental in distributing a file to a large number of people at once.

> What's next, should we just move all of mailnews and mail under
> chat/ also so instantbird can send mail as fallback when chat protocols are
> not working?
I find this comment to be fairly rude and not constructive. Please refrain from using language like this in the future. Additionally, this is purposeful hyperbole in order to make these changes seem ridiculous and more invasive than they are. Cloudfile was designed as a standalone module and that is how we're treating it.

Comment 47

5 years ago
(In reply to Magnus Melin from comment #45)
> Having thunderbird code disoriented to support non-thunderbird applications
> just isn't right.

This is a really strange attitude to take, considering the whole existence of /chat could be seen as disorienting Instantbird code to support Thunderbird... it's not a productive attitude at all.
Of course I exaggerated, but it's to make a fair point, IMO. 

I have no recollection of this particular issue being discussed in any status meeting, and even so, that's hardly any place to make such decisions.

You want to call it "generalizing code" instead of disorienting because it happens to be a generalization to you. For everybody else, it not. Every potential contributor would go, WHY is that THERE!? We already have enough to keep up with w/ mail vs. mailnews/ and occacional editor/ stuff. The net effect of all the shared code is very probably not positive at this point, if it ever were... 
Code generalization does come with a high price - that's one of the realizations that made firefox succeed. (Forking the UI and start clean.)
Component: General → FileLink
Product: Chat Core → Thunderbird
Version: trunk → Trunk
(In reply to aleth [:aleth] from comment #47)
> This is a really strange attitude to take, considering the whole existence
> of /chat could be seen as disorienting Instantbird code to support
> Thunderbird... it's not a productive attitude at all.

It was not my decision to take, and (as i understand) separating chat backend and front end is very reasonable, and not a good comparison.
(In reply to Magnus Melin from comment #48)
> Of course I exaggerated, but it's to make a fair point, IMO.

A bug is not a place for arguments and exaggeration. Please be respectful to the people working here. The change here has already been discussed (otherwise Mike wouldn't have r+'ed it in comment 38), but if you feel further discussion is needed, please take it somewhere else (eg. tb-planning). Thanks.
sawrubh: Can you please attach your attempted unbitrotted patch, please.
(Reporter)

Comment 52

5 years ago
This is the unbitrotted patch. On Linux, I get the whole Box sign in experience, I haven't copied the browserRequest.css for other platforms so the sign in window wouldn't look all fancy on them. I'll do this once I get Box signing working functionally. So you'll need to sign into Box, Allow access to Thunderbird and then you'll see the error in IB : https://pastebin.mozilla.org/5553226

I checked the url auth parameters and they are the same in Thunderbird (where this works) and IB (where isn't working).
Attachment #8440350 - Attachment is obsolete: true
Attachment #8440350 - Flags: review?(florian)
Attachment #8463478 - Flags: feedback?(clokep)
Comment on attachment 8463478 [details] [diff] [review]
Move cloudfile to chat/ and other changes to make it work (v9)

I had to make a few minor changes to get this to work at all. (Mostly I had to change it to choose accounts[0] in the UI. I also added a Cu.reportError in conversation.xml in the catch statement.

In order to get this to actually log in successfully I had to change our cookieBehavior to 0. Thunderbird defaults to 0 [1], while Instantbird defaults to 2 [2]. This is rather unfortunate, I wonder if Florian has thoughts for how to get around that.

On a different note...the code doesn't seem to handle if the "Thunderbird" folder already exists. Sorry for taking so long to look at this!

[1] http://mxr.mozilla.org/comm-central/source/mail/app/profile/all-thunderbird.js#301
[2] http://mxr.mozilla.org/comm-central/source/im/app/profile/all-instantbird.js#264
Attachment #8463478 - Flags: feedback?(clokep) → feedback+
(Reporter)

Comment 54

5 years ago
I will not be able to take a look at this before December so unassigning myself.
Assignee: saurabhanandiit → nobody
(Reporter)

Updated

5 years ago
Status: ASSIGNED → NEW

Updated

4 years ago
Blocks: 783477

Comment 55

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #53)
> Comment on attachment 8463478 [details] [diff] [review]
> Move cloudfile to chat/ and other changes to make it work (v9)
> 
> I had to make a few minor changes to get this to work at all. (Mostly I had
> to change it to choose accounts[0] in the UI. I also added a Cu.reportError
> in conversation.xml in the catch statement.

It doesn't look like you attached the changed version?

> In order to get this to actually log in successfully I had to change our
> cookieBehavior to 0. Thunderbird defaults to 0 [1], while Instantbird
> defaults to 2 [2]. This is rather unfortunate, I wonder if Florian has
> thoughts for how to get around that.

This is now fixed since bug 1121874.
You need to log in before you can comment on or make changes to this bug.