Last Comment Bug 683511 - GCLI needs a 'cookie' command
: GCLI needs a 'cookie' command
Status: RESOLVED FIXED
[gclicommands][good first bug]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 16
Assigned To: Dio Synodinos
:
Mentors:
Depends on: 720641
Blocks: GCLICMD
  Show dependency treegraph
 
Reported: 2011-08-31 07:50 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-07-12 01:41 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds support for cookie command(s) in GCLI (5.04 KB, patch)
2012-02-21 14:23 PST, Dio Synodinos
no flags Details | Diff | Splinter Review
Adds support for cookie command(s) in GCLI and tests (7.03 KB, patch)
2012-04-25 13:46 PDT, Dio Synodinos
no flags Details | Diff | Splinter Review
This far: 0 tests pass, 3 tests fail. (7.68 KB, patch)
2012-04-30 15:06 PDT, Dio Synodinos
no flags Details | Diff | Splinter Review
Upload 4 (9.09 KB, patch)
2012-05-30 04:25 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
Upload 5 (20.09 KB, patch)
2012-07-06 08:27 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-08-31 07:50:17 PDT
This command allows the user to view, add and remove cookies for a given domain
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-18 10:00:30 PST
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-11 10:19:37 PST
Bug triage, filter on PEGASUS.
Comment 3 Dio Synodinos 2012-02-08 12:06:25 PST
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] (needinfo me or ping on irc) 2012-02-08 14:45:37 PST
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 2012-02-08 15:14:44 PST
Yes, I'll give it a try.
Comment 6 Panos Astithas [:past] 2012-02-09 08:56:19 PST
(In reply to Dionysios G. Synodinos from comment #5)
> Yes, I'll give it a try.

It's yours!
Comment 7 Dio Synodinos 2012-02-21 14:23:25 PST
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.
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-23 11:40:22 PST
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 2012-02-28 00:03:56 PST
(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] (needinfo me or ping on irc) 2012-04-09 05:16:32 PDT
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 2012-04-13 03:11:16 PDT
(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 2012-04-25 13:46:20 PDT
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.
Comment 13 Panos Astithas [:past] 2012-04-25 14:36:45 PDT
(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] (needinfo me or ping on irc) 2012-04-29 11:25:51 PDT
GCLI Triage.
Comment 15 Dio Synodinos 2012-04-30 15:06:56 PDT
Created attachment 619710 [details] [diff] [review]
This far: 0 tests pass, 3 tests fail.
Comment 16 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-05-30 04:25:49 PDT
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?
Comment 17 Dio Synodinos 2012-05-30 05:48:51 PDT
(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] (needinfo me or ping on irc) 2012-05-30 14:36:19 PDT
(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] (needinfo me or ping on irc) 2012-05-31 07:03:06 PDT
Some work on a DateType:
https://github.com/joewalker/gcli/commits/cookie-683511
Comment 20 Michael Ratcliffe [:miker] [:mratcliffe] 2012-06-29 03:12:36 PDT
Filter on teabags
Comment 21 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-07-06 08:27:09 PDT
Created attachment 639684 [details] [diff] [review]
Upload 5
Comment 22 Heather Arthur [:harth] 2012-07-06 10:58:17 PDT
Comment on attachment 639684 [details] [diff] [review]
Upload 5

Changing review to dcamp, as he knows about cookies.
Comment 23 Dave Camp (:dcamp) 2012-07-06 11:03:51 PDT
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] (needinfo me or ping on irc) 2012-07-09 03:47:32 PDT
(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) 2012-07-09 17:33:25 PDT
Comment on attachment 639684 [details] [diff] [review]
Upload 5

r+ without the Cc->Components.classes changes.
Comment 26 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 04:00:45 PDT
Added to master patch in bug 768998
Comment 27 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 04:03:21 PDT
Copy / paste error ... added to master patch in but 771555.

Note You need to log in before you can comment on or make changes to this bug.