always-false (useless) condition in gcli util.js

RESOLVED FIXED in Firefox 51

Status

P2
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: tromey, Assigned: Towkir, Mentored)

Tracking

unspecified
Firefox 51

Firefox Tracking Flags

(firefox51 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
This condition in gcli:

https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/devtools/shared/gcli/source/lib/gcli/util/util.js#570

that is:

if (typeof 'KeyEvent' === 'undefined') {

... is never taken, because typeof 'KeyEvent' is 'string'.
Priority: -- → P2
Summary: always-true (useless) condition in gcli util.js → always-flase (useless) condition in gcli util.js
Summary: always-flase (useless) condition in gcli util.js → always-false (useless) condition in gcli util.js
Beautiful.

The if block should be removed so that we are left with the contents of the else block.
Mentor: mratcliffe
Whiteboard: [good first bug][lang=js]
We should just delete the whole thing. Our engine now does the right thing with KeyEvent, so we don't need the (broken) shim.
(Reporter)

Comment 3

2 years ago
FWIW we're re-shimming it in bug 1290227.
(In reply to Tom Tromey :tromey from comment #3)
> FWIW we're re-shimming it in bug 1290227.

>> typeof KeyEvent
→  "function"

We should check that our shim isn't clashing with a real implementation that's recently landed in Nightly.
(Assignee)

Comment 5

2 years ago
I would like to work on this one.
Can you tell me what should be done ?
Are you talking about removing the whole "if" function on the line mentioned on comment 0 ?
Flags: needinfo?(mratcliffe)
Comment hidden (obsolete)
Surely we can just use the global KeyEvent can't we. The whole point of devtools.html is to be getting away from Moz specific code, and just using the web platform, so using the global seems like the best solution.
(Reporter)

Comment 8

2 years ago
(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #7)
> Surely we can just use the global KeyEvent can't we. The whole point of
> devtools.html is to be getting away from Moz specific code, and just using
> the web platform, so using the global seems like the best solution.

KeyEvent.DOM_VK_* is not portable afaik.
For devtools.html we're adding a "keycodes.js" module.
See bug 1290227.

The portable way to do this kind of thing is not use keyCode at all, but rather
|.code|.  See https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code

However in this particular case I think either just removing the "if" is sufficient;
or if gcli can depend on modules elsewhere, just do what the patches in bug 1290227 do.
@Towkir: So just remove the if statement and keep the contents of else... that is all we need for the moment.
(Assignee)

Comment 10

2 years ago
Only in the util.js file ?
Flags: needinfo?(mratcliffe)
yup
Flags: needinfo?(mratcliffe)
(Assignee)

Comment 12

2 years ago
Created attachment 8780671 [details] [diff] [review]
useless_condition.patch

Here it is
Attachment #8780671 - Flags: review?(mratcliffe)
Comment on attachment 8780671 [details] [diff] [review]
useless_condition.patch

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

Looks good but can you fix the indentation?
Attachment #8780671 - Flags: review?(mratcliffe)
(Assignee)

Comment 14

2 years ago
Created attachment 8780697 [details] [diff] [review]
useless_condition.patch

indented
Attachment #8780671 - Attachment is obsolete: true
Attachment #8780697 - Flags: review?(mratcliffe)
Comment on attachment 8780697 [details] [diff] [review]
useless_condition.patch

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

Great job... just add a semicolon after the closing brace and we are done.
Attachment #8780697 - Flags: review?(mratcliffe) → review+
(Assignee)

Comment 16

2 years ago
Created attachment 8780700 [details] [diff] [review]
useless_condition.patch

I intentionally deleted that, thought it is not necessary at the last line :p
Attachment #8780697 - Attachment is obsolete: true
Attachment #8780700 - Flags: review?(mratcliffe)
Sorry... I think you forgot to apply your change before submitting the patch.
(Assignee)

Comment 18

2 years ago
Created attachment 8780711 [details] [diff] [review]
useless_condition.patch

Sorry the patch was already applied while I ran qrefresh. Hope it's ok now.
Attachment #8780700 - Attachment is obsolete: true
Attachment #8780700 - Flags: review?(mratcliffe)
(Assignee)

Updated

2 years ago
Attachment #8780711 - Flags: review?(mratcliffe)
Comment on attachment 8780711 [details] [diff] [review]
useless_condition.patch

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

(In reply to [:Towkir] Ahmed from comment #18)
> Created attachment 8780711 [details] [diff] [review]
> useless_condition.patch
> 
> Sorry the patch was already applied while I ran qrefresh. Hope it's ok now.

We have all done that at some point... great job!

Can you add the checkin-needed keyword in the keyword field?
Attachment #8780711 - Flags: review?(mratcliffe) → review+
(Assignee)

Comment 20

2 years ago
Sure, why not
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/a8e4a3aafbd9
Remove the always-false (useless) 'if' condition from GCLI util.js. r=miker
Keywords: checkin-needed

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a8e4a3aafbd9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.