1341211 - Get rid of nsIFilePicker.show() use in c-c

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Frank-Rainer Grahl (:frg)]

+++ This bug was initially created as a clone of Bug #1334975 +++

Bug 1334975 removed  nsIFilePicker.show(). Its still used in comm-central in:

> calendar/lightning/content/lightning-item-iframe.js
> mailnews/import/content/importDialog.js

Comment 1

[Jorg K (CEST = GMT+2)]

You could have given us the line numbers or a query, since "show" is hard to find. Looking at the files, I can't see it.

Looking at https://hg.mozilla.org/integration/mozilla-inbound/rev/597004bec637 I can see:
-  if (fp.show() == nsIFilePicker.returnOK) {
+  fp.open(rv => {
+    if (rv != nsIFilePicker.returnOK) {
+      return;
+    }
and variations.

Sadly, https://dxr.mozilla.org/comm-central/search?q=fp.show&redirect=false yields about 20 call sites of |fp.show()|.

This is now on M-I and will bust us when it hits M-C.

Comment 2

[Jorg K (CEST = GMT+2)]

Here comes more:
https://dxr.mozilla.org/comm-central/search?q=filepicker.show&redirect=false
this time also showing the files you mentioned.

Comment 3

[Frank-Rainer Grahl (:frg)]

Sorry that is actually how I checked it :) Should have put the query in.

Comment 4

[Jorg K (CEST = GMT+2)]

I'm afraid this is more complicated, since this needs to be changed over to promises:
https://hg.mozilla.org/integration/mozilla-inbound/rev/597004bec637#l5.12

Comment 5

[Frank-Rainer Grahl (:frg)]

Drat. And searching for nsIFilePicker.returnOK I found a few more .show variations:

https://dxr.mozilla.org/comm-central/search?q=nsIFilePicker.returnOK&redirect=false

Comment 6

[Jorg K (CEST = GMT+2)]

Perhaps it's not so bad since .show() is already deprecated, they're just gearing up for removal:
https://hg.mozilla.org/integration/mozilla-inbound/rev/597004bec637#l24.12

So we have a bit of time to adapt our code.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: One example

I think this is what we need to do. I picked an easy example. Due to the high volume of use of nsIFilePicker.show(), we'll be here for a while.

Comment 8

[Frank-Rainer Grahl (:frg)]

Good. Just looked at it briefly and thought it would actually removed today.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: One example (v2).

More like this, I think.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Frank-Rainer Grahl from comment #8)
> Good. Just looked at it briefly and thought it would actually removed today.
Well, bug 1334975 says: Get rid of nsIFilePicker.show() *use* in gecko.
Most likely used by a ton of add-ons. So this is our wake-up call. If we don't act, we'll be dead in due course. So let's do this now in peace instead of later in a mad rush.

I asked when the function will go: Bug 1334975 comment #23.

I think we all need to understand promises better:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
So perhaps someone can organise a crash course.

Comment 11

[Jorg K (CEST = GMT+2)]

Tooru-san, could you help us out here? Our expertise on promises is sadly low and the people who know (Kent and Joshua) and mostly not available.

Comment 12

[Frank-Rainer Grahl (:frg)]

> I think this is what we need to do.

Won't work. The promise is async and won't return a true/false value as before as far as I understood it. 

I am currently doing something similar in Bug 912031 and without Async.promiseSpinningly I would have needed to refactor everything:

https://bug912031.bmoattachments.org/attachment.cgi?id=8836507

Still not finished with this one.

Comment 13

[Tooru Fujisawa [:arai]]

If the caller needs the value immediately, caller also needs fix.

for "browsePhoto" case, looks like the callers aren't actually using the return value
https://dxr.mozilla.org/comm-central/rev/56f686a5c3c0da25c08d6542149bbe66eafca5c3/mail/components/addrbook/content/abCardOverlay.xul#470
https://dxr.mozilla.org/comm-central/rev/56f686a5c3c0da25c08d6542149bbe66eafca5c3/suite/mailnews/addrbook/abCardOverlay.xul#407

I don't think the event handler for simple button changes the behavior depending on the return value.
(if it's an apply button or something, it would tho)

Comment 14

[Frank-Rainer Grahl (:frg)]

There is already an old Bug 796994 for suite. Maybe the WIP patch there can be used as a base for mail. I didn't look at it yet but it has problems and the creator is no longer around and had not finished it.

Comment 15

[Magnus Melin [:mkmelin]]

Well the patch in bug bug 796994 uses Promise.defer which is obsolete.

For the cases where we don't care about the return value things shouldn’t be that hard to switch over, just a bunch of work.

Comment 16

[Magnus Melin [:mkmelin]]

I could work on this, unless somebody else wants to.

Comment 17

[Jorg K (CEST = GMT+2)]

I'm happy for you to work on this, but we should raise the knowledge of promise-based solutions in the team as per my private message.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8839370 [details] [diff] [review]
One example (v2).

Looks like my patch here makes no sense after all. Before the function was synchronous and returned a boolean value. Surely returning a promise now makes no sense since the caller doesn't expect any.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1341211-abCardOverlay-MsgComposeCommands.patch - three examples

OK, in the simplest of cases where sync execution is not necessary and no one needs to look at the result of the operation, the conversion is simply this. Tested and working ;-)

By the looks of it, each usage needs to be looked at individually, but in the end, it doesn't look so bad.

Magnus, feel free to integrate the patch into your work. I just wanted to ascertain the scope of the problem. No interference intended.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1341211-abCardOverlay-MsgComposeCommands.patch - three examples

(removed trailing spaces and unused variable)

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: bug1341211_nsIFilePicker-show.patch

Philipp, do you want to review this?

The /editor changes are basically untested (as I don't build seamonkey), but part of them could be in use somewhere in thunderbird. It's hard to say with /edtitor files...

It touches a lot of really ugly ooold code, and indention was partly way off, so I had to reformat to get a readable format. I'll attach a diff -w next.

Comment 22

[Magnus Melin [:mkmelin]]

Attachment: bug1341211_nsIFilePicker-show.w.patch

Ignoring white-space changes

Comment 23

[Magnus Melin [:mkmelin]]

Affected places are at least:

 - Preferences | General | Play sound file
 - Preferences | Chat | Play sound file
 - OPML import/export
 - File | Open | Calendar file
 - Calendar [context] | Export calendar
 - Event and Tasks | Export 
 - Event and Tasks | Import
 - Preferences | Calendar | Reminder sound
 - New Event | Attach File | <cloud provider>
( - Editor | Open file )
( - Editor | Save as ) - XXX TODO rework
 - set page background (choose file) 
 - EdDialogCommon.js GetLocalFileURL 
 - (INSTANT BIRD) prefs
 - Open | Saved Message
 - Address Book | Contact | Photo
 - Compose | Attach File | <cloud provider>
 - Compose | Attach File
 - Preferences | Attachments | select Application (types empty on my test profile, didn't test)
 - Preferences | Attachments | Download to <folder>
 - mozmill stuff
 - Account Settings | Server settings | Local directory
 - Tools | Import
 - Compose: Page colors and backgrounds | Choose file
 - Image Properties | Choose File...

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8848870 [details] [diff] [review]
bug1341211_nsIFilePicker-show.w.patch

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

I'm not a peer for editor/, I think it would be best if Jörg would review this. I've made a comment on one thing that popped out to me.

The calendar changes look fine to me, r=philipp on those. I've also looked at the remaining changes and from a code perspective they look fine to me.

::: calendar/base/content/preferences/alarms.js
@@ +78,1 @@
>                                 .createInstance(nsIFilePicker);

Can you align the periods here?

::: editor/ui/composer/content/editor.xul
@@ +31,5 @@
>  <window id="editorWindow"
>          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>          onload="EditorOnLoad()"
>          onunload="EditorShutdown()"
> +        onclose="return await EditorCanClose()"

This probably won't work. The onclose function handler will expect either returning true/false or nothing at all. Right now you are returning a promise.

The workaround would be to return false, then manually close the window when the promise resolves to true.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8848869 [details] [diff] [review]
bug1341211_nsIFilePicker-show.patch

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

I looked through about a third of the file (mostly looking at calendar which I shouldn't look at) and then my head started spinning.

May I suggest to split this into at least five parts: calendar, im, mail, mailnews and editor. Formally no one has peerage everywhere, but more importantly, practically we need to split the work load in between the best reviewers we can get. We also need to get the SM people on board to review and test this. Also practically, you don't want to submit a new 90 KB patch for every little thing that needs tweaking losing comments on other parts in the meantime.

::: calendar/base/content/dialogs/calendar-creation.js
@@ +9,5 @@
>   * provider.
>   */
>  function openLocalCalendar() {
>      const nsIFilePicker = Components.interfaces.nsIFilePicker;
> +    let fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);

Why did you change the variable name here, when below you left 'picker'. That causes unnecessary changes. I would change that back (but then, I'm not a review peer here).

@@ +19,4 @@
>  
> +    fp.open(rv => {
> +        if (rv != nsIFilePicker.returnOK || !fp.file) {
> +           return;

Nit. Calendar uses four spaces.

::: editor/ui/composer/content/ComposerCommands.js
@@ +846,5 @@
> +        resolve(null);
> +      }
> +    });
> +  });
> +  // XXXmagnus: rework PromptForSaveLocation, return this result as promise ?

Hmm, what are you saying here?

Comment 26

[Frank-Rainer Grahl (:frg)]

> +        onclose="return await EditorCanClose()"

I just needed to sync an async call for Bug 912031. Something like this

> +    var entryData = null;
> +
> +    if (aData == "formhistory-add" || aData == "formhistory-change") {
> +      // Use Async.promiseSpinningly to Sync the call.
> +      Async.promiseSpinningly(this._promiseReadFormHistory(cGuid).then(entry => {
> +        if (entry) {
> +          entryData = entry;

from 

https://bug912031.bmoattachments.org/attachment.cgi?id=8848863

should work when you immediately need the return value in a non async function.

Comment 27

[Magnus Melin [:mkmelin]]

(In reply to Philipp Kewisch [:Fallen] from comment #24)
> Comment on attachment 8848870 [details] [diff] [review]
> > +        onclose="return await EditorCanClose()"
> 
> This probably won't work. The onclose function handler will expect either
> returning true/false or nothing at all. Right now you are returning a
> promise.

No, it's returning the value the promise resolves to, after the promise resolves. Without the "await" it would return the promise. But, this needs to be tested. 

FRG: thx for the pointer! Will check out Async.promiseSpinningly if it's needed. If you're building SeaMonkey, can you try out the patch in Editor?

Comment 28

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) from comment #25)
> Comment on attachment 8848869 [details] [diff] [review]
> May I suggest to split this into at least five parts: calendar, im, mail,
> mailnews and editor. Formally no one has peerage everywhere, but more
> importantly, practically we need to split the work load in between the best
> reviewers we can get. We also need to get the SM people on board to review
> and test this. Also practically, you don't want to submit a new 90 KB patch

Pros and cons to both approaches. People can look at relevant parts when reviewing testing. But yes it's a big patch.

> Why did you change the variable name here, when below you left 'picker'.
> That causes unnecessary changes. I would change that back (but then, I'm not
> a review peer here).

It was easier for copy-paste purposes. I'll keep it now, since otherwise I'll have to test it again.

> > +  });
> > +  // XXXmagnus: rework PromptForSaveLocation, return this result as promise ?
> 
> Hmm, what are you saying here?

Leftover comment for my self. Removed now.

Comment 29

[Magnus Melin [:mkmelin]]

Attachment: bug1341211_nsIFilePicker-show.patch

Comment 30

[Frank-Rainer Grahl (:frg)]

>> FRG: thx for the pointer! Will check out Async.promiseSpinningly if it's needed. 

Didn't find much documentation but it works fine for the two cases in the Data Manager patch. 
Could probably even be simplified like here for PlacesUtils.keywords.fetch:

https://dxr.mozilla.org/comm-central/source/mozilla/services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm#439

But making everything async and using await is for sure preferred.

>> If you're building SeaMonkey, can you try out the patch in Editor?

Yes. Can't promise that I manage this week but next weekend for sure.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_cal.patch

r+ from Philipp.

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_editor.patch [landed comment #54]

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_im.patch

Asking Patrick since I haven't seen Aleth in a while.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_mail.patch

A lot of Mozmill stuff here, so Aceman's my man ;-)

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_mailnews.patch

I've split this up. Divide and conquer ;-)

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8848937 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch

Alta88, could you please take a look at the feed changes.

Comment 37

[alta88]

Comment on attachment 8848937 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch

From feed subscriptions dialog: import/export, bad file, cancel all work fine.

From Tools Import, to a new feeds account: after the account is created an incorrect alert "Could not open the file." shows after the file picker opens. The same thing likely happens for mail etc option file pickers.

Comment 38

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8848933 [details] [diff] [review]
bug1341211_nsIFilePicker-show_editor.patch [landed comment #54]

Found some time to look at the editor patch today. This doesn't work in editor.xul:

> +        onclose="return await EditorCanClose()"

Error message is that await is only available in an async function.

Comment 39

[Patrick Cloke [:clokep]]

Comment on attachment 8848934 [details] [diff] [review]
bug1341211_nsIFilePicker-show_im.patch

I'm a bit overwhelmed at the moment. nhnt11 is going to try to take a look at this quicker than me. Setting r? so he remembers. :)

Comment 40

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8848934 [details] [diff] [review]
bug1341211_nsIFilePicker-show_im.patch

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

Thanks for the patch! Please don't forget to update the commit message to indicate the final reviewer of this patch (r=nhnt11)! :)

::: im/content/preferences/applications.js
@@ +939,5 @@
>      // as we handle it specially ourselves.
>      aEvent.stopPropagation();
>  
>      var handlerApp;
> +    let onSelectionDone = function() {

Please use an arrow function here. Not just for cosmetic reasons: there are references to `this` within the function and considering it's called from a Promise callback, we'd better be careful with scope (fwiw I realize the Promise callback itself is an arrow function, but still).

@@ +1003,5 @@
> +      // selection, then add it to the list of possible handlers.
> +
> +      fp.open(rv => {
> +        if (rv == nsIFilePicker.returnOK && fp.file &&
> +            this._isValidHandlerExecutable(fp.file)) {

Nit: This code's readability might be improved by doing something like:

fp.open(rv => {
  if (rv != nsIFilePicker.returnOK || !fp.file ||
      !this._isValidHandlerExecutable(fp.file))
    return;

  handlerApp = Components.classes[...]
  ...
  handlerInfo.addPossibleApplicationHandler(handlerApp);
}).then(() => {
  onSelectionDone();
});

Up to you, what do you think?

Comment 41

[Magnus Melin [:mkmelin]]

(In reply to Frank-Rainer Grahl from comment #38)
> Comment on attachment 8848933 [details] [diff] [review]
> bug1341211_nsIFilePicker-show_editor.patch
> 
> Found some time to look at the editor patch today. This doesn't work in
> editor.xul:
> 
> > +        onclose="return await EditorCanClose()"
> 
> Error message is that await is only available in an async function.

Are you able to check if this works:

onclose="return async function() { return await EditorCanClose(); }"

Comment 42

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Magnus Melin from comment #41)

> Are you able to check if this works:
> 
> onclose="return async function() { return await EditorCanClose(); }"

That would return an async function, which evaluates to true always. Even if you would return (async function() { ...})(), that would return a promise, which also always evaluates to true, even if the fulfillment value is non-truty.

You really need to do something like this, because the onclose handler is synchronous and there is no way around it.


onclose="triggerCanClose(); return false;"

function triggerCanClose() {
  EditorCanClose().then(res => {
    if (res) {
      window.close();
    }
  });
}

(maybe you need to do something different than window.close, I am not sure if that triggers the onclose event again)

Comment 43

[Frank-Rainer Grahl (:frg)]

Attachment: 1341211_nsIFilePicker-show_editor-addenum.patch

Patch on patch. As Fallen pointed out 

> onclose="return async function() { return await EditorCanClose(); }"

didn't work. Making the call synchronous with Async.promiseSpinningly works.

I think I tested everything and didn't find an error elsewhere. 

I changed an if in ComposerCommands.js 
> -      if (rv != nsIFilePicker.returnCancel) {
> +      if (rv == nsIFilePicker.returnOK && fp.file) {

Could also be done with
> +      if (rv != nsIFilePicker.returnCancel && fp.file) {

Pick what you like better. If IanN doesn't find anything else I would give it an r+ and a big thanks :)

Comment 44

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8848933 [details] [diff] [review]
bug1341211_nsIFilePicker-show_editor.patch [landed comment #54]

see privious comment and patch.

Comment 45

[:aceman]

(In reply to Frank-Rainer Grahl from comment #43)
> I changed an if in ComposerCommands.js 
> > -      if (rv != nsIFilePicker.returnCancel) {
> > +      if (rv == nsIFilePicker.returnOK && fp.file) {

I'd prefer this one. Only whitelist know-good values (returnOK), not exclude everything else. Some new return value would ruin the check.

Comment 46

[:aceman]

Comment on attachment 8848935 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mail.patch

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

Thanks for the patch.
I have manually tested most of the filepicker uses and they seem to work. But there are critical problems in the mozmill changes.

::: mail/components/addrbook/content/abCardOverlay.js
@@ +1017,5 @@
>  
> +  fp.open(rv => {
> +    if (rv != nsIFilePicker.returnOK) {
> +      return;
> +    }

Do we need the braces for a one-line block (multiple occurrences)?

::: mail/components/preferences/applications.js
@@ +1626,5 @@
> +    let onSelectionDone = function() {
> +      // Rebuild the actions menu whether the user picked an app or canceled.
> +      // If they picked an app, we want to add the app to the menu and select it.
> +      // If they canceled, we want to go back to their previous selection.
> +      this.rebuildActionsMenu();

I get an error here when choosing a file to handle a mime type:
TypeError: this.rebuildActionsMenu is not a function 1 applications.js:1630:7
	onSelectionDone chrome://messenger/content/preferences/applications.js:1630:7
	chooseApp/< chrome://messenger/content/preferences/applications.js:1700:9

@@ +1648,4 @@
>  
>      if (AppConstants.platform == "win") {
> +      var params = {};
> +      var handlerInfo = this._handledTypes[this._list.selectedItem.type];

You could change to 'let' when reindenting this.

::: mail/test/mozmill/shared-modules/test-attachment-helpers.js
@@ +79,5 @@
>  
>    appendFilter: function gMFP_appendFilter(aTitle, aFilter) {
>    },
>  
>    show: function gMFP_show() {

Is this still needed?

@@ +85,5 @@
>    },
>  
> +  open: function(aFilePickerShownCallback) {
> +    aFilePickerShownCallback(Ci.nsIFilePicker.returnOK);
> +  },

This fails some mozmill tests:
SUMMARY-UNEXPECTED-FAIL | test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_inserts_linebreak_on_empty_compose
  EXCEPTION: [JavaScript Error: "aFilePickerShownCallback is not a function" {file: "resource://mozmill/modules/frame.js -> file:///mail/test/mozmill/shared-modules/test-attachment-helpers.js" line
: 88}]'[JavaScript Error: "aFilePickerShownCallback is not a function" {file: "resource://mozmill/modules/frame.js -> file:///mail/test/mozmill/shared-modules/test-attachment-helpers.js" line: 88}]
' when calling method: [nsIFilePicker::open]
    at: nonesuch line 1523
       attachToCloud MsgComposeCommands.js:1523 3
       subtest_inserts_linebreak_on_empty_compose test-cloudfile-attachment-urls.js:243 3

::: mail/test/resources/mozmill/mozmill/extension/resource/modules/utils.js
@@ +148,5 @@
>     if (/^chrome:\/\//.test(loc)) { return true; }
>     else { return false; }
>  }
>  
> +var runFile = function(w){

Space before {.

@@ +152,5 @@
> +var runFile = function(w){
> +  //define the interface
> +  var nsIFilePicker = Components.interfaces.nsIFilePicker;
> +  var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
> +  //define the file picker window

In openFile() below you remove these comments.

@@ +169,5 @@
> +   //Move focus to output tab
> +   //w.document.getElementById('mmtabs').setAttribute("selectedIndex", 2);
> +   //send it into the JS test framework to run the file
> +   // jstest.runFromFile(thefile.path);
> +  }

Missing ');' ?
This makes all mozmill tests fail...

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 8848933 [details] [diff] [review]
bug1341211_nsIFilePicker-show_editor.patch [landed comment #54]

Clearing my review queue ;-) This needs a new patch, merged with FRG's addendum.

Comment 48

[Jorg K (CEST = GMT+2)]

Comment on attachment 8848937 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch

Clearing my review queue ;-) I think a new patch is required here.

Comment 49

[Jorg K (CEST = GMT+2)]

This is now becoming urgent, see bug 1387800. We must land this before that bug lands. I'll land the Calendar, Editor and Chat parts now (I hope the patches still apply) and then we need to worry about Mail and Mailnews.

Comment 50

[Jorg K (CEST = GMT+2)]

Attachment: 1341211_nsIFilePicker-show_editor-addenum.patch - refreshed [landed comment #54]

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_im.patch - addressed first review comment [landed comment #54]

Comment 52

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_cal.patch - refreshed

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_cal.patch - refreshed, take 2 [landed comment #54]

Renamed 'fp' back to 'picker' in alarms.js and calendar-creation.js to remove needless changes.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d361db344198
Get rid of nsIFilePicker.show() use in c-c (editor part). r=jorgk,frg
https://hg.mozilla.org/comm-central/rev/5406b6f6d665
Get rid of nsIFilePicker.show() use in c-c (editor part, addendum). rs=jorgk
https://hg.mozilla.org/comm-central/rev/4f7178fa17ba
Get rid of nsIFilePicker.show() use in c-c (im part). r=nhnt11
https://hg.mozilla.org/comm-central/rev/12d38a72ca33
Get rid of nsIFilePicker.show() use in c-c (calendar part). r=philipp

Comment 55

[Jorg K (CEST = GMT+2)]

Maybe Alta88 can help us out here with the mailnews patch since this is becoming urgent.

Comment 56

[Jorg K (CEST = GMT+2)]

The good news is that landing those four patches didn't create a test failure:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=12d38a72ca33d238188d10157529efc0875749a1

The red B's have a different reason and the Z was there before, bug 1387704.

So now we need to get the mail/ and mailnews/ parts done before bug 1387800 strikes.

Comment 57

[Magnus Melin [:mkmelin]]

Will have a look soon.

Comment 58

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #45)
> (In reply to Frank-Rainer Grahl from comment #43)
> > I changed an if in ComposerCommands.js 
> > > -      if (rv != nsIFilePicker.returnCancel) {
> > > +      if (rv == nsIFilePicker.returnOK && fp.file) {
> 
> I'd prefer this one. Only whitelist know-good values (returnOK), not exclude
> everything else. Some new return value would ruin the check.

My original patch is correct. You should check for exactly nsIFilePicker.returnCancel in this case. In case you choose an existing file and confirm to replace that returnReplace is returned instead. I'll include a change to fix that.

Comment 59

[Jorg K (CEST = GMT+2)]

Sorry about that, can you please only attach a patch for that one file to land as a follow-up follow-up :-(

I've vaguely looked over the patches I landed and sometimes nsIFilePicker.returnOK is used, in other cases nsIFilePicker.returnCancel. I couldn't make much sense of it.

So |if (rv != nsIFilePicker.returnCancel  && fp.file) {| isn't correct either?

Comment 60

[Magnus Melin [:mkmelin]]

Attachment: bug1341211_nsIFilePicker-show_mailnews.patch

Working AFAIK, fixed the feed import issue

Comment 61

[Magnus Melin [:mkmelin]]

Attachment: bug1341211_nsIFilePicker-show_mail.patch

Fixed the mentioned problems

Comment 62

[Magnus Melin [:mkmelin]]

Try run in progress - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cecf3332c6de2f93339dc31745812011dd66683a

Comment 63

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #59)
> Sorry about that, can you please only attach a patch for that one file to
> land as a follow-up follow-up :-(

I lazily included it in the mail/ patch.

> 
> I've vaguely looked over the patches I landed and sometimes
> nsIFilePicker.returnOK is used, in other cases nsIFilePicker.returnCancel. I
> couldn't make much sense of it.
> 
> So |if (rv != nsIFilePicker.returnCancel  && fp.file) {| isn't correct
> either?

For that case no, as we bail out in the if.

Comment 64

[Jorg K (CEST = GMT+2)]

Comment on attachment 8894266 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch

This looks great (looked at interdiff), but I'll let Alta88 OK it.

Comment 65

[Jorg K (CEST = GMT+2)]

I cancelled the try run since it will fail badly. At least the mail patch doesn't apply, since nsILocalFile is dead.

Please rebase and send new patches. I will land the editor hunk separately.

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show-editor.patch - addendum follow-up [landed comment #67]

Removing old or rotten patches.

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/959c482d6c7e
Get rid of nsIFilePicker.show() use in c-c (editor part, addendum, follow-up). rs=jorgk DONTBUILD

Comment 68

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_mailnews.patch - refreshed [landed comment #77]

OK, mailnews refreshed.

Comment 69

[Jorg K (CEST = GMT+2)]

Attachment: bug1341211_nsIFilePicker-show_mail.patch - refreshed

OK, I refreshed the patches for you since it's one hour later in Finland ;-)

Comment 70

[Jorg K (CEST = GMT+2)]

New rebased try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=13fd8cc0dfeb471fe098a78b0f5d4e2aa9fc32ea

Like the service?

Comment 71

[Jorg K (CEST = GMT+2)]

Alta88: Interdiff is really rotten on those patches. If you want to see what Magnus changed, please use:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8848937&action=interdiff&newid=8894266&headers=1
After my refresh interdiff stopped working.

Comment 72

[Jorg K (CEST = GMT+2)]

The try run has some failures:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cloudfile/test-cloudfile-attachment-item.js | test-cloudfile-attachment-item.js::test_upload_cancel_repeat
and four in
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/downloads/test-about-downloads.js

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/quick-filter-bar/test-filter-logic.js
on Mac only is a known failure, see bug 1387704.

Comment 73

[:aceman]

The test-about-downloads.js uses the gMockFilePicker from test-attachment-helpers.js, maybe it needs some more adaptations.

Comment 74

[Jorg K (CEST = GMT+2)]

Non-Mac consistently shows:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cloudfile/test-cloudfile-attachment-item.js
and four in
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/downloads/test-about-downloads.js

Comment 75

[:aceman]

Comment on attachment 8894271 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mail.patch - refreshed

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

For some reason, in applications.js, AppConstants is undefined on line 1274.
This happens when clicking "use other..." in the action for a content type.

Comment 76

[alta88]

Comment on attachment 8894270 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch - refreshed [landed comment #77]

Everything seems to work fine.

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/06c8fe51d683
Get rid of nsIFilePicker.show() use in c-c (mailnews part). r=alta88 DONTBUILD

Comment 78

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #74)
> Non-Mac consistently shows:
> TEST-UNEXPECTED-FAIL |
> /builds/slave/test/build/tests/mozmill/cloudfile/test-cloudfile-attachment-
> item.js
> and four in
> TEST-UNEXPECTED-FAIL |
> /builds/slave/test/build/tests/mozmill/downloads/test-about-downloads.js

I don't seem to reproduce these locally

Comment 79

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #75)
> Comment on attachment 8894271 [details] [diff] [review]
> bug1341211_nsIFilePicker-show_mail.patch - refreshed
> 
> Review of attachment 8894271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For some reason, in applications.js, AppConstants is undefined on line 1274.
> This happens when clicking "use other..." in the action for a content type.

Don't see this either

Comment 80

[Jorg K (CEST = GMT+2)]

mozmake SOLO_TEST=downloads/test-about-downloads.js mozmill-one
fails locally with
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\downloads\test-about-downloads.js | test-about-downloads.js::test_save_attachment_files_in_list
  EXCEPTION: Timeout waiting for saving three attachment files.
    at: utils.js line 408
       TimeoutError utils.js:408 13
       waitFor utils.js:446 11
       MozMillController.prototype.waitFor controller.js:687 3
       test_save_attachment_files_in_list test-about-downloads.js:168 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9

And
mozmake SOLO_TEST=cloudfile/test-cloudfile-attachment-item.js mozmill-one as well.
fails with:
SUMMARY-PASS | test-cloudfile-attachment-item.js::setupModule
SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\mail\test\mozmill\cloudfile\test-cloudfile-attachment-item.js | test-cloudfile-attachment-item.js::test_upload_cancel_repeat
  EXCEPTION: JavaScript component does not have a method named: "show"'JavaScript component does not have a method named: "show"' when calling method: [nsIFilePicker::show]
    at: nonesuch line 4030
       AttachFile MsgComposeCommands.js:4030 7
       test_upload_cancel_repeat test-cloudfile-attachment-item.js:51 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestFile frame.js:534 3
Same as on the server:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-13fd8cc0dfeb471fe098a78b0f5d4e2aa9fc32ea/try-comm-central-win32/try-comm-central_win7_ix_test-mozmill-bm119-tests1-windows-build8.txt.gz

At first I ran it without the patch, and that worked, doh :-(

Comment 81

[:aceman]

(In reply to Magnus Melin from comment #79)
> > For some reason, in applications.js, AppConstants is undefined on line 1274.
> > This happens when clicking "use other..." in the action for a content type.
> 
> Don't see this either

OK, it is on "Application details..." item. And it is not be caused by this patch.
I will fix this in a new bug. There are also other problems.

(In reply to Magnus Melin from comment #78)
> > and four in
> > TEST-UNEXPECTED-FAIL |
> > /builds/slave/test/build/tests/mozmill/downloads/test-about-downloads.js
> 
> I don't seem to reproduce these locally

I can see these 4 failures locally.

Comment 82

[Magnus Melin [:mkmelin]]

Attachment: bug1341211_nsIFilePicker-show_mail.patch

This fixes the test-cloudfile-attachment-item.js issue. 

Unfortunately I notice we have callers in cpp for show() in cpp code too. Very nice :/
https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp#930

I'm not sure which of those we're getting into, but one in messenger is causing the test-about-downloads.js failures

[Exception... "JavaScript component does not have a method named: "show"'JavaScript component does not have a method named: "show"' when calling method: [nsIFilePicker::show]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: chrome://messenger/content/msgHdrViewOverlay.js :: AttachmentInfo_save :: line 1842"  data: no]


There is basically only one other cpp use case in the code base to see how you're supposed to do this in with open and nsIFilePickerShownCallback. https://dxr.mozilla.org/comm-central/source/mozilla/dom/html/HTMLInputElement.cpp#954
Anyone wants to take on this? I'm unlikely to have enough time to do it in the next days.

Comment 83

[Jorg K (CEST = GMT+2)]

Comment on attachment 8896421 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mail.patch

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

::: mail/test/resources/mozmill/mozmill/extension/resource/modules/utils.js
@@ +207,5 @@
> +        // forcing the user to save as a .js file
> +        if (thefile.path.indexOf(".js") == -1) {
> +          // define the file interface
> +          var file = Components.classes["@mozilla.org/file/local;1"]
> +                              .createInstance(Components.interfaces.nsILocalFile);

Sigh, one day you should refresh your local build since nsILocalFile positively won't work any more :-(

Comment 84

[Magnus Melin [:mkmelin]]

Ah yes it's refreshed. Wrongly unbitrotted.

Comment 85

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #82)
> Unfortunately I notice we have callers in cpp for show() in cpp code too.
Six, to be precise.

Looks like M-C still has one, too:
https://dxr.mozilla.org/comm-central/rev/47248637eafa9a38dade8dc3aa6c4736177c8d8d/mozilla/widget/nsBaseFilePicker.cpp#87

Comment 86

[Magnus Melin [:mkmelin]]

Attachment: bug1341211_nsIFilePicker-show_mail.patch [landed comment #91]

s/nsILocalFile/nsIFile/

Comment 87

[Jorg K (CEST = GMT+2)]

Comment on attachment 8896424 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mail.patch [landed comment #91]

mozmake SOLO_TEST=cloudfile/test-cloudfile-attachment-item.js mozmill-one
passed now, as Magnus said.

mozmake SOLO_TEST=downloads/test-about-downloads.js mozmill-one still fails, as Magnus said.

(In reply to Magnus Melin from comment #82)
> Unfortunately I notice we have callers in cpp for show() in cpp code too.
...
> I'm not sure which of those we're getting into, but one in messenger is
> causing the test-about-downloads.js failures
Are you sure? I don't understand that. So far nsIFilePicker::Show() hasn't been removed, so why would some once-working test suddenly fail if it's using the C++ code?

I've shipped this off to another try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=92a983f2d5790c2c20e06c1df7b2a28b08526c32
to check that the additional mozmill changes in this patch don't affect anything else.

BTW, bug 1387800, removal of nsIFilePicker::Show() is now on autoland, to be merged soon, so we will be busted once again. It will be C++ bustage, so we won't even be able to compile.

I can take a look at the C++ changes but I'd appreciate of someone else sorted out the downloads test. Maybe Aceman has some time.

Comment 88

[Jorg K (CEST = GMT+2)]

Attachment: 1341211-cpp-bustage.patch

Patch to temporarily remove calls to nsIFilePicker::Show().

Comment 89

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #87)
> I've shipped this off to another try run:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=92a983f2d5790c2c20e06c1df7b2a28b08526c32
> to check that the additional mozmill changes in this patch don't affect
> anything else.
Only failure in test-about-downloads.js, so that's somewhat positive :-)

Comment 90

[Jorg K (CEST = GMT+2)]

Attachment: 1341211-cpp-part.patch - Patch for AB [landed comment #91]

OK, changing nsIFilePicker::Show() to nsIFilePicker::Open() by adding a callback is really quite simple.

In this WIP I've converted the caller in nsAbManager.cpp. Tested and working.

The recipe:
Add a class which has a 'Done' method. Instantiate this class passing the necessary information to the worker, that Done function. The worker becomes all the code in the original after the Show() call. It's even minimal code change.

Maybe someone wants to review this before I *have* to land it to fix C++ bustage.

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d960dcde18c4
Get rid of nsIFilePicker.show() use in c-c (mail part). r=aceman
https://hg.mozilla.org/comm-central/rev/71eca1701d3a
Temporarily disable test-about-downloads.js. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/f305e8b4e790
Replace nsIFilePicker::Show() with nsIFilePicker::Open() in the address book manager. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/8802479e93e7
Temporary C++ bustage fix since nsIFilePicker::Show() was removed. rs=bustage-fix

Comment 92

[Jorg K (CEST = GMT+2)]

Attachment: 1341211-cpp-bustage.patch [landed comment #91, backed out comment #110]

Comment 93

[Jorg K (CEST = GMT+2)]

Attachment: 1341211-test-bustage.patch [landed comment #91, backed out comment #110]

Comment 94

[Jorg K (CEST = GMT+2)]

M-C just merged so I landed the patches we had so far.

This has become a little confusing, so here is what's left to do:

1) test-about-downloads.js needs to be fixed and the patch that disables the failing test
   needs to be reverted.
2) mailnews/base/src/nsMessenger.cpp needs to be fixed since there I commented out
   nsIFilePicker::Show() in order to be able to compile.

Item 2) is actually quite hard to do. The address book manager conversion was simple, but in nsMessenger.cpp we have five callers. One in nsMessenger::SaveOneAttachment() is easy to convert, the others are hard:

nsMessenger::PromptIfFileExists() is actually delivering a sync result, which needs complete refactoring since the result now arrives async in the callback.
nsMessenger::SaveAllAttachments() may be straight forward, but it calls PromptIfFileExists() in the worker part, so after the Show() call for an added twist :-(
nsMessenger::GetSaveAsFile() is hard since it's meant to return a sync result, so complete refactoring.
nsMessenger::GetSaveToDir() is the same, returning sync result.

So we will have a Daily where attachments and other things can't be saved. Fortunately on Windows, drag & drop is working ;-)

Comment 95

[:aceman]

(In reply to Jorg K (GMT+2) from comment #87)
> (In reply to Magnus Melin from comment #82)
> > Unfortunately I notice we have callers in cpp for show() in cpp code too.
> ...
> > I'm not sure which of those we're getting into, but one in messenger is
> > causing the test-about-downloads.js failures
> Are you sure? I don't understand that. So far nsIFilePicker::Show() hasn't
> been removed, so why would some once-working test suddenly fail if it's
> using the C++ code?

My analysis says that Magnus converted the mockFilePicker (overlaying the real filePicker code) to provide open() but the C++ backend in SaveOneAttachment() still called show(). So the mock code never got run and the test failed. Now you have disabled the C++ function (it returns failure) so the test will fail again.
I have confirmed the theory by reverting the hunk in the mockFilePicker to use show() again an a build still having show() support in m-c. The test then passes.
 
> I can take a look at the C++ changes but I'd appreciate of someone else
> sorted out the downloads test. Maybe Aceman has some time.

Please fix the C++ code and the test may start to work.

Comment 96

[:aceman]

Comment on attachment 8896645 [details] [diff] [review]
1341211-cpp-bustage.patch [landed comment #91, backed out comment #110]

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

::: mailnews/base/src/nsMessenger.cpp
@@ +860,5 @@
>    if (NS_SUCCEEDED(rv) && lastSaveDir)
>      filePicker->SetDisplayDirectory(lastSaveDir);
>  
> +  // Temporary bustage fix, see bug 1341211 - nsMessenger::SaveOneAttachment()
> +  // rv = filePicker->Show(&dialogReturn);

There was no dialogReturn variable in this function, you copied this from elsewhere.

@@ +933,5 @@
>    if (NS_SUCCEEDED(rv) && lastSaveDir)
>      filePicker->SetDisplayDirectory(lastSaveDir);
>  
> +  // Temporary bustage fix, see bug 1341211 - nsMessenger::SaveAllAttachments()
> +  // rv = filePicker->Show(&dialogReturn);

There was no dialogReturn variable in this function, you copied this from elsewhere.

@@ +1259,5 @@
>      filePicker->SetDisplayDirectory(lastSaveDir);
>  
>    nsCOMPtr<nsIFile> localFile;
> +  // Temporary bustage fix, see bug 1341211 - nsMessenger::GetSaveAsFile()
> +  // rv = filePicker->Show(&dialogReturn);

There was no dialogReturn variable in this function, you copied this from elsewhere.

@@ +1350,5 @@
>      filePicker->SetDisplayDirectory(lastSaveDir);
>  
>    int16_t dialogResult;
> +  // Temporary bustage fix, see bug 1341211 - nsMessenger::GetSaveToDir()
> +  // rv = filePicker->Show(&dialogReturn);

There was no dialogReturn variable in this function, you copied this from elsewhere.

Comment 97

[:aceman]

Comment on attachment 8896424 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mail.patch [landed comment #91]

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

I now think this is correct.

::: mail/test/mozmill/shared-modules/test-attachment-helpers.js
@@ +80,5 @@
>    appendFilter: function gMFP_appendFilter(aTitle, aFilter) {
>    },
>  
> +  open: function(aFilePickerShownCallback) {
> +    aFilePickerShownCallback.done(Ci.nsIFilePicker.returnOK);

This change may be needed, but it needs that also the nsMessenger.cpp::SaveOneAttachment() switches to open() for the test to work. We will see if the change is right when that C++ function is converted.

Comment 98

[:aceman]

Comment on attachment 8896639 [details] [diff] [review]
1341211-cpp-part.patch - Patch for AB [landed comment #91]

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

Thanks, exporting an addressbook from the addressbook window works for me. It opens the filepicker and saves the file (e.g. csv).

::: mailnews/addrbook/src/nsAbManager.h
@@ +64,5 @@
>      }
>    };
>  
> +  class nsFilePickerShownCallback
> +    : public nsIFilePickerShownCallback

What mess is needed to get a bit of async that complicates everything.

Comment 99

[:aceman]

Comment on attachment 8896424 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mail.patch [landed comment #91]

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

::: mail/components/preferences/downloads.js
@@ +30,2 @@
>        folderListPref.value = this._fileToIndex(file);
>      }

Unfortunately this landed with missing closing ');' here... General and Attachments panes are broken. I'll fix it in 1388882.

::: mail/components/preferences/general.js
@@ +152,5 @@
>        // convert the nsIFile into a nsIFile url
>        document.getElementById("mail.biff.play_sound.url").value = fp.fileURL.spec;
>        this.readSoundLocation(); // XXX We shouldn't have to be doing this by hand
>        this.updatePlaySound();
> +    };

Unfortunately this landed with missing ');' here... I'll fix it in 1388882.

Comment 100

[Jorg K (CEST = GMT+2)]

Can we please fix it here, so keep all the patches together.

Comment 101

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c91c7c77fa2e
Get rid of nsIFilePicker.show() use in c-c (mail part, follow-up). r=aceman DONTBUILD

Comment 102

[:aceman]

Sure, thanks.

Comment 103

[Magnus Melin [:mkmelin]]

Comment on attachment 8896639 [details] [diff] [review]
1341211-cpp-part.patch - Patch for AB [landed comment #91]

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

Looks ok to me, thx!

Comment 104

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #95)
> Please fix the C++ code and the test may start to work.
I thought I had a good idea by implementing a sync function 
  nsMessenger::ShowPicker(nsIFilePicker *aPicker, short *aResult)
to save ourselves the hassle of refactoring everything. So far no luck but I haven't given up yet.

Comment 105

[Jorg K (CEST = GMT+2)]

Magnus, do you have a good idea what to do here. As per my comment #94, only the code in nsMessenger::SaveOneAttachment() is easy to convert. Last night I looked at nsMessenger::GetSaveAsFile() which is expected to sync-return a result. It calls Show(), so all the processing in the function after the call and all the processing in the caller would have to go into the callback. Looking at the caller nsMessenger::SaveAs(), that's a nightmare since it also calls PromptIfFileExists() which has another Show() call. Same for nsMessenger::SaveAllAttachments() where Show() is followed by PromptIfFileExists() in the same function.

To avoid a complete refactoring, I tried to get a sync nsMessenger::ShowPicker() going, but failed so far.

Comment 106

[Magnus Melin [:mkmelin]]

Sorry, I don't have any easy ideas. Async in cpp is such a pain :(
Likely you'll have to refactor quite a lot to make it work.

Comment 107

[alta88]

Doesn't it seem like these messenger.saveAs type calls (all from .js in a very brief look) using the .cpp filepicker should just be converted to .js methods..?

Comment 108

[Jorg K (CEST = GMT+2)]

Thanks, but I have a C++ solution for now.

Comment 109

[Jorg K (CEST = GMT+2)]

Attachment: 1341211-messenger.patch [landed comment #110]

Comment 110

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3aee71e7e200
Backed out changeset 71eca1701d3a for being a temporary fix. a=backout
https://hg.mozilla.org/comm-central/rev/91182177f8e7
Backed out changeset 8802479e93e7 for being a temporary fix. a=backout
https://hg.mozilla.org/comm-central/rev/36bd1eaceefb
Provide sync picker method in nsMessenger.cpp. r=aceman

Comment 111

[Jorg K (CEST = GMT+2)]

I should mention that the sync picker was my idea, but Joshua Cranmer pointed me to:
https://dxr.mozilla.org/comm-central/rev/d2b81af84925c478d9a7c2ef888351e1ba9b5fce/mailnews/mapi/mapihook/src/msgMapiHook.cpp#345
while :smaug (Olli Pettay) had also mentioned spinning the event loop when talking to him on IRC.

The last failing test also passes, so we're done here \o/ !!

Comment 112

[alta88]

(In reply to Jorg K (GMT+2) from comment #108)
> Thanks, but I have a C++ solution for now.

Except continuing to do it using hacky cpp workarounds for a user interaction that has no business being in cpp any longer is extremely inferior to converting it to .js, unless all that is just talk.

Comment 113

[Jorg K (CEST = GMT+2)]

Agreed. But our product was broken since you couldn't save a thing. Not too bad on Windows, where there is drag&drop, but that doesn't seem to work on Linux. Feel free to file a bug and implement a replacement. Under bustage pressure the only thing I can do is plug the hole.

Comment 114

[:aceman]

Comment on attachment 8897169 [details] [diff] [review]
1341211-messenger.patch [landed comment #110]

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

Thanks, the test also passes for me locally.
Maybe JS version would be preferable, but there are also addons calling this as messenger.* . Submitting clever JS hacks is welcome.

::: mailnews/base/src/nsMessenger.cpp
@@ -1,5 @@
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>  /* 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/. */
> -

Why removing this blank line? Did you mean the line between prsystem.h and nsMessenger.h ?

@@ +316,5 @@
> +  mPickerDone = true;
> +  return NS_OK;
> +}
> +
> +nsresult nsMessenger::ShowPicker(nsIFilePicker *aPicker, short *aResult)

Why 'short' and int16_t and not nsresult? Is this decided in m-c? Still you could return nsresult from ShowPicker, it is our helper.

@@ +321,5 @@
> +{
> +  nsCOMPtr<nsIFilePickerShownCallback> callback =
> +    new nsMessenger::nsFilePickerShownCallback();
> +  nsFilePickerShownCallback *cb =
> +    static_cast<nsFilePickerShownCallback*>(callback.get());

Why is this a normal pointer and the one above is nsCOMPtr?

Comment 115

[Jorg K (CEST = GMT+2)]

1) I noticed the blank line and restored it in the landed version.
2) 'short' was the original data type for the result
   https://hg.mozilla.org/comm-central/rev/36bd1eaceefb0e11a64ac72870faedabd3a88315#l1.82
   https://hg.mozilla.org/mozilla-central/rev/6dacffdd0ed1#l15.19
   It's actually not a result value, but an integer:
   https://dxr.mozilla.org/comm-central/rev/47248637eafa9a38dade8dc3aa6c4736177c8d8d/mozilla/widget/nsIFilePicker.idl#34
   Accidentally I mixed 'short' and 'int16_t', but they are the same.
3) The nsCOMPtr points to an interface class, in this case nsIFilePickerShownCallback.
   That class must inherit from nsISupports and come with all bells and whistles, like having methods
   Addref() and Releast().
   callback->mPickerDone doesn't compile since nsIFilePickerShownCallback doesn't have such a member.
   'cb' is just a handy shortcut to access the public members of the callback object we create.
   'callback' will be destroyed when the function exits, then 'cb' will be stale, but after the
   function exits, we don't care. In the function, it will be safe to use.

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/20658ec6d032
Follow-up for nsMessenger.cpp: Use int16_t instead of short. r=me DONTBUILD