Closed Bug 1003761 Opened 6 years ago Closed 6 years ago

Cannot edit a cookie or remove breakpoints via devtoolbar (TypeError: arg.text.toLowerCase is not a function)

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

32 Branch
x86
macOS
defect
Not set

Tracking

(firefox29 affected, firefox30 affected, firefox31 affected, firefox32 affected, firefox33 fixed)

RESOLVED FIXED
Firefox 33
Tracking Status
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected
firefox33 --- fixed

People

(Reporter: m+bugzilla, Assigned: jwalker)

References

Details

Attachments

(2 files)

Attached video bug-cookie.mov
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140429030201

Steps to reproduce:

- Shift+F2
- "cookie list"
- click on "remove" or "edit"


Actual results:

Nothing happened


Expected results:

It should edit or remove the cookie.
I can also reproduce this on 29. :-(
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Graphic Commandline and Toolbar
Ever confirmed: true
Yep, it's easily since 30 and apparently by your words since 29.
Clicking "Edit" I get:

System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/types/command.js:121 - TypeError: arg.text.toLowerCase is not a function
Duplicate of this bug: 1005605
Duplicate of this bug: 1007969
Summary: Cannot edit a cookie via devtoolbar → Cannot edit a cookie via devtoolbar (TypeError: arg.text.toLowerCase is not a function)
doing some debugging and found that the |break list| command is also creating buttons that fail with the same error.

The templater appears to be putting the wrong thing in for the onclick handler.
Summary: Cannot edit a cookie via devtoolbar (TypeError: arg.text.toLowerCase is not a function) → Cannot edit a cookie or remove breakpoints via devtoolbar (TypeError: arg.text.toLowerCase is not a function)
So part 1 of this is dumb.
Requisition.update() cli.js:1485
"if (typed instanceof HTMLElement)" is never going to work in Gecko. Rather than monkey around with using HTMLElement on the web and Ci.nsIDOMHTMLElement or whatever in Gecko, I'm going to duck type against querySelector since that's what I'm about to call.

Part 2 was more subtle. Back in the depths of time, GCLI had loads of events and it was confusing. We purged the events right back, and were pleased to have got rid of events from the main processor Requisition entirely, however we overlooked that commands calling context.update in order to update the command line call directly into Requisition and without events we can't let the UI know that the world has changed. So we put back one event just for this.

Patch shortly.
In comment 7, I described 2 parts to this patch. There is a 3rd which I might have been able to get away without, but I fixed anyway. I broke the 3 parts out into a github pull request and commented on what they were doing individually to make things clearer (Click the [...] for the full comments)

https://tbpl.mozilla.org/?tree=Try&rev=fa0fc41db32e

If you're digging into this problem doesn't make you a good reviewer, shout and I'll ask someone else.
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Attachment #8447327 - Flags: review?(rcampbell)
Comment on attachment 8447327 [details] [diff] [review]
0001-Bug-1003761-Fix-clicking-on-shortcuts-in-GCLI-output.patch

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

looks fine to me, but if you have a regular GCLI reviewer who can validate your changes to cli.js, I might feel a bit better.

Also, having a test for these gcli shortcuts would prevent this from regressing in the future. Would be nice to have.

::: toolkit/devtools/gcli/source/lib/gcli/cli.js
@@ +779,5 @@
> + * clicking on a shortcut. The focus want to check the cursor position while
> + * the shortcut is updating the command line.
> + * This function allows us to detect and back out of this problem.
> + * We should be able to remove this function when all the state in a
> + * requisition can be encapsulated and updated atomically.

this sounds like a TODO comment. Not sure it's worth filing a follow-up or an issue in the GCLI master repo or not. Guessing not.

@@ +785,5 @@
> +Requisition.prototype.isUpToDate = function() {
> +  if (!this._args) {
> +    return false;
> +  }
> +  for (var i = 0; i < this._args.length; i++) {

don't you wish you had ES6? :)
Attachment #8447327 - Flags: review?(rcampbell) → review+
(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> Comment on attachment 8447327 [details] [diff] [review]
> 0001-Bug-1003761-Fix-clicking-on-shortcuts-in-GCLI-output.patch
> 
> Review of attachment 8447327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks fine to me, but if you have a regular GCLI reviewer who can validate
> your changes to cli.js, I might feel a bit better.

OK, I'll get someone else to take a pass through.

> Also, having a test for these gcli shortcuts would prevent this from
> regressing in the future. Would be nice to have.

Yup. bug 1033490

> ::: toolkit/devtools/gcli/source/lib/gcli/cli.js
> @@ +779,5 @@
> > + * clicking on a shortcut. The focus want to check the cursor position while
> > + * the shortcut is updating the command line.
> > + * This function allows us to detect and back out of this problem.
> > + * We should be able to remove this function when all the state in a
> > + * requisition can be encapsulated and updated atomically.
> 
> this sounds like a TODO comment. Not sure it's worth filing a follow-up or
> an issue in the GCLI master repo or not. Guessing not.

Yeah - I thought about that, but I'd like to not fix it if I can get away with it! We can create a bug if/when we need to fix it.

FWIW, bugzilla is where all GCLI bugs happen.

> @@ +785,5 @@
> > +Requisition.prototype.isUpToDate = function() {
> > +  if (!this._args) {
> > +    return false;
> > +  }
> > +  for (var i = 0; i < this._args.length; i++) {
> 
> don't you wish you had ES6? :)

Yup. Fat arrows. Missing those kills me the most.
Attachment #8447327 - Flags: review+ → review?(mratcliffe)
Attachment #8447327 - Flags: review?(mratcliffe) → review+
https://hg.mozilla.org/mozilla-central/rev/ef6fcda84ce4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
QA Whiteboard: [good first verify]
Duplicate of this bug: 1062207
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.