Closed
Bug 1244265
Opened 9 years ago
Closed 9 years ago
JSON Viewer: save button does nothing
Categories
(DevTools :: JSON Viewer, defect)
DevTools
JSON Viewer
Tracking
(firefox49 fixed)
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: clarkbw, Assigned: davidwalsh)
References
Details
Attachments
(1 file, 2 obsolete files)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46795/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46795/
Attachment #8741858 -
Flags: review?(odvarko)
Assignee | ||
Comment 2•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8741858 -
Flags: review?(odvarko)
Comment 3•9 years ago
|
||
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
Assignee | ||
Comment 4•9 years ago
|
||
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/
Attachment #8741858 -
Flags: review?(odvarko)
Assignee | ||
Comment 5•9 years ago
|
||
Ahh, OK, cool. Updated!
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
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
Attachment #8741858 -
Flags: review?(odvarko) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 8•9 years ago
|
||
It'd be nice to have a test for this, so this bug doesn't happen again.
Keywords: checkin-needed
Comment 10•9 years ago
|
||
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
Flags: needinfo?(dwalsh)
Assignee | ||
Comment 11•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49019/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49019/
Attachment #8745373 -
Flags: review?(odvarko)
Assignee | ||
Updated•9 years ago
|
Attachment #8741858 -
Attachment is obsolete: true
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49029/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49029/
Assignee | ||
Updated•9 years ago
|
Attachment #8745373 -
Attachment is obsolete: true
Attachment #8745373 -
Flags: review?(odvarko)
Assignee | ||
Updated•9 years ago
|
Attachment #8745406 -
Flags: review?(odvarko)
Comment 15•9 years ago
|
||
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
Flags: needinfo?(pbrosset)
Comment 16•9 years ago
|
||
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 )
Flags: needinfo?(pbrosset)
Comment 17•9 years ago
|
||
@pbro: thanks!
@davidwalsh: Please add `let MockFilePicker = SpecialPowers.MockFilePicker;` into the test file and I'll r+.
Honza
Flags: needinfo?(dwalsh)
Comment 18•9 years ago
|
||
(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;
Assignee | ||
Comment 19•9 years ago
|
||
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/
Attachment #8745406 -
Flags: review?(odvarko)
Assignee | ||
Comment 20•9 years ago
|
||
Updated as requested; please confirm everything is as it should be. Thank you!
Flags: needinfo?(dwalsh)
Updated•9 years ago
|
Attachment #8745406 -
Flags: review?(odvarko)
Comment 21•9 years ago
|
||
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
Attachment #8745406 -
Flags: review?(odvarko) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
Keywords: checkin-needed
And an eslint fix as a followup https://hg.mozilla.org/integration/fx-team/rev/f7bb3675bb71
Comment 24•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cfbf0743aefd
https://hg.mozilla.org/mozilla-central/rev/f7bb3675bb71
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•8 years ago
|
Assignee: nobody → dwalsh
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•