The default bug view has changed. See this FAQ.

GCLI needs a 'cookie' command

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools: Console
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwalker, Assigned: Dio Synodinos)

Tracking

unspecified
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gclicommands][good first bug])

Attachments

(1 attachment, 4 obsolete attachments)

This command allows the user to view, add and remove cookies for a given domain
Blocks: 659052
No longer blocks: 671406
Whiteboard: [good first bug]
OS: Windows 7 → All
Hardware: x86_64 → All
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Blocks: 711051
No longer blocks: 659052
Bug triage, filter on PEGASUS.
Priority: -- → P2
Target Milestone: --- → Firefox 12
No longer blocks: 711051
Target Milestone: Firefox 12 → Firefox 13
(Assignee)

Comment 3

5 years ago
Would a syntax like this be ok?

> cookie set --key fruit --value banana
> cookie get --key fruit
> cookie get fruit
> cookie remove fruit
> cookie remove --key fruit
> cookie set --key fruit --value banana --expires 'Wed Jan 01 2030 00:00:00 GMT+0200 (EET)}' --path '/mydir' --domain 'example.com' --secure true
> cookie set --key fruit --value banana --max-age 2592000 --path '/mydir' --domain 'example.com'
Looks good.
I think we can do without the named parameters when they're mandatory:

> cookie get fruit
> cookie remove fruit
> cookie set fruit banana
> cookie set fruit banana --expires '...' --path '/mydir' --domain 'example.com' --secure true

Makes sense?
(Assignee)

Comment 5

