1327784 - When I save JSON files, Firefox doesn't autofills file name into save dialog

Status: VERIFIED FIXED

(DevTools :: JSON Viewer, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url [1] or [2] or [3]
2. Click "Save" button in JSON viewer

AR:  Browser opens native save dialog, but there's no file name in field "File name"
ER:  Browser should autofill file name, just like it always does when I save file

It's already too hard to save JSON files, so I'm forced to click "Save" button instead of Ctrl+S
But now it turns out that "Save" button doesn't work as well, and now I have to close modal save
dialog, copy file name or some part of url (if file doesn't have a name), then [probably make
the same mistake - Ctrl+S instead of "Save"] paste file name in modal save dialog

> [1] http://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.map
> [2] https://mail.yandex.ru/neo2/handlers/handlers3.jsx?_h=do-settings,settings
> [3] https://www.dropbox.com/subscribe?host_int=0&trace=%2Fhome&user_id=112673760&nid=0&ns_map=182477297_13678%2C214117373_1205

Comment 1

[Ankita Tanwar]

Hi,

I would like to work on this bug but is it possible to provide some information on where to start.

Thanks,
Ankita

Comment 2

[Daisuke Akatsuka (:daisuke)]

I talked with Ankita on irc.
I'm not familiar with JSON Viewer.
Please ask to triage owner.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ankita Tanwar from comment #1)
> Hi,
> 
> I would like to work on this bug but is it possible to provide some
> information on where to start.
Sounds great!

1) Here is the place where onSave handler is implemented. It's executed when the user clicks on the 'Save' button. 
https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/devtools/client/jsonview/main.js#52

2) `getTargetFile` is implemented here:
https://dxr.mozilla.org/mozilla-central/rev/7ef1e9abd296a8edc39b7efc8d637767ba2f77ed/devtools/client/jsonview/utils.js#25

It open and initializes the 'Save as' dialog. It should also be responsible for setting the file name.

See this MDN page:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFilePicker

It should be possible to set the file name through `defaultString` attribute.

3) Also, I noticed that `show()` is deprecated and we should probably use `open()` instead.

Should I assign the bug to you?

Honza

Comment 4

[Ankita Tanwar]

Hello Jan.

You can assign this bug to me. I would love to work on it.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, forgot to do that, done now!

Honza

Comment 6

[Saghan]

Hi Jan,
 I talked to Ankita, we are working on this together. I put console.log() in main.js and utils.js on the lines you have mentioned. It does not get displayed in console. The file json-viewer.js seems to be involved. onSaveJson() is called from there. This calls window.dispatchEvent() with custom event. I couldn't find which file contains the definition of this method. The comment in file says "It is handled in chrome scope". What does this mean? Is there a systematic way to see call hierarchy of javascript files? 
-Thanks in advance.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Saghan from comment #6)
> Hi Jan,
>  I talked to Ankita, we are working on this together.
We were discussing similar issues on IRC today with Anktita so, all questions might be answered now.

Honza

Comment 8

[Ankita Tanwar]

Attachment: Bug_1327784_1.patch

Assigning the document title to "defaultString" parameter of "nsIFilePicker" solves the problem.

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8848282 [details] [diff] [review]
Bug_1327784_1.patch

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

I was testing the patch on my machine and when saving JSON
from this url: https://api.github.com/ the default file name is:
https---api.github.com- - Nightly

When using `File -> Save Page As...` the default file name is: 
https _api.github.com_.htm


Could we make it the same?

You can search the code base and find how Firefox Save As dialog does it.
See http://searchfox.org/

Thanks!

Honza

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Any progress on this bug?
It would be so great to have this landed!

Honza

Comment 11

[Ankita Tanwar]

Hi Honza,

Apologies for the late response.
Lately, I haven't been able to work on this bug due to workload. I will start working on it again and submit a fix asap.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ankita Tanwar from comment #11)
> Hi Honza,
> 
> Apologies for the late response.
> Lately, I haven't been able to work on this bug due to workload. I will
> start working on it again and submit a fix asap.
Thanks for the update!

Thanks for working on this, I am looking forward
to check out the patch.
Honza

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

See also bug 1349437 comment #12

Honza

Comment 14

[Oriol Brufau [:Oriol]]

Instead of producing a custom save dialog, which has various problems:
 - Name not autofilled
 - Save location is globaly remembered, not by domain
 - Downloaded file does not appear in Firefox's Downloads

maybe the save button should just trigger native save (bug 1349437)?

I think it would be something like

    XPCOMUtils.defineLazyGetter(this, "ContentAreaUtils", function() {
      let ContentAreaUtils = {};
      Services.scriptloader.loadSubScript("chrome://global/content/contentAreaUtils.js", ContentAreaUtils);
      return ContentAreaUtils;
    });
    // ...
    ContentAreaUtils.saveBrowser(gBrowser.selectedBrowser);

