Closed Bug 1341211 Opened 3 years ago Closed 2 years ago

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

Categories

(MailNews Core :: Import, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: frg, Assigned: mkmelin)

References

Details

Attachments

(11 files, 18 obsolete files)

21.44 KB, patch
Details | Diff | Splinter Review
2.89 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
7.83 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
12.65 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
1.11 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
20.02 KB, patch
alta88
: review+
jorgk
: feedback+
Details | Diff | Splinter Review
30.90 KB, patch
aceman
: review+
Details | Diff | Splinter Review
7.80 KB, patch
aceman
: review+
mkmelin
: review+
Details | Diff | Splinter Review
4.24 KB, patch
Details | Diff | Splinter Review
2.92 KB, patch
Details | Diff | Splinter Review
7.10 KB, patch
aceman
: review+
Details | Diff | Splinter Review
+++ 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
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.
Here comes more:
https://dxr.mozilla.org/comm-central/search?q=filepicker.show&redirect=false
this time also showing the files you mentioned.
Sorry that is actually how I checked it :) Should have put the query in.
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
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
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.
Attached patch One example (obsolete) — Splinter Review
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.
Good. Just looked at it briefly and thought it would actually removed today.
Attached patch One example (v2). (obsolete) — Splinter Review
More like this, I think.
Attachment #8839365 - Attachment is obsolete: true
(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.
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.
Flags: needinfo?(arai.unmht)
> 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.
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)
Flags: needinfo?(arai.unmht)
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.
See Also: → 796994
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.
I could work on this, unless somebody else wants to.
Assignee: nobody → mkmelin+mozilla
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 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.
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.
Attachment #8839370 - Attachment is obsolete: true
Attachment #8840033 - Flags: feedback?(mkmelin+mozilla)
(removed trailing spaces and unused variable)
Attachment #8840033 - Attachment is obsolete: true
Attachment #8840033 - Flags: feedback?(mkmelin+mozilla)
Attachment #8840036 - Flags: feedback?(mkmelin+mozilla)
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.
Attachment #8848869 - Flags: review?(philipp)
Ignoring white-space changes
Attachment #8840036 - Attachment is obsolete: true
Attachment #8840036 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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 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.
Attachment #8848870 - Flags: review+
Attachment #8848869 - Flags: review?(philipp)
Attachment #8848869 - Flags: review?(jorgk)
Attachment #8848869 - Flags: review+
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?
Attachment #8848869 - Flags: review?(jorgk)
> +        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.
(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?
(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.
Attachment #8848869 - Attachment is obsolete: true
Attachment #8848923 - Flags: review?(jorgk)
Attachment #8848923 - Flags: feedback?(frgrahl)
>> 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.
r+ from Philipp.
Attachment #8848923 - Attachment is obsolete: true
Attachment #8848923 - Flags: review?(jorgk)
Attachment #8848923 - Flags: feedback?(frgrahl)
Attachment #8848932 - Flags: review+
Asking Patrick since I haven't seen Aleth in a while.
Attachment #8848934 - Flags: review?(clokep)
A lot of Mozmill stuff here, so Aceman's my man ;-)
Attachment #8848935 - Flags: review?(acelists)
I've split this up. Divide and conquer ;-)
Attachment #8848937 - Flags: review?(jorgk)
Comment on attachment 8848937 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch

Alta88, could you please take a look at the feed changes.
Attachment #8848937 - Flags: review?(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.
Attachment #8848937 - Flags: review?(alta88) → review-
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 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. :)
Attachment #8848934 - Flags: review?(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?
Attachment #8848934 - Flags: review?(nhnt11)
Attachment #8848934 - Flags: review?(clokep)
Attachment #8848934 - Flags: review+
(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(); }"
Flags: needinfo?(frgrahl)
(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)
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 :)
Flags: needinfo?(frgrahl)
Attachment #8853922 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 8848933 [details] [diff] [review]
bug1341211_nsIFilePicker-show_editor.patch [landed comment #54]

see privious comment and patch.
Attachment #8848933 - Flags: feedback?(frgrahl) → feedback+
(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 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...
Attachment #8848935 - Flags: review?(acelists) → feedback+
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.
Attachment #8848933 - Flags: review?(jorgk)
Comment on attachment 8848937 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch

Clearing my review queue ;-) I think a new patch is required here.
Attachment #8848937 - Flags: review?(jorgk)
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.
Depends on: 1387800
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
Attachment #8853922 - Attachment is obsolete: true
Attachment #8853922 - Flags: feedback?(mkmelin+mozilla)
Attachment #8894185 - Flags: review+
Attachment #8848932 - Attachment is obsolete: true
Attachment #8894189 - Flags: review+
Renamed 'fp' back to 'picker' in alarms.js and calendar-creation.js to remove needless changes.
Attachment #8894189 - Attachment is obsolete: true
Attachment #8894195 - Flags: review+
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Maybe Alta88 can help us out here with the mailnews patch since this is becoming urgent.
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.
Will have a look soon.
Flags: needinfo?(mkmelin+mozilla)
(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.
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?
Flags: needinfo?(acelists)
Working AFAIK, fixed the feed import issue
Attachment #8848937 - Attachment is obsolete: true
Attachment #8894266 - Flags: review?(jorgk)
Fixed the mentioned problems
Attachment #8848935 - Attachment is obsolete: true
Attachment #8894267 - Flags: review?(jorgk)
(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 on attachment 8894266 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch

This looks great (looked at interdiff), but I'll let Alta88 OK it.
Attachment #8894266 - Flags: review?(jorgk)
Attachment #8894266 - Flags: review?(alta88)
Attachment #8894266 - Flags: feedback+
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.
Removing old or rotten patches.
Attachment #8848870 - Attachment is obsolete: true
Attachment #8894266 - Attachment is obsolete: true
Attachment #8894267 - Attachment is obsolete: true
Attachment #8894266 - Flags: review?(alta88)
Attachment #8894267 - Flags: review?(jorgk)
Attachment #8894269 - Flags: review+
Pushed by mozilla@jorgk.com:
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
OK, mailnews refreshed.
Attachment #8894270 - Flags: review?(alta88)
Attachment #8894270 - Flags: feedback+
OK, I refreshed the patches for you since it's one hour later in Finland ;-)
Attachment #8894271 - Flags: review?(acelists)
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.
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.
The test-about-downloads.js uses the gMockFilePicker from test-attachment-helpers.js, maybe it needs some more adaptations.
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 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 on attachment 8894270 [details] [diff] [review]
bug1341211_nsIFilePicker-show_mailnews.patch - refreshed [landed comment #77]

Everything seems to work fine.
Attachment #8894270 - Flags: review?(alta88) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/06c8fe51d683
Get rid of nsIFilePicker.show() use in c-c (mailnews part). r=alta88 DONTBUILD
(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
(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
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/mozilla@jorgk.com-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 :-(
(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.
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.
Attachment #8894271 - Attachment is obsolete: true
Attachment #8894271 - Flags: review?(acelists)
Attachment #8896421 - Flags: review?(acelists)
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 :-(
Ah yes it's refreshed. Wrongly unbitrotted.
(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
s/nsILocalFile/nsIFile/
Attachment #8896421 - Attachment is obsolete: true
Attachment #8896421 - Flags: review?(acelists)
Attachment #8896424 - Flags: review?(acelists)
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.
Flags: needinfo?(acelists)
Attached patch 1341211-cpp-bustage.patch (obsolete) — Splinter Review
Patch to temporarily remove calls to nsIFilePicker::Show().
(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 :-)
Attachment #8848933 - Attachment description: bug1341211_nsIFilePicker-show_editor.patch → bug1341211_nsIFilePicker-show_editor.patch [landed comment #54]
Attachment #8894185 - Attachment description: 1341211_nsIFilePicker-show_editor-addenum.patch - refreshed → 1341211_nsIFilePicker-show_editor-addenum.patch - refreshed [landed comment #54]
Attachment #8894186 - Attachment description: bug1341211_nsIFilePicker-show_im.patch - addressed first review comment → bug1341211_nsIFilePicker-show_im.patch - addressed first review comment [landed comment #54]
Attachment #8894195 - Attachment description: bug1341211_nsIFilePicker-show_cal.patch - refreshed, take 2 → bug1341211_nsIFilePicker-show_cal.patch - refreshed, take 2 [landed comment #54]
Attachment #8894269 - Attachment description: bug1341211_nsIFilePicker-show-editor.patch - addendum follow-up → bug1341211_nsIFilePicker-show-editor.patch - addendum follow-up [landed comment #67]
Attachment #8894270 - Attachment description: bug1341211_nsIFilePicker-show_mailnews.patch - refreshed → bug1341211_nsIFilePicker-show_mailnews.patch - refreshed [landed comment #77]
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.
Attachment #8896639 - Flags: review?(mkmelin+mozilla)
Attachment #8896639 - Flags: review?(acelists)
Attachment #8896639 - Attachment description: 1341211-cpp-part.patch - WIP → 1341211-cpp-part.patch - Patch for AB
Pushed by mozilla@jorgk.com:
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
Attachment #8896424 - Attachment description: bug1341211_nsIFilePicker-show_mail.patch → bug1341211_nsIFilePicker-show_mail.patch [landed comment #91]
Attachment #8896639 - Attachment description: 1341211-cpp-part.patch - Patch for AB → 1341211-cpp-part.patch - Patch for AB [landed comment #91]
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 ;-)
Flags: needinfo?(acelists)
(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 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 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.
Attachment #8896424 - Flags: review?(acelists) → review+
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.
Attachment #8896639 - Flags: review?(acelists) → review+
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.
Can we please fix it here, so keep all the patches together.
Depends on: 1388882
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c91c7c77fa2e
Get rid of nsIFilePicker.show() use in c-c (mail part, follow-up). r=aceman DONTBUILD
No longer depends on: 1388882
Sure, thanks.
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!
Attachment #8896639 - Flags: review?(mkmelin+mozilla) → review+
(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.
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.
Flags: needinfo?(mkmelin+mozilla)
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.
Flags: needinfo?(mkmelin+mozilla)
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..?
Thanks, but I have a C++ solution for now.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
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/ !!
Target Milestone: --- → Thunderbird 57.0
Attachment #8896645 - Attachment description: 1341211-cpp-bustage.patch [landed comment #91] → 1341211-cpp-bustage.patch [landed comment #91, backed out comment #110]
Attachment #8896646 - Attachment description: 1341211-test-bustage.patch [landed comment #91] → 1341211-test-bustage.patch [landed comment #91, backed out comment #110]
Attachment #8897169 - Attachment description: 1341211-messenger.patch → 1341211-messenger.patch [landed comment #110]
(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.
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 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?
Attachment #8897169 - Flags: review?(acelists) → review+
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.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/20658ec6d032
Follow-up for nsMessenger.cpp: Use int16_t instead of short. r=me DONTBUILD
Depends on: 1395252
You need to log in before you can comment on or make changes to this bug.