Closed Bug 1009056 Opened 7 years ago Closed 7 years ago

Web Audio Editor inspector width should be a preference

Categories

(DevTools Graveyard :: Web Audio Editor, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: jsantell, Assigned: yashmehrotra95, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 9 obsolete files)

To store the width that the user sets it, like the debugger sidepanel's width
No longer blocks: 1022248
Mentor: jsantell
Whiteboard: [good first bug][lang=js]
hi, I would like to work on this bug. I have already set up my firefox dev environment. Can you please help me get started with this bug.
Hello Yash! Thanks for checking out this bug. Some background information:

The inspector view can be found in ./browser/devtools/webaudioeditor/views/inspector.js

We'll need to do 2 things:

* Set the inspector width to whatever value this preference is when it starts up (done in the initialize() function, currently setting to the hardcoded INSPECTOR_WIDTH value.
* On resize (onResize() function), we should set the preference to the new width of the inspector (which is stored as this.el in the inspector view)

For setting and storing the pref, we can use the Services global:

Services.prefs.setIntPref(PREFNAME, value);
Services.prefs.getIntPref(PREFNAME);

For a pref name, lets go with `devtools.webaudioeditor.inspectorWidth`, which will have to be set in `./browser/app/profile/firefox.js`. By default, this should be the value of INSPECTOR_WIDTH in the current tools.

Let me know if you have any questions, and hop on our irc channel #devtools where me or someone else can help!
Assignee: nobody → yashmehrotra95
Testing will be tricky, but there are lots of tests currently (browser/devtools/webaudioeditor/test/) that should get you to the right spot. Make sure you add the new test file to the browser.ini in that directory
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Hello Yash! Thanks for checking out this bug. Some background information:
> 
> The inspector view can be found in
> ./browser/devtools/webaudioeditor/views/inspector.js
> 
> We'll need to do 2 things:
> 
> * Set the inspector width to whatever value this preference is when it
> starts up (done in the initialize() function, currently setting to the
> hardcoded INSPECTOR_WIDTH value.
> * On resize (onResize() function), we should set the preference to the new
> width of the inspector (which is stored as this.el in the inspector view)
> 
> For setting and storing the pref, we can use the Services global:
> 
> Services.prefs.setIntPref(PREFNAME, value);
> Services.prefs.getIntPref(PREFNAME);
> 
> For a pref name, lets go with `devtools.webaudioeditor.inspectorWidth`,
> which will have to be set in `./browser/app/profile/firefox.js`. By default,
> this should be the value of INSPECTOR_WIDTH in the current tools.
> 
> Let me know if you have any questions, and hop on our irc channel #devtools
> where me or someone else can help!

Ok, Thanks Jordan. I think I understood most of what you said, I will be submitting a patch within 3-4 days.
Attached patch 1009056.diff (obsolete) — Splinter Review
I have submitted a patch in accordance to what you said. I haven't tested it yet as I wanted to know whether this is good in theory or any other changes are to be made?
Attachment #8561216 - Flags: feedback?(jsantell)
Yash, looking great! Just make sure that the pref that's being set is the width of the inspector, not the element itself.

Also an edge case to look for: is it possible to make the inspector so small that it's hard to see it? Maybe we should set a minimum width during the initialization, so if that this happens, when a user reopens it's set to 300 or so.
Attachment #8561216 - Flags: feedback?(jsantell) → feedback+
Attached patch Patch Updated (obsolete) — Splinter Review
Hi, I updated the patch with some minor fixes and also included the edge case you were mentioning. Is it good enough or some other changes also need to be made ?
Attachment #8561216 - Attachment is obsolete: true
Attachment #8562728 - Flags: review?(jsantell)
Comment on attachment 8562728 [details] [diff] [review]
Patch Updated

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

Looks great! Just some fixes in there, as it will not currently run due to `pref` being undefined. But this is the idea, and I think we're good for tests now. Should be straight forward to test, ensure that sliding the inspector changes the pref (may be hard to do in a test, so can fake by calling _onResize), and that when restarting the tool, the width of the inspector is the same as what is set in the pref. Let me know if you have any questions!

::: browser/devtools/webaudioeditor/views/inspector.js
@@ -4,5 @@
>  "use strict";
>  
> -// Store width as a preference rather than hardcode
> -// TODO bug 1009056
> -const INSPECTOR_WIDTH = 300;

Let's leave this const here as `MIN_INSPECTOR_WIDTH` so we don't have magic numbers later (with 300 just showing up)

@@ +27,5 @@
>    initialize: function () {
>      // Set up view controller
>      this.el = $("#web-audio-inspector");
>      this.splitter = $("#inspector-splitter");
> +    this.el.setAttribute("width", pref("devtools.webaudioeditor.inspectorWidth"));

`pref` is not defined here. You'll want Services.prefs.getIntPref and Services.prefs.setIntPref

@@ +144,5 @@
>      this.show();
>    },
>  
>    _onResize: function () {
> +    if (this.el.getAttribute("width") < 300 ) {

Use MIN_INSPECTOR_WIDTH here
Attachment #8562728 - Flags: review?(jsantell)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8)

> `pref` is not defined here. You'll want Services.prefs.getIntPref and
> Services.prefs.setIntPref
> 

For the above, is pref("foo","bar"); correct for firefox.js ?
I wasn't sure how the Services.prefs work, so now I am assuming I need to write - Services.prefs.setIntPref("devtools.webaudioeditor.inspectorWidth", MIN_INSPECTOR_WIDTH);

I will be submitting a new patch fixing this.
For firefox.js, that is correct -- this creates the preference in about:config.

For the code in the inspector, `Services.prefs` has methods to set and get different types of preferences like, setIntPref and getIntPref, which would be used here since we're dealing with an integer pref for the width.

Yes, anywhere you access or set the preference, you should use the Services.prefs methods. Thanks!
Attached patch 1009056.diff - Updated (obsolete) — Splinter Review
Hi, updated it. Thanks for helping me out. Are any more changes required ?
Attachment #8562728 - Attachment is obsolete: true
Attachment #8562898 - Flags: review?(jsantell)
Looks great! Do you think you can write a test for this, mentioned in comment #8? Adding a test file in ./browser/devtools/webaudioeditor/test/, adding it to browser.ini there, and instructions for running tests here:
https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests
Comment on attachment 8562898 [details] [diff] [review]
1009056.diff - Updated

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

Ping me for a review once there are tests, let me know if you need any help with them!
Attachment #8562898 - Flags: review?(jsantell)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13)
Ok Jordan, thanks, I have started working on the tests. I am new to this so its taking a bit more time than usual, I will sending the test patch in 2-3 days. BTW, should I send a different patch for the test or all the changes in one patch ?
Attached file browser_wa_inspector-width.js (obsolete) —
I added a basic test, it changes the width, reloads, and then checks whether the width is the same or not. Are there any other additions that should be made for the test ? Also, if there are mistakes, please tell me where I need to make the changes.
Attachment #8564571 - Flags: feedback?(jsantell)
Comment on attachment 8564571 [details]
browser_wa_inspector-width.js

Thanks Yash! Looks good. Few things:

* The tests and fix should all be in one patch
* You'll have to add the test to browser.ini
* The test probably has to wait for the nodes to register themselves and click on a node to open the inspector after a refresh to test, I don't think this test will pass
* Ensure the test passes, and no other tests break
Attachment #8564571 - Flags: feedback?(jsantell) → feedback+
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #16)
> Comment on attachment 8564571 [details]
> browser_wa_inspector-width.js
> 
> Thanks Yash! Looks good. Few things:
> 
> * The tests and fix should all be in one patch
Ok, will do it.
> * You'll have to add the test to browser.ini
Yes, I added it to browser.ini but forgot to send that.
> * The test probably has to wait for the nodes to register themselves and
> click on a node to open the inspector after a refresh to test, I don't think
> this test will pass
Can you please elaborate on this part, I am not able to comprehend what you mean (code wise)
> * Ensure the test passes, and no other tests break
Ok, sure.
After reloading the page the second time in the test, you should wait for the nodes to be rendered, like you have earlier in the test:

[actors] = yield Promise.all([
  get3(gFront, "create-node"),
  waitForGraphRendered(panelWin, 3, 2)
]);

After that, you might need to open up the inspector to get the width (although I think maybe just the margin is changed when opening)
Attached file browser_wa_inspector-width.js (obsolete) —
The test file is added to the browser.ini so I didn't send that.
I made various to the file according to what you said. Please tell me if any other changes are required. Thanks!
Attachment #8564571 - Attachment is obsolete: true
Attachment #8565071 - Flags: feedback?(jsantell)
Yash, that's the right idea

* currently, there are some syntax errors, and redeclaration of nodeIds (should leave the second `let` out, as you can only use that once per scope for a variable), so the test fails immediately.

* `new_inspector_width` should be camelCase

* Trying out the patch seems like everything's ok!

Next steps are to combine everything into one patch, and clean the tests up so they're passing for one last review
Attachment #8565071 - Flags: feedback?(jsantell)
Attached patch 1009056.diff - Updated (obsolete) — Splinter Review
Hi Jordan, I made the changes accordingly as you mentioned in comment #20.
All the changes are in one patch.
Attachment #8562898 - Attachment is obsolete: true
Attachment #8565071 - Attachment is obsolete: true
Attachment #8566130 - Flags: review?(jsantell)
Comment on attachment 8566130 [details] [diff] [review]
1009056.diff - Updated

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

Still syntax errors in test:

17 INFO TEST-UNEXPECTED-FAIL | browser/devtools/webaudioeditor/test/browser_wa_inspector-width.js | Exception thrown - at chrome://mochitests/content/browser/browser/devtools/webaudioeditor/test/browser_wa_inspector-width.js:9 - SyntaxError: missing { before function body
SUITE-END | took 7s

Be sure to run tests for just this file:

./mach mochitest-devtools browser/devtools/webaudioeditor/test/browser_wa_inspector-width.js

As well as the entire tool:

./mach mochitest-devtools browser/devtools/webaudioeditor/

Everything looks good though, just a matter of actually getting the tests to pass
Attachment #8566130 - Flags: review?(jsantell)
Attached patch 1009056.diff (obsolete) — Splinter Review
Sorry for the syntax errors. Updated the patch.
I wanted to ask you a question regarding firefox building. Sometimes when I update my tree (`hg pull -u`) and after building it, it shows many errors and does not build. To fix it, I change my nodeset to a previous revision (by hit and trial).

What is the best way to fix this, as due to this, 90% of times, I can't test my code.
Thanks!
Attachment #8566130 - Attachment is obsolete: true
Attachment #8566993 - Flags: review?(jsantell)
(In reply to Yash Mehrotra from comment #23)
> Created attachment 8566993 [details] [diff] [review]
> 1009056.diff
> 
> Sorry for the syntax errors. Updated the patch.
> I wanted to ask you a question regarding firefox building. Sometimes when I
> update my tree (`hg pull -u`) and after building it, it shows many errors
> and does not build. To fix it, I change my nodeset to a previous revision
> (by hit and trial).
> 
> What is the best way to fix this, as due to this, 90% of times, I can't test
> my code.
> Thanks!

You should make sure you pop all the patches out first before pulling (hg qpop -a).
If you get errors for building, you can try clobbering (mach clobber) then do a full build (mach build).
Btw, you can always reach #introduction on IRC if you need help.
Also when updating, sometimes you can get away with not having to rebuild everything and clobbering. I use this script for that: https://github.com/jsantell/home/blob/master/bin/reclobber

If making changes to just the web audio tool you can do 
./mach build browser/devtools/webaudioeditor

You shouldn't have to hg pull often as the web audio tools do not have a high rate of patches landing so you should be OK without it
Still errors running the tests -- if you're on IRC, I'm jsantell on there and can help you through the building/testing process
Tried to send this to you on IRC but you logged off and I'll be away for a few minutes, but we do run tests on an online server on all environments, example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=249641b765d3 But they need to pass locally first. If you'd like I can fix the syntax errors if you're having trouble running the tests and push to try, as the testing logic itself works
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #27)
> Tried to send this to you on IRC but you logged off and I'll be away for a
> few minutes, but we do run tests on an online server on all environments,
> example:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=249641b765d3 But
> they need to pass locally first. If you'd like I can fix the syntax errors
> if you're having trouble running the tests and push to try, as the testing
> logic itself works

Thanks a lot, my University provides a **** internet connection and hence was getting disconnected from IRC multiple times.

So, I went through the let documentation on MDN. So, if I substitute let [actors] with var [actors] will it be all right?
Yes, or just omit "let" the second time and it becomes an assignment rather than a declaration
Attached patch 1009056.diff (obsolete) — Splinter Review
I changed `let [actors]` to `[actors] = `
I hope this is good.
Attachment #8566993 - Attachment is obsolete: true
Attachment #8566993 - Flags: review?(jsantell)
Attachment #8567235 - Flags: review?(jsantell)
Attached patch 1009056.diff (obsolete) — Splinter Review
hopefully the final patch.Thanks for the help.
Attachment #8567235 - Attachment is obsolete: true
Attachment #8567235 - Flags: review?(jsantell)
Attachment #8567272 - Flags: review?(jsantell)
Attached patch 1009056.diffSplinter Review
added r=jsantel
Attachment #8567272 - Attachment is obsolete: true
Attachment #8567272 - Flags: review?(jsantell)
Attachment #8567276 - Flags: review?(jsantell)
Comment on attachment 8567276 [details] [diff] [review]
1009056.diff

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

Looks great, all tests are passing for me locally! Sending it up to the try server -- if everything looks good there, we'll get this merged in. Thanks, Yash!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=630c5454e47d
Attachment #8567276 - Flags: review?(jsantell) → review+
https://hg.mozilla.org/integration/fx-team/rev/116ca223e290
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/116ca223e290
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.