Bug 1349437 comment #12 would need to be fixed but I did not reproduce that.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Oriol from comment #14)
> Instead of producing a custom save dialog, which has various problems:
>  - Name not autofilled
>  - Save location is globaly remembered, not by domain
>  - Downloaded file does not appear in Firefox's Downloads
> 
> maybe the save button should just trigger native save (bug 1349437)?
> 
> I think it would be something like
> 
>     XPCOMUtils.defineLazyGetter(this, "ContentAreaUtils", function() {
>       let ContentAreaUtils = {};
>      
> Services.scriptloader.loadSubScript("chrome://global/content/
> contentAreaUtils.js", ContentAreaUtils);
>       return ContentAreaUtils;
>     });
>     // ...
>     ContentAreaUtils.saveBrowser(gBrowser.selectedBrowser);
> 
> Bug 1349437 comment #12 would need to be fixed but I did not reproduce that.

Sounds good to me!

Honza

Comment 16

[Oriol Brufau [:Oriol]]

Attachment: json-native-save.patch

I meant something like this. The problem is that it always saves the original JSON, never the prettified one.

Ankita, Jan, any idea? I thought about opening a new tab, load the desired contents in there, save that tab, and close it before the dialog appears (hopefully it will be asynchroneous). But seems too hacky.

Comment 17

[Oriol Brufau [:Oriol]]

Attachment: json-native-save-v2.patch

Ankita, I hope you don't mind if I steal this bug :)

Handling prettified JSON was a bit messy. I managed to do it by storing the data in a blob url and downloading that. The blob is created in the content process so that when you close the tab with the json, the url is revoked and the blob can be garbage collected. The filename is obtained from the document, not the blob.

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8865309 [details] [diff] [review]
json-native-save-v2.patch

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

Thanks for the patch!

Please see my inline comments

Honza

::: devtools/client/jsonview/main.js
@@ +65,5 @@
> +      // - Uses the given blob URL containing the custom contents to save.
> +      // - Obtains the file name from the URL of the document, not the blob.
> +      let persistable = browser.QueryInterface(Ci.nsIFrameLoaderOwner)
> +        .frameLoader.QueryInterface(Ci.nsIWebBrowserPersistable);
> +      let stack = components.stack.caller;

Please move the 'stack' variable into `onError` handler. Also the, error handling feels a bit hacky. Could we just use: new Error() ?

@@ +69,5 @@
> +      let stack = components.stack.caller;
> +      persistable.startPersistence(message.data.windowID, {
> +        onDocumentReady(doc) {
> +          let uri = chrome.makeURI(doc.documentURI, doc.characterSet);
> +          let filename = chrome.getDefaultFileName(undefined, uri, doc, null);

When I switch off the JSON View and press Ctrl+S (Win10) to save the page I am seeing that the default file name is 'api.github.com'.

In case of the JSON Viewer enabled, I am seeing: 'https _api.github.com_'. Why the difference?

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Ankita: what do you think about the patch?

Honza

Comment 20

[Oriol Brufau [:Oriol]]

(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> Please move the 'stack' variable into `onError` handler. Also the, error
> handling feels a bit hacky. Could we just use: new Error() ?

I just copied the code from saveBrowser. I also think Error makes more sense.
Maybe Error is not defined in that scope, I will check later, but chrome.Error should work.

> When I switch off the JSON View and press Ctrl+S (Win10) to save the page I
> am seeing that the default file name is 'api.github.com'.
> 
> In case of the JSON Viewer enabled, I am seeing: 'https _api.github.com_'.
> Why the difference?

That's because bug 1362268 hadn't landed yet :)

Comment 21

[Oriol Brufau [:Oriol]]

Attachment: json-native-save-v3.patch

Removed stack and used Error.

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8865477 [details] [diff] [review]
json-native-save-v3.patch

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

Looks great, thanks for working on this!

R+

Honza

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd31968cb8d7
Let JSON Viewer save using standard saving methods from contentAreaUtils.js. r=honza

Comment 24

[Carsten Book [:Tomcat]]

backed out from https://treeherder.mozilla.org/logviewer.html#?job_id=98010657&repo=mozilla-inbound

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c30e21ceb7
Backed out changeset dd31968cb8d7 for eslint failure

Comment 26

[Oriol Brufau [:Oriol]]

Attachment: json-native-save-v4.patch

Replacing Blob with window.Blob
Damn linter...

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8866417 [details] [diff] [review]
json-native-save-v4.patch

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

Yep, fixes the eslint error for me.

Thanks,
Honza

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c43489fc05f
Let JSON Viewer save using standard saving methods from contentAreaUtils.js. r=honza

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c43489fc05f

Comment 30

[Md. Rahimul Islam]

I have successfully reproduced the bug in firefox Nightly 53.0a1 (2017-01-01) (32-bit) with windows 10 (32 bit) 

Verified as fixed with latest nightly 55.0a1 (2017-05-31) (32-bit)

Build ID:   20170531030204
Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0

Comment 31

[Md.Tarikul Islam Oashi]

I have reproduced this bug with nightly 53.0a1 (2017-01-01) (64-bit) on "Linux Mint 18.1 Serena"(64 Bit).

The bug's fix is now verified on Latest nightly 55.0a1.

Build ID 	20170530100155
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[Bugday-20170531]

Comment 32

[Md.Tarikul Islam Oashi]

As per Comment 30 & Comment 31, I am marking this bug as verified fixed