Closed Bug 1240196 Opened 4 years ago Closed 4 years ago

Replace instances of console jsterm inputNode.value with a getInputValue function

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

Eventually we will want to swap out the underlying DOM object (bug 983473) so we shouldn't have to rely on it being an input field.
Comment on attachment 8708630 [details]
MozReview Request: Bug 1240196 - Replace instances of console jsterm inputNode.value with a getInputValue function;r=linclark

Lin, would you mind taking a look at this change when you have a chance?  At some point I'd like to convert the console's input field to not be a XUL textbox so starting to pull out some of the direct access.  I'll file other bugs for other places where DOM APIs are used directly (focus, selection APIs, etc).
Attachment #8708630 - Flags: review?(lclark)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8708630 [details]
MozReview Request: Bug 1240196 - Replace instances of console jsterm inputNode.value with a getInputValue function;r=linclark

https://reviewboard.mozilla.org/r/31123/#review28263

This looks good to me.

::: devtools/client/webconsole/test/browser_webconsole_bug_594497_history_arrow_keys.js:26
(Diff revision 1)
> -  inputNode = values = null;
> +  jsterm = inputNode = values = null;

Should things like this be registered as a cleanup function? That could be done in another issue, though.

::: devtools/client/webconsole/webconsole.js:3867
(Diff revision 1)
> +   * Gets the value from the input field

Should this have an @return comment?
Attachment #8708630 - Flags: review?(lclark) → review+
(In reply to Lin Clark from comment #4)
> Comment on attachment 8708630 [details]
> MozReview Request: Bug 1240196 - Replace instances of console jsterm
> inputNode.value with a getInputValue function;r=linclark
> 
> https://reviewboard.mozilla.org/r/31123/#review28263
> 
> This looks good to me.

Thanks for the review!

> devtools/client/webconsole/test/
> browser_webconsole_bug_594497_history_arrow_keys.js:26
> (Diff revision 1)
> > -  inputNode = values = null;
> > +  jsterm = inputNode = values = null;
> 
> Should things like this be registered as a cleanup function? That could be
> done in another issue, though.

In general, yeah we should put stuff like that in cleanup functions.  My preference is to pass these in as params to individual test helpers rather than keep them as globals that need cleanup.  In this case that's what I'd prefer to do (and it's made a lot easier by add_task).  I'm working through some cleanup on select tests in Bug 1112599, and maybe the bigger project is 'convert all webconsole tests to use add_task'.

> ::: devtools/client/webconsole/webconsole.js:3867
> (Diff revision 1)
> > +   * Gets the value from the input field
> 
> Should this have an @return comment?

Yup, thanks
https://hg.mozilla.org/mozilla-central/rev/dba0373001fe
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.