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

Status: RESOLVED FIXED

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

Description

[kud]

Attachment: 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.

Comment 1

[:Gijs (he/him)]

I can also reproduce this on 29. :-(

Comment 2

[kud]

Yep, it's easily since 30 and apparently by your words since 29.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 6

[Rob Campbell [:rc] (:robcee)]

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.

Comment 7

[Joe Walker [:jwalker]]

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.

Comment 8

[Joe Walker [:jwalker]]

Attachment: 0001-Bug-1003761-Fix-clicking-on-shortcuts-in-GCLI-output.patch

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.

Comment 9

[Rob Campbell [:rc] (:robcee)]

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? :)

Comment 10

[Joe Walker [:jwalker]]

(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.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/ef6fcda84ce4

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef6fcda84ce4