Closed
Bug 1003761
Opened 11 years ago
Closed 11 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)
Tracking
(firefox29 affected, firefox30 affected, firefox31 affected, firefox32 affected, firefox33 fixed)
RESOLVED
FIXED
Firefox 33
People
(Reporter: m+mozilla, Assigned: jwalker)
References
Details
Attachments
(2 files)
3.79 MB,
video/quicktime
|
Details | |
8.65 KB,
patch
|
miker
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
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
Updated•11 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → affected
Updated•11 years ago
|
Summary: Cannot edit a cookie via devtoolbar → Cannot edit a cookie via devtoolbar (TypeError: arg.text.toLowerCase is not a function)
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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+
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Attachment #8447327 -
Flags: review+ → review?(mratcliffe)
Updated•11 years ago
|
Attachment #8447327 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 12•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Updated•11 years ago
|
QA Whiteboard: [good first verify]
Updated•11 years ago
|
status-firefox33:
--- → fixed
Updated•7 years ago
|
Product: Firefox → DevTools
Updated•7 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•