1349437 - System Save command tries to save HTML for JSON viewer

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement)

Description

[:Harald Kirschner :digitarald]

I wanted to save a JSON file I had open in the viewer with CMD-S. The dialog offers me to save a HTML file, while it should offer to save the JSON file.

This works for other file viewers, like PDF.

Comment 1

[Oriol Brufau [:Oriol]]

For PDF, this was done in https://github.com/mozilla/pdf.js/commit/04599bc4
I think JSON Viewer needs something similar.

Comment 2

[Oriol Brufau [:Oriol]]

Attachment: save-json.patch

Based on https://github.com/mozilla/pdf.js/commit/04599bc4

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: save-json.patch

Reworded the comment, somebody could think "should" meant it was something to be implemented.

Comment 4

[Oriol Brufau [:Oriol]]

Attachment: save-json.patch

Previous patch kinda worked but I noticed the mime type had already been changed to application/vnd.mozilla.json.view, so the save dialog didn't show the proper extension.

I moved to code to converter-observer.js

Comment 5

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

Comment on attachment 8863528 [details] [diff] [review]
save-json.patch

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

Thanks for working on this!

But, I am still seeing ".htm" extension.

My STR:
1) Load https://api.github.com/
2) Press Ctrl+S (I am on Win10)
3) Dialog appears, the default file name is: https _api.github.com_.htm -> BUG

Honza

::: devtools/client/jsonview/converter-observer.js
@@ +91,5 @@
> +      let responseType = request.contentType;
> +      if (JSON_TYPES.includes(responseType)) {
> +        // Let "save as" save the original JSON, not the viewer
> +        request.QueryInterface(Ci.nsIWritablePropertyBag);
> +        request.setProperty('contentType', responseType);

nit: please use quotation marks => "contentType"

Comment 6

[Oriol Brufau [:Oriol]]

Attachment: screenshot with patch

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> But, I am still seeing ".htm" extension.
> 
> My STR:
> 1) Load https://api.github.com/
> 2) Press Ctrl+S (I am on Win10)
> 3) Dialog appears, the default file name is: https _api.github.com_.htm ->
> BUG

I don't understand, it works for me, I am also on Win10. And this kind of fix worked for PDF.

Comment 7

[Oriol Brufau [:Oriol]]

Ah, I get it, my patch only fixes the problem for non-e10s. Will search how it was done for PDFs in the e10s case.

Comment 8

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

(In reply to Oriol from comment #7)
> Ah, I get it, my patch only fixes the problem for non-e10s. Will search how
> it was done for PDFs in the e10s case.
Yes, that make sense. I've been testing with e10s on.

Honza

Comment 9

[Oriol Brufau [:Oriol]]

OK, the problem is that this fix must be done in the content process:

    request.QueryInterface(Ci.nsIWritablePropertyBag);
    request.setProperty("contentType", responseType);

onStartRequest in converter-child.js is in the content process, but it's too late because the content type has already been replaced by "application/vnd.mozilla.json.view".

That's why I moved it to getMIMETypeFromContent in converter-observer.js, but that's chrome process! Does not work in e10s.

I don't know much about browser internals and multiprocess, so I think the best solution is hardcoding "application/json" in converter-child.js. The original type might also be "application/manifest+json", but probably not often, and anyways downloading proper contents with wrong type seems better than the current behavior of downloading wrong contents with wrong type. A follow-up bug can be filed to handle that case properly.

Is this reasonable?

Comment 10

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

(In reply to Oriol from comment #9)
> OK, the problem is that this fix must be done in the content process:
> 
>     request.QueryInterface(Ci.nsIWritablePropertyBag);
>     request.setProperty("contentType", responseType);
> 
> onStartRequest in converter-child.js is in the content process, but it's too
> late because the content type has already been replaced by
> "application/vnd.mozilla.json.view".
> 
> That's why I moved it to getMIMETypeFromContent in converter-observer.js,
> but that's chrome process! Does not work in e10s.
> 
> I don't know much about browser internals and multiprocess, so I think the
> best solution is hardcoding "application/json" in converter-child.js. The
> original type might also be "application/manifest+json", but probably not
> often, and anyways downloading proper contents with wrong type seems better
> than the current behavior of downloading wrong contents with wrong type. A
> follow-up bug can be filed to handle that case properly.
> 
> Is this reasonable?
Yes, let's try it.

Honza

Comment 11

[Oriol Brufau [:Oriol]]

Attachment: save-json-v2.patch

Comment 12

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

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

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

I am not seeing `.json` file extension for the default file name,
but the content is stored properly.

R+

We might solve the default file name issue as part of bug 1327784

Thanks for working on this!
Honza

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62f440a4a655
Add "save as" support for JSON Viewer. r=honza

Comment 14

[Oriol Brufau [:Oriol]]

(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> I am not seeing `.json` file extension for the default file name,
> but the content is stored properly.

I get the `.json` extension both with and without e10s, as shown in attachment 8863793 [details].
Do you get it when saving with devtools.jsonview.enabled=false ?

Comment 15

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

(In reply to Oriol from comment #14)
> (In reply to Jan Honza Odvarko [:Honza] from comment #12)
> > I am not seeing `.json` file extension for the default file name,
> > but the content is stored properly.
> 
> I get the `.json` extension both with and without e10s, as shown in
> attachment 8863793 [details].
> Do you get it when saving with devtools.jsonview.enabled=false ?

I am loading https://api.github.com/
My OS: Win10

1) devtools.jsonview.enabled=true
default file name: https _api.github.com_
(without the patch: https _api.github.com_.htm)

2) devtools.jsonview.enabled=false
default file name: api.github.com
(without the patch: api.github.com)

Honza

Comment 16

[Oriol Brufau [:Oriol]]

I get this, also in Win10

1) devtools.jsonview.enabled=false
default file name: api.github.com.json
default file type: (*.json)

2) devtools.jsonview.enabled=true before patch
default file name: https _api.github.com_.html
default file type: Web Page, complete (*.htm;*.html)

3) devtools.jsonview.enabled=true after patch
default file name: https _api.github.com_.json
default file type: (*.json)

4) devtools.jsonview.enabled=true after patch altered to set "application/jsonnnnn"
default file name: https _api.github.com_
default file type: com_ File (*.com_)

(Yesterday my file type for application/json was "JavaScript Object Notation (*.json)" instead of just "(*.json)". Not sure why it changed)

It seems your browser or computer doesn't recognize application/json, so the .json extension is not enforced. But I guess that if you attempt to save some url ending with .json (e.g. https://a.4cdn.org/boards.json), the filename will have .json extension.

I don't know if Firefox has its own configuration about this, or if it depends on Windows.

Comment 17

[Oriol Brufau [:Oriol]]

OK, the difference in names depending on `devtools.jsonview.enabled` is because `document.title` is the empty string with `devtools.jsonview.enabled=false`, but the url with `devtools.jsonview.enabled=true`. `https _api.github.com_` seems a weird name, so we can use `win.document.title = "";` to obtain `api.github.com`.

Then, the extension is obtained like this:

    Components.classes["@mozilla.org/mime;1"]
    .getService(Components.interfaces.nsIMIMEService)
    .getFromTypeAndExtension("application/json", "")
    .primaryExtension; // "json"

You can run the code in the browser console. Since you don't have the .json extension, it should throw.
The code is in https://dxr.mozilla.org/mozilla-central/rev/4a6a71f4aa22e4dc3961884ce505ce34bdd799a2/uriloader/exthandler/nsExternalHelperAppService.cpp#2589-2704
First it asks the OS, then searches Firefox datastore, finally asks extras.
In my case, the former two fail, but the extras have it: https://dxr.mozilla.org/mozilla-central/rev/4a6a71f4aa22e4dc3961884ce505ce34bdd799a2/uriloader/exthandler/nsExternalHelperAppService.cpp#548
My guess is that you have that mime type wrongly configured in your OS.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62f440a4a655

Comment 19

[Oriol Brufau [:Oriol]]

Sorry, wrong bug