683511 - GCLI needs a 'cookie' command

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Joe Walker [:jwalker]]

This command allows the user to view, add and remove cookies for a given domain

Comment 1

[Joe Walker [:jwalker]]

Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.

Comment 2

[Joe Walker [:jwalker]]

Bug triage, filter on PEGASUS.

Comment 3

[Dio Synodinos]

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'

Comment 4

[Joe Walker [:jwalker]]

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?

Comment 5

[Dio Synodinos]

Yes, I'll give it a try.

Comment 6

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

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

It's yours!

Comment 7

[Dio Synodinos]

Attachment: Adds support for cookie command(s) in GCLI

Seems to be working for me. Any feedback is welcome, thanks.

Comment 8

[Joe Walker [: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'?

Comment 9

[Dio Synodinos]

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

Comment 10

[Joe Walker [:jwalker]]

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.

Comment 11

[Dio Synodinos]

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

Comment 12

[Dio Synodinos]

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

Comment 13

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

(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

Comment 14

[Joe Walker [:jwalker]]

GCLI Triage.

Comment 15

[Dio Synodinos]

Attachment: This far: 0 tests pass, 3 tests fail.

Comment 16

[Joe Walker [:jwalker]]

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

Comment 17

[Dio Synodinos]

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

Comment 18

[Joe Walker [:jwalker]]

(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

Comment 19

[Joe Walker [:jwalker]]

Some work on a DateType:
https://github.com/joewalker/gcli/commits/cookie-683511

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Filter on teabags

Comment 21

[Joe Walker [:jwalker]]

Attachment: Upload 5

Comment 22

[Heather Arthur [:harth]]

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

Changing review to dcamp, as he knows about cookies.

Comment 23

[Dave Camp (:dcamp)]

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?

Comment 24

[Joe Walker [:jwalker]]

(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

[Dave Camp (:dcamp)]

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

r+ without the Cc->Components.classes changes.

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Added to master patch in bug 768998

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Copy / paste error ... added to master patch in but 771555.