1244265 - JSON Viewer: save button does nothing

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

via bug 1223143 comment 7

> - The Save buttons (in the JSON and Raw Data tabs) don’t seem to work.

Maybe we should drop the save button?  It either has to work or we need to get rid of it.

Comment 1

[David Walsh :davidwalsh]

Attachment: MozReview Request: Bug 1244265 - Show file prompt upon save button click. r?Honza

Review commit: https://reviewboard.mozilla.org/r/46795/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46795/

Comment 2

[David Walsh :davidwalsh]

https://reviewboard.mozilla.org/r/46795/#review43367

Could you advise on any standards I've missed?  I also didn't include a test -- let me know if you have ideas for a good way to test the save dialog opening and closing.

Comment 3

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

Comment on attachment 8741858 [details]
MozReview Request: Bug 1244265 - Show file prompt upon save button click. r?Honza

https://reviewboard.mozilla.org/r/46795/#review43751

Thanks for looking at this!

Honza

::: devtools/client/jsonview/main.js:6
(Diff revision 1)
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>  /* 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/. */
> -/* globals JsonViewUtils*/
>  

We need the comment to avoid eslint warning about not defined global

::: devtools/client/jsonview/main.js:20
(Diff revision 1)
> +                               "devtools/client/jsonview/utils");
> +
>  XPCOMUtils.defineLazyGetter(this, "JsonViewService", function() {
>    return require("devtools/client/jsonview/utils");
>  });
>  

So, the only thing that needs to be fixed here is replacing the:

XPCOMUtils.defineLazyGetter(this, "JsonViewService", function() {

with:

XPCOMUtils.defineLazyGetter(this, "JsonViewUtils", function() {

... and all should be good.
(it was a typo)

Honza

Comment 4

[David Walsh :davidwalsh]

Comment on attachment 8741858 [details]
MozReview Request: Bug 1244265 - Show file prompt upon save button click. r?Honza

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46795/diff/1-2/

Comment 5

[David Walsh :davidwalsh]

Ahh, OK, cool.  Updated!

Comment 6

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

(In reply to David Walsh :davidwalsh from comment #2)
> https://reviewboard.mozilla.org/r/46795/#review43367
> 
> Could you advise on any standards I've missed?  I also didn't include a test
> -- let me know if you have ideas for a good way to test the save dialog
> opening and closing.
Some of those might be useful:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Asave%20path%3Abrowser%20path%3Atest&case=true

Honza

Comment 7

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

Comment on attachment 8741858 [details]
MozReview Request: Bug 1244265 - Show file prompt upon save button click. r?Honza

https://reviewboard.mozilla.org/r/46795/#review43759

Thanks!

Honza

Comment 8

[Tim Nguyen :ntim]

It'd be nice to have a test for this, so this bug doesn't happen again.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0587870803e7

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out in  https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=f0210b36ff71 since of this patches caused a bustage like :

https://hg.mozilla.org/integration/fx-team/rev/ee2fff382352d57eeb2c7b5ae35c8eaa254e486e

seems the error is ==============================
 05:54:52     INFO -  The error occurred while processing the following file or one of the files it includes:
 05:54:52     INFO -      /builds/slave/fx-team-m64-000000000000000000/build/src/devtools/client/jsonview/css/moz.build
 05:54:52     INFO -  The error occurred when validating the result of the execution. The reported error is:
 05:54:52     INFO -      File listed in FINAL_TARGET_FILES does not exist: /builds/slave/fx-team-m64-000000000000000000/build/src/devtools/client/jsonview/css/twisty-closed.svg
 05:54:52     INFO -  *** Fix above errors and then restart with               "/usr/bin/make -f client.mk build"
05:54:52 INFO - make[3]: *** [conf

Comment 11

[David Walsh :davidwalsh]

Attachment: MozReview Request: Bug 1244265 - Add test for save button prompt in JSONView. r?Honza

Review commit: https://reviewboard.mozilla.org/r/49019/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49019/

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/6804430a4707

Comment 13

[David Walsh :davidwalsh]

Attachment: MozReview Request: Bug 1244265 - Show file picker when save button clicked within the JSONView component

Review commit: https://reviewboard.mozilla.org/r/49029/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49029/

Comment 14

[David Walsh :davidwalsh]

Updated to include test!

Comment 15

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

Looks good to me!

The only thing needed is fixing eslint errors.

@pbro: is it ok to put new `MockFilePicker` global into devtools/.eslint.mochitest file?

Honza

Comment 16

[Patrick Brosset <:pbro>]

In fact it would be better to use
let MockFilePicker = SpecialPowers.MockFilePicker;

because SpecialPowers is already a defined global in our browser mochitest config.
(note that the devtools mochitest config only adds a few things, but mostly extends from testing/mochitest/browser.eslintrc: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser.eslintrc )

Comment 17

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

@pbro: thanks!

@davidwalsh: Please add `let MockFilePicker = SpecialPowers.MockFilePicker;` into the test file and I'll r+.

Honza

Comment 18

[Tim Nguyen :ntim]

(In reply to Jan Honza Odvarko [:Honza] from comment #17)
> @pbro: thanks!
> 
> @davidwalsh: Please add `let MockFilePicker = SpecialPowers.MockFilePicker;`
> into the test file and I'll r+.
> 
> Honza

Better yet, with destructuring:
let {MockFilePicker} = SpecialPowers;

Comment 19

[David Walsh :davidwalsh]

Comment on attachment 8745406 [details]
MozReview Request: Bug 1244265 - Show file picker when save button clicked within the JSONView component

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49029/diff/1-2/

Comment 20

[David Walsh :davidwalsh]

Updated as requested; please confirm everything is as it should be.  Thank you!

Comment 21

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

Comment on attachment 8745406 [details]
MozReview Request: Bug 1244265 - Show file picker when save button clicked within the JSONView component

https://reviewboard.mozilla.org/r/49029/#review46287

Great job here, thanks!

Honza

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cfbf0743aefd

Comment 23

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

And an eslint fix as a followup https://hg.mozilla.org/integration/fx-team/rev/f7bb3675bb71

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cfbf0743aefd
https://hg.mozilla.org/mozilla-central/rev/f7bb3675bb71