5 years ago
Yes, I'll give it a try.
(In reply to Dionysios G. Synodinos from comment #5)
> Yes, I'll give it a try.

It's yours!
Assignee: nobody → synodinos
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
Created attachment 599368 [details] [diff] [review]
Adds support for cookie command(s) in GCLI

Seems to be working for me. Any feedback is welcome, thanks.
Attachment #599368 - Flags: review?(jwalker)
Comment on attachment 599368 [details] [diff] [review]
Adds support for cookie command(s) in GCLI

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

Thanks Dio. This looks great.
In addition to my query on document below, we also need to work out some tests. I can probably help if you'd like, because I should write some infrastructure to make it easier to write command tests.

::: browser/devtools/webconsole/GcliCommands.jsm
@@ +313,5 @@
> +  ],
> +  returnType: 'string',
> +  exec: function Command_cookieGet(args, context) {
> +    let document = context.environment.contentDocument;
> +    return unescape(document.defaultView.window.document.cookie.replace(new RegExp("(?:^|.*;\\s*)" 

Isn't document.defaultView.window.document the same as 'document'?
(Assignee)

Comment 9

5 years ago
(In reply to Joe Walker from comment #8)
> Comment on attachment 599368 [details] [diff] [review]
> Adds support for cookie command(s) in GCLI
> 
> Review of attachment 599368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Dio. This looks great.
> In addition to my query on document below, we also need to work out some
> tests. I can probably help if you'd like, because I should write some
> infrastructure to make it easier to write command tests.

Yes, without tests it feels kinda dirty :)

Please add the infrastructure with an example, and I'll add the necessary tests for the 'cookie' command.

> ::: browser/devtools/webconsole/GcliCommands.jsm
> @@ +313,5 @@
> > +  ],
> > +  returnType: 'string',
> > +  exec: function Command_cookieGet(args, context) {
> > +    let document = context.environment.contentDocument;
> > +    return unescape(document.defaultView.window.document.cookie.replace(new RegExp("(?:^|.*;\\s*)" 
> 
> Isn't document.defaultView.window.document the same as 'document'?

If I remember correctly no, but I'll double-check and update the patch if necessary. I'm currently experiencing segfault with the current nightly and trying to work around this.
Target Milestone: Firefox 13 → Firefox 14
Re: test framework. I'm done, it's in the review queue, so I hope it will land soon.

I've converted all of the existing tests to the new system. As an example (to save digging around in patches), here is Panos' debugger test suite https://gist.github.com/2343085

The actual patch is here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=720641&attachment=612898

If you click on head.js towards the bottom of the navigation section, you can see the documentation for the test framework.

I've marked this bug depending on the landing of the test framework, so we'll know.
Depends on: 720641
(Assignee)

Comment 11

5 years ago
(In reply to Joe Walker from comment #10)
Great, as soon as I can get the nightly to build successfully on my system again (bug 745122), I'll give it a try.
(Assignee)

Comment 12

5 years ago
Created attachment 618417 [details] [diff] [review]
Adds support for cookie command(s) in GCLI and tests

1.) Has testing landed? I don't see any messages when I do make -C obj-x86_64-apple-darwin11.3.0/browser. 

2.) Should I be doing something like:

Services.prefs.setBoolPref("devtools.gcli.enable", true)
<Test goes here>
Services.prefs.setBoolPref("devtools.gcli.enable", false);

All feedback is welcomed.
(In reply to Dio Synodinos from comment #12)
> Created attachment 618417 [details] [diff] [review]
> Adds support for cookie command(s) in GCLI and tests
> 
> 1.) Has testing landed? I don't see any messages when I do make -C
> obj-x86_64-apple-darwin11.3.0/browser. 
> 
> 2.) Should I be doing something like:
> 
> Services.prefs.setBoolPref("devtools.gcli.enable", true)
> <Test goes here>
> Services.prefs.setBoolPref("devtools.gcli.enable", false);
> 
> All feedback is welcomed.

It landed a few hours ago in fx-team and those things are taken care of for you automatically. You only need to add the test in Makefile.in, in the _BROWSER_TEST_FILES section. Then you rebuild and run it with:

TEST_PATH=browser/devtools/shared/test/browser_gcli_cookie.js make -C obj-ff-dbg mochitest-browser-chrome
GCLI Triage.
Target Milestone: Firefox 14 → Firefox 15
Blocks: 745773
(Assignee)

Comment 15

5 years ago
Created attachment 619710 [details] [diff] [review]
This far: 0 tests pass, 3 tests fail.

Updated

5 years ago
No longer depends on: 720641

Updated

5 years ago
Depends on: 720641
No longer blocks: 745773
Created attachment 628305 [details] [diff] [review]
Upload 4

Update at last!

Here's what I changed:
- Rebased since gcli got it's own directory
- Added 'cookie' command
- Fixed several tests, only one fails now (see below)
- Made several of the 'cookie set' parameters optional

Here's what I think we must do:
- Localize the missing strings
- Document the regexp in Command_cookieGet
- Fix the test which is related to the regexp in Command_cookieGet

Here's what I'd like to do (and I can help with):
- GCLI needs a date type so we can say '--expires now'
- Add a cookieName type so we get completion for 'cookie get' and
  'cookie remove'
- Get the UI updates for parameters in
- Add a 'cookie list' command?
Attachment #599368 - Attachment is obsolete: true
Attachment #618417 - Attachment is obsolete: true
Attachment #619710 - Attachment is obsolete: true
Attachment #599368 - Flags: review?(jwalker)
(Assignee)

Comment 17

5 years ago
(In reply to Joe Walker from comment #16)
> Created attachment 628305 [details] [diff] [review]
> Upload 4
> 
> Update at last!
> 
> Here's what I changed:
> - Rebased since gcli got it's own directory
> - Added 'cookie' command
> - Fixed several tests, only one fails now (see below)
> - Made several of the 'cookie set' parameters optional
> 
> Here's what I think we must do:
> - Localize the missing strings
> - Document the regexp in Command_cookieGet
> - Fix the test which is related to the regexp in Command_cookieGet
> 
> Here's what I'd like to do (and I can help with):
> - GCLI needs a date type so we can say '--expires now'
> - Add a cookieName type so we get completion for 'cookie get' and
>   'cookie remove'
> - Get the UI updates for parameters in
> - Add a 'cookie list' command?

Would love to help on those. Is it ok to go over them in person in NYC or are there any milestones before that?
(In reply to Dio Synodinos from comment #17)
> Would love to help on those. Is it ok to go over them in person in NYC or
> are there any milestones before that?

I'm happy to hack on it in NYC. The first 3 items are things you could take a look at easily if you had time.
Thanks
Some work on a DateType:
https://github.com/joewalker/gcli/commits/cookie-683511
Target Milestone: Firefox 15 → Firefox 16
Blocks: 768998
Filter on teabags
Whiteboard: [good first bug] → [gclicommands][good first bug]
Created attachment 639684 [details] [diff] [review]
Upload 5
Attachment #628305 - Attachment is obsolete: true
Attachment #639684 - Flags: review?(fayearthur)
Comment on attachment 639684 [details] [diff] [review]
Upload 5

Changing review to dcamp, as he knows about cookies.
Attachment #639684 - Flags: review?(fayearthur) → review?(dcamp)

Comment 23

5 years ago
Comment on attachment 639684 [details] [diff] [review]
Upload 5

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

::: browser/devtools/commandline/GcliCommands.jsm
@@ -237,5 @@
>      let ctx = canvas.getContext("2d");
>      ctx.drawWindow(window, left, top, width, height, "#fff");
>  
>      let data = canvas.toDataURL("image/png", "");
> -    let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);

Why all the Cc->Components.classes?
(In reply to Dave Camp (:dcamp) from comment #23)
> Comment on attachment 639684 [details] [diff] [review]
> Upload 5
> 
> Review of attachment 639684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ -237,5 @@
> >      let ctx = canvas.getContext("2d");
> >      ctx.drawWindow(window, left, top, width, height, "#fff");
> >  
> >      let data = canvas.toDataURL("image/png", "");
> > -    let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
> 
> Why all the Cc->Components.classes?

No good reason. Will remove.

Comment 25

5 years ago
Comment on attachment 639684 [details] [diff] [review]
Upload 5

r+ without the Cc->Components.classes changes.
Attachment #639684 - Flags: review?(dcamp) → review+
Added to master patch in bug 768998
Copy / paste error ... added to master patch in but 771555.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.