Closed Bug 683511 Opened 10 years ago Closed 9 years ago

GCLI needs a 'cookie' command

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 16

People

(Reporter: jwalker, Assigned: synodinos)

References

Details

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

Attachments

(1 file, 4 obsolete files)

This command allows the user to view, add and remove cookies for a given domain
Blocks: GCLI-FUTURE
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: GCLI-12
No longer blocks: GCLI-FUTURE
Bug triage, filter on PEGASUS.
Priority: -- → P2
Target Milestone: --- → Firefox 12
No longer blocks: GCLI-12
Target Milestone: Firefox 12 → Firefox 13
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?
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
Seems to be working for me. Any feedback is welcome, thanks.
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'?
(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
(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.
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
No longer depends on: 720641
Depends on: 720641
No longer blocks: 745773
Attached patch Upload 4 (obsolete) — Splinter Review
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)
(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
Target Milestone: Firefox 15 → Firefox 16
Blocks: GCLICMD
Filter on teabags
Whiteboard: [good first bug] → [gclicommands][good first bug]
Attached patch Upload 5Splinter Review
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 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 on attachment 639684 [details] [diff] [review]
Upload 5

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