854299 - Update DownloadLastDir.getFile usage to nsIContentPrefService2

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

DownloadLastDir.getFile is a synchronous API that uses nsIContentPrefService, so it will require some larger changes in order to change it to the asynchronous nsIContentPrefService2

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: WIP - the good parts

Work in progress with the parts that are working for the change in DownloadLastDir and its usage from contentAreaUtils

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 728842 [details] [diff] [review]
WIP - the good parts

some stuff here is about to change

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Prompt, take 1 -- not used

Just posting this patch with the previous approach I was going for, but in the end we decided not to use it. The approach was to make nsExternalHelperAppService call contentPrefs->get pref to "warm up" the service cache, so that later DownloadLastDir could pull the data from the cache instead of waiting for an async read.
But we decided not to use it since this breaks the abstraction created by nsIHelperAppDialog and couples the impl to an implementation detail of nsHelperAppDlg.js, which makes things ugly.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch for uriloader/exthandler parts

bz: nsExternalHelperAppService indirectly used contentPrefs through:
SaveToDisk -> PromptForSaveToFile -> nsHelperAppDlg.promptForSaveToFile -> gDownloadLastDir.getFile

however, the new nsIContentPrefService2 API is async and this callstack need to be made flexible to handle that. What the patch does is:

- Creates a PromptForSaveToFileAsync that implementors of nsIHelperAppLauncherDialog might choose to implement. In this function, instead of returning the chosen file, the callback nsIHelperAppLauncher::fileChosenThroughPicker must be called after the work is finished
- Make SaveToDisk not directly call nsIHelperAppLauncherDialog::PromptForSaveToFile. Instead it calls PromptUserToChooseFileDest which may call the async version
- After everything is done, ContinueSave is then called

The patch also maintains compat with the sync version of PromptForSaveToFile to not break other embeddors or addons.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

The code to make nsHelperAppDlg.js/DownloadLastDir.jsm to actually use the new contentPref api is not part of that patch.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: DownloadLastDir.getFile

This adds a new function to DownloadLastDir, getFileAsync, which uses the new contentPrefs API to retrieve the values and get them async'ly.
DownloadLastDir.setFile is also converted.

The old sync function, DownloadLastDir.getFile is maintained to not break add-ons that are using it, but it operates in "fallback" mode that only access the in-memory cache, so it can return wrong values if they're not cached.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: contentAreaUtils

And this is the change to contentAreaUtils to use the new DownloadLastDir.getFileAsync API from the previous patch.

Since a lot of this patch is wrapping up some functions in a callback and indenting them, here's a `hg diff -w` to make it easier to see the actual changes:
http://www.pastebin.mozilla.org/2252703

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: nsHelperAppDlg.promptForSaveToFile

And this is the change to nsHelperAppDlg to use the new async API.

`hg qdiff -w` for simplicity: http://www.pastebin.mozilla.org/2252794

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730342 [details] [diff] [review]
Patch for uriloader/exthandler parts

>-      rv = PromptForSaveToFile(getter_AddRefs(fileToUse), mTempLeafName, mTempFileExtension);
>+      PromptUserToChooseFileDest(mTempLeafName, mTempFileExtension);

Why not keep the function name PromptForSaveToFile?  Nothing says the user is what will respond... ;)

>+  nsresult ContinueSave(nsIFile *);

Document when this gets called (and in particular that it never gets called with a null argument).

>+  void fileChosenThroughPicker(in nsIFile aFile);

Maybe saveDestinationAvailable?  Might want some sr on the API changes here...

>+   * in aLauncher (aLauncher.fileChosenThroughPikcer) is called with the selected

Either way, s/Pikcer/Picker/

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730545 [details] [diff] [review]
nsHelperAppDlg.promptForSaveToFile

r=me.  Thank you for the diff -w!

Comment 11

[:Felipe Gomes (needinfo for replies!)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> >-      rv = PromptForSaveToFile(getter_AddRefs(fileToUse), mTempLeafName, mTempFileExtension);
> >+      PromptUserToChooseFileDest(mTempLeafName, mTempFileExtension);
> 
> Why not keep the function name PromptForSaveToFile?  Nothing says the user
> is what will respond... ;)

This nsExternalHelperAppService.h function being named the same as the function defined at nsIHelperAppLauncherDialog.idl (but unrelated interfaces) was making things very confusing for me, so that's why I wanted to rename it.
Any suggestions for a better name? Maybe I should drop the "User" part of the name

> >+  void fileChosenThroughPicker(in nsIFile aFile);
> 
> Maybe saveDestinationAvailable?  Might want some sr on the API changes
> here...

Sure, will rename it and request sr

Comment 12

[:Felipe Gomes (needinfo for replies!)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> >+  nsresult ContinueSave(nsIFile *);
> 
> Document when this gets called (and in particular that it never gets called
> with a null argument).

Done. Also, a small change if it's okay: I think it'd be better to change NS_PRECONDITION to NS_ENSURE_ARG_POINTER at the beginning of ContinueSave

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch for uriloader/exthandler parts. r=bz

Roc, could you take a look at the API changes here as suggested by bz?

In summary, to async'ify nsExternalAppHandler::SaveToDisk, this patch splits it in two parts, and creates an async function promptForSaveToFileAsync, and a corresponding callback to be called when that function is done (saveDestinationAvailable), which then calls the second part of SaveToDisk to continue the process.

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 730528 [details] [diff] [review]
DownloadLastDir.getFile

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

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +155,5 @@
> +      handleResult: function(aResult) result = aResult,
> +      handleCompletion: function(aReason) {
> +        let file = plainPrefFile;
> +        if (aReason == Components.interfaces.nsIContentPrefCallback2.COMPLETE_OK &&
> +           result instanceof Components.interfaces.nsIContentPref) {

If result is not null, it should always be instanceof nsIContentPref.  Did you find a bug in content prefs, or is this just a way of checking if result is null?

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 730528 [details] [diff] [review]
DownloadLastDir.getFile

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

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +102,5 @@
>      gDownloadLastDirFile = null;
>    },
>    getFile: function (aURI) {
>      if (aURI && isContentPrefEnabled()) {
> +      // This function, when called with aURI, is now deprecated because

I wonder if it would be better to keep using the sync content pref service in this case, since this method is now deprecated, just as the sync service is.  I think one of the deals with deprecation in general is, "we won't change it, but it's going away soon, so use this new better thing."  What do you think?

This comment should probably be above the method too so that it better stands out to hypothetical people reading the code because they want to use this API.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 730532 [details] [diff] [review]
contentAreaUtils

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

Thanks for the diff -w.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

(In reply to Drew Willcoxon :adw from comment #14)
> If result is not null, it should always be instanceof nsIContentPref.  Did
> you find a bug in content prefs, or is this just a way of checking if result
> is null?

No bug, that was just my way to add the check for not null

> I wonder if it would be better to keep using the sync content pref service
> in this case, since this method is now deprecated, just as the sync service
> is.  I think one of the deals with deprecation in general is, "we won't
> change it, but it's going away soon, so use this new better thing."  What do
> you think?

That's a good point, if Services.contentPrefs is sticking around for compat then there's probably no reason to change the behavior of this function too, as long as nothing in the tree calls it. Perhaps I should revert it.

Comment 18

[Drew Willcoxon :adw]

(In reply to :Felipe Gomes from comment #17)
> Perhaps I should revert it.

Yeah, I think so.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Can we not just get rid of the synchronous DownloadLastDir getter now? Or is that just something for a followup?

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #19)
> Can we not just get rid of the synchronous DownloadLastDir getter now? Or is
> that just something for a followup?

Probably not, mxr shows various add-ons that are currently using it

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Any suggestions for a better name?

"AskForDestinationFile"?  "RequestDestinationFile"?  "RequestSaveDestination"?

That last might work well with "saveDestinationAvailable", actually.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> I think it'd be better to change NS_PRECONDITION to NS_ENSURE_ARG_POINTER

I can live with that, I guess...

Comment 23

[:Felipe Gomes (needinfo for replies!)]

(In reply to Boris Zbarsky (:bz) from comment #22)
> > I think it'd be better to change NS_PRECONDITION to NS_ENSURE_ARG_POINTER
> 
> I can live with that, I guess...

I mentioned because I just thought NS_ENSURE_ARG_POINTER would be more appropriate, but if NS_PRECONDITION is better I can keep it, no problem.

RequestSaveDestination sounds the best name, I'll change it to that.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Felipe Gomes from comment #20)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #19)
> > Can we not just get rid of the synchronous DownloadLastDir getter now? Or is
> > that just something for a followup?
> 
> Probably not, mxr shows various add-ons that are currently using it

Hrm, a lot of those hits seem to be files copied wholesale. Seems like we should still try; we can do that in a followup bug though.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: Test changes

Comment 26

[:Felipe Gomes (needinfo for replies!)]

I changed the function name to SaveDestinationAvailable, went back to using NS_PRECONDITION, added some more documentation and updated the tests related to DownloadLastDir to support the async api.

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c4adb0c295
https://hg.mozilla.org/integration/mozilla-inbound/rev/0abac8d9c9a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e63c771fec44
https://hg.mozilla.org/integration/mozilla-inbound/rev/201be2cf5d88
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c72d5cfe1fb

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hoping to see a green try run before pushing it again...

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c804845016a
https://hg.mozilla.org/integration/mozilla-inbound/rev/929ea187b775

Comment 28

[[e-mail]]

Comment on attachment 730545 [details] [diff] [review]
nsHelperAppDlg.promptForSaveToFile

>-    if (picker.show() == nsIFilePicker.returnCancel) {
>-      // null result means user cancelled.
>-      aLauncher.fileChosenThroughPicker(null);
>-      return;
>-    }
>+      if (picker.show() == nsIFilePicker.returnCancel) {
>+        // null result means user cancelled.
>+        aLauncher.fileChosenThroughPicker(null);
>+        return;
>+      }
You missed your chance to switch to picker.open() ;-)

Comment 29

[:Felipe Gomes (needinfo for replies!)]

Attachment: Change to picker.open

Change to picker.open, which also incidentally fixes the test that failed in the previous push

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 731995 [details] [diff] [review]
Change to picker.open

the patch to change to picker.open got rid of the test problem everywhere except on Windows: https://tbpl.mozilla.org/?tree=Try&rev=5f8166b2a202

So I had to find a different way to fix the test itself that doesn't need that change. It seems to have worked: https://tbpl.mozilla.org/?tree=Try&rev=34c425dda435, and now going again for a full try run to verify: https://tbpl.mozilla.org/?tree=Try&rev=d54ba5608029

I probably could keep the picker.open change together here but since it won't be necessary and I have been testing things without it, I won't add more churn to this bug and will leave that for a follow-up

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Green on try

https://hg.mozilla.org/integration/mozilla-inbound/rev/b560513ee661
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd044aa4a1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee33c2d4374
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae681d69430
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe664dd25564

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b560513ee661
https://hg.mozilla.org/mozilla-central/rev/1fd044aa4a1f
https://hg.mozilla.org/mozilla-central/rev/4ee33c2d4374
https://hg.mozilla.org/mozilla-central/rev/4ae681d69430
https://hg.mozilla.org/mozilla-central/rev/fe664dd25564

Comment 33

[[e-mail]]

Comment on attachment 731995 [details] [diff] [review]
Change to picker.open

(Just for completeness, the patch looks fine, except that it's very ugly to have to bind this twice just to get at validateLeafName; perhaps it should be moved somewhere where it's more easily accessible.)