792986 - add tagging support for cookies

Status: NEW

(Core :: Networking: Cookies, defect, P5)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

mgoodwin, geekboy and I have talked about adding a tag field for cookies. Mark wants it for https://people.mozilla.com/~mgoodwin/SameDomain/, I want it for cookie experiments.

Adding a tag hashmap will avoid the need for adding individual fields to nsICookie.idl for every new cookie feature.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: schema changes and migration for extended attributes in moz_cookie

Comment 2

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes

Have had to put some of the tests in here as I can't test insert / update on the new schema until we can get access to the data.

Comment 3

[Mark Goodwin [:mgoodwin]]

Attachment: schema changes and migration for extended attributes in moz_cookie

As before with (some) nits addressed

Comment 4

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes

As before with (some) nits addressed

Comment 5

[Mark Goodwin [:mgoodwin]]

Also...
(In reply to Monica Chew from bug 795346 comment #14)
> >    /**
> > +   * Add a cookie. nsICookieService is the normal way to do this. This
> > +   * method is something of a backdoor.
> 
> I don't understand this comment. Why is it a backdoor? Who's supposed to use
> this?

This is taken from the comment for the original nsIcookieManager2 Add - I think what it means is that under normal circumstances you should use the methods in nsICookieService to create a cookie from a string rather than making it directly with nsICookieManager2.

Comment 6

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes

added spaces between params of AddWithAttributes

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 686675 [details] [diff] [review]
Parse, persists and allow access via xpcom to extended attributes

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

Hi Mark,

I don't see where the attribute string is ever parsed from the call to set cookie. I think it makes more sense to add nsIArray attributes to nsCookieAttribute, the struct, then call your parsing function from within ParseAttributes. Otherwise this patch is difficult to make sense of. That also lets you get rid of addWithAttributes, which doesn't make sense right now.

Thanks,
Monica

::: netwerk/cookie/nsCookieService.cpp
@@ +1874,5 @@
>  
> +  nsClassHashtable<nsCStringHashKey, nsCString> attrs;
> +  attrs.Init();
> +
> +  ParseExtendedAttributes(aAttrString, attrs);

aattrstring is never set meaningfully

::: netwerk/cookie/nsCookieService.h
@@ +277,5 @@
>      void                          AddCookieToList(const nsCookieKey& aKey, nsCookie *aCookie, DBState *aDBState, mozIStorageBindingParamsArray *aParamsArray, bool aWriteToDB = true);
>      void                          UpdateCookieInList(nsCookie *aCookie, int64_t aLastAccessed, mozIStorageBindingParamsArray *aParamsArray);
>      static bool                   GetTokenValue(nsASingleFragmentCString::const_char_iterator &aIter, nsASingleFragmentCString::const_char_iterator &aEndIter, nsDependentCSubstring &aTokenString, nsDependentCSubstring &aTokenValue, bool &aEqualsFound);
>      static bool                   ParseAttributes(nsDependentCString &aCookieHeader, nsCookieAttributes &aCookie);
> +    static bool                   ParseExtendedAttributes(const nsACString &aAttributes, nsClassHashtable<nsCStringHashKey, nsCString> &aParsed);

I know it's not normal to include documentation in header files, but let's break this trend! It's confusing to have 2 similarly named methods that have such different params. Can you either rename this ParseTags, or make this private and put the hashtable in nsCookieAttributes?

::: netwerk/cookie/nsICookie2.idl
@@ +79,5 @@
> +     * @return the value for the specified attribute.
> +     *
> +     * @see getExtendedAttributes
> +     */
> +    AUTF8String getAttributeValue(in AUTF8String aName);

I think we need setters as well. I'd like the naming to be more consistent, and would be in favor of

getAttributes() // for returning the whole array
setAttributes(nsIArray attrs) // for setting the whole array
string getAttribute(string name) const // for returning a single one
bool setAttribute(string value) // for setting a single one, could return true if the attr already exists, that might be helpful

In other words, don't call it extended only sometimes.

::: netwerk/cookie/nsICookieManager2.idl
@@ +57,5 @@
>             in int64_t     aExpiry);
>  
>    /**
> +   * Add a cookie. nsICookieService is the normal way to do this. This
> +   * method is something of a backdoor.

Please be specific as to who is supposed to call this, and when. If this is only to accommodate a way to set the attributes, I think it's better change callsites to pass in null attributes.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 686659 [details] [diff] [review]
schema changes and migration for extended attributes in moz_cookie

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

Looks ok to me -- however it also looks like the change in nscookieservice is completely covered by your other patch. Are you planning to check this in first?

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey Mark,

I think what I would like to see is a self-contained patch that includes a test that calls setCookie with whatever syntax you've decided to adopt for the tags, then a call to get the cookie and its attributes that shows that the tags were indeed parsed correctly. I'll be around tomorrow am if you want to chat.

Monica

Comment 10

[Mark Goodwin [:mgoodwin]]

Attachment: schema changes and migration for extended attributes in moz_cookie

Comment 11

[Mark Goodwin [:mgoodwin]]

Attachment: tests for schema changes (and schema update)

Comment 12

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes

Comment 13

[Mark Goodwin [:mgoodwin]]

Attachment: tests for parsing / persistence of attributes

(In reply to Monica Chew [:mmc] from comment #9)
> I think what I would like to see is a self-contained patch that includes a
> test that calls setCookie with whatever syntax you've decided to adopt for
> the tags, then a call to get the cookie and its attributes that shows that
> the tags were indeed parsed correctly. 

Hopefully this addresses the above request.

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 728287 [details] [diff] [review]
Parse, persists and allow access via xpcom to extended attributes

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

::: netwerk/cookie/nsCookie.cpp
@@ +165,5 @@
> +}
> +
> +/******************************************************************************
> + * nsCookie:
> + * obtain an attribute string. E.g. for persisting.

This comment seems to be misplaced.

@@ +184,5 @@
> +  return PL_DHASH_NEXT;
> +}
> +
> +void
> +nsCookie::AttributesString(nsACString &aAttrString) const

This is a bad function name. It seems the caller wants to set the attribute string, then do operations on it?

Why not make GetAttributesString() { return mAttributes; }

The other way leads to weird patterns.

::: netwerk/cookie/nsCookie.h
@@ +65,3 @@
>  
>      virtual ~nsCookie() {}
> +    void AttributesString(nsACString &aAttrString) const;

Why is this an ACString? Do you need to accommodate embedded nulls?

@@ +79,5 @@
>      inline bool IsSession()               const { return mIsSession; }
>      inline bool IsDomain()                const { return *mHost == '.'; }
>      inline bool IsSecure()                const { return mIsSecure; }
>      inline bool IsHttpOnly()              const { return mIsHttpOnly; }
> +    inline bool HasAttributes() const { return mAttributes.Count() > 0; }

CountAttributes() { return mAttributes.Count(); } seems to be more useful here.

@@ +80,5 @@
>      inline bool IsDomain()                const { return *mHost == '.'; }
>      inline bool IsSecure()                const { return mIsSecure; }
>      inline bool IsHttpOnly()              const { return mIsHttpOnly; }
> +    inline bool HasAttributes() const { return mAttributes.Count() > 0; }
> +    inline void SetAttribute(nsCString &key, nsCString *value) { mAttributes.Put(key, value); }

Do you want callers to be able to enumerate attributes?

::: netwerk/cookie/nsCookieService.cpp
@@ +3217,5 @@
> +
> +  while (attrStart != attrEnd && !newAttr) {
> +    newAttr = GetTokenValue(attrStart, attrEnd, tokenString, tokenValue, equalsFound);
> +
> +    const char* keyString, * valueString;

*valueString for consistency

@@ +3221,5 @@
> +    const char* keyString, * valueString;
> +    tokenString.GetData(&keyString);
> +    tokenValue.GetData(&valueString);
> +    nsCString* key = new nsCString(tokenString);
> +    nsCString* value = new nsCString(tokenValue);

Why do you need keyString and valueString at all? You don't seem to be using them.

::: netwerk/cookie/nsICookie2.idl
@@ +63,5 @@
>  
> +    /**
> +     * Get the names of extended attributes set on this cookie
> +     *
> +     * @return an nsIArray of attribute names.

nsIArray of strings? What kind of strings? It needs to be in the comments.

::: netwerk/cookie/nsICookieManager2.idl
@@ +57,5 @@
>             in int64_t     aExpiry);
>  
>    /**
> +   * Add a cookie. nsICookieService is the normal way to do this. This
> +   * method is something of a backdoor.

Please delete comments that don't make sense rather than copying and pasting existing comments around.

@@ +97,5 @@
> +           in boolean     aIsSecure,
> +           in boolean     aIsHttpOnly,
> +           in boolean     aIsSession,
> +           in int64_t     aExpiry,
> +           in AUTF8String aAttributes);

You might check if xpcom passes trailing arguments in JS with defaults, i.e. empty string for aAttributes. In that case you might not need addWithAttributes. Of course that would break all the JS callers that rely on default aExpiry=0 as a trailing argument already, I'm not sure how common that is.

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 728289 [details] [diff] [review]
tests for parsing / persistence of attributes

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

::: extensions/cookie/test/unit/test_cookies_attributes.js
@@ +30,5 @@
> +  let failure = true;
> +
> +  // test throw on unset attr
> +  try {
> +    cookie.getAttributeValue('nonexistant');

nonexistent

@@ +31,5 @@
> +
> +  // test throw on unset attr
> +  try {
> +    cookie.getAttributeValue('nonexistant');
> +  } catch(e) {

This needs to be more specific. Can you do

const Cr = Components.results;
try {
  cookie.getAttributeValue("nonexistent");
  throw("Getting a non-existent attribute should fail");
} catch (ex if ex.result == Cr.NS_ERROR_FAILURE) { }

except replace it with the right error type. You don't need do_check_false(failure), if the test throws unexpected error or doesn't throw, it will fail.

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 728285 [details] [diff] [review]
schema changes and migration for extended attributes in moz_cookie

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

::: netwerk/cookie/nsCookieService.cpp
@@ +2032,5 @@
>    bool isSecure = 0 != aRow->AsInt32(IDX_SECURE);
>    bool isHttpOnly = 0 != aRow->AsInt32(IDX_HTTPONLY);
>  
> +  rv = aRow->GetUTF8String(IDX_ATTRIBUTES, attrString);
> +  NS_ASSERT_SUCCESS(rv);

These should be calling NS_ENSURE_SUCCESS(rv, rv) which is used everywhere else in the codebase. There's no need to redefine a macro that does the same thing in this file.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 728286 [details] [diff] [review]
tests for schema changes (and schema update)

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

::: extensions/cookie/test/unit/test_schema_4_migration.js
@@ +23,5 @@
> +  // Set up a profile.
> +  let profile = do_get_profile();
> +
> +  // Create a schema 4 database.
> +  let schema4db = new CookieDatabaseConnection(do_get_cookie_file(profile), 4);

This is so similar to the other test. Can you get rid of one test file by parameterizing the old version number, and calling migrate_schema(old_version) in do_run_test, for old_version=4 and 5?

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Mark,

I suggest structuring your patches so that they are rollback units, e.g. bundle the test changes with the implementation changes so that in case of error, it's easier to rollback 1 patch instead of 2.

Monica

Comment 19

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes (with tests)

Comment 20

[Mark Goodwin [:mgoodwin]]

Attachment: schema changes and migration for extended attributes in moz_cookie (with tests)

Comment 21

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes (with tests)

Comment 22

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 730311 [details] [diff] [review]
schema changes and migration for extended attributes in moz_cookie (with tests)

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

::: extensions/cookie/test/unit/head_cookies.js
@@ +165,5 @@
>    this.creationTime = creationTime;
>    this.isSession = isSession;
>    this.isSecure = isSecure;
>    this.isHttpOnly = isHttpOnly;
> +  this.extendedAttributes = extendedAttributes ? extendedAttributes  : '';

Can you just do this.extendedAttributes = extendedAttributes?

Comment 23

[Mark Goodwin [:mgoodwin]]

(In reply to Monica Chew [:mmc] from comment #22)
> Comment on attachment 730311 [details] [diff] [review]
> schema changes and migration for extended attributes in moz_cookie (with
> tests)
> 
> Review of attachment 730311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: extensions/cookie/test/unit/head_cookies.js
> @@ +165,5 @@
> >    this.creationTime = creationTime;
> >    this.isSession = isSession;
> >    this.isSecure = isSecure;
> >    this.isHttpOnly = isHttpOnly;
> > +  this.extendedAttributes = extendedAttributes ? extendedAttributes  : '';
> 
> Can you just do this.extendedAttributes = extendedAttributes?

I don't think so; if extendedAttributes is null or undefined I want this.extendedAttributes to be the empty string (not null or undefined).

Comment 24

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes (with tests)

Tidied GetAttribute

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 730312 [details] [diff] [review]
Parse, persists and allow access via xpcom to extended attributes (with tests)

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

Sorry for the delay, I lost my draft comments on my laptop. Going ahead and publishing without looking at the whole patch because I think this attachment is already out of date.

::: extensions/cookie/test/unit/head_cookies.js
@@ +111,5 @@
>    do_check_eq(Services.cookiemgr.countCookiesFromHost(uri.host), expected);
>  }
>  
> +function do_set_cookie(uri, channel, cookie, session, expected, attrs="") {
> +  let suffix = (attrs ? "; "+attrs : "") + (session ? "" : "; max-age=1000");

" + attrs (spaces around +) for readability and consistency

@@ +121,4 @@
>  // Set four cookies; with & without channel, http and non-http; and test
>  // the cookie count against 'expected' after each set.
> +function do_set_cookies(uri, channel, session, expected, attrs="") {
> +  let suffix = (attrs ? "; "+attrs : "") + (session ? "" : "; max-age=1000");

same

::: extensions/cookie/test/unit/test_cookies_attributes.js
@@ +22,5 @@
> +  var enm = Services.cookiemgr.getCookiesFromHost(host);
> +  cookie = enm.getNext().QueryInterface(Ci.nsICookie2);
> +
> +  // test the attributes can be retrieved
> +  do_check_eq(cookie.getAttributeValue('someattr'),'blah');

space after , for readability and consistency

@@ +30,5 @@
> +  // test that multiple values result in the last set attribute value
> +  do_check_eq(cookie.getAttributeValue('moar'),'pickles');
> +
> +  // test case insensitivity on attribute names
> +  do_check_eq(cookie.getAttributeValue('Otherattr'),'');

// test case sensitivity on value names too?

::: netwerk/cookie/nsCookie.cpp
@@ +12,5 @@
> +#include <nsPrintfCString.h>
> +
> +/******************************************************************************
> + * nsCookie:
> + * old style constructor

misplaced comment

@@ +15,5 @@
> + * nsCookie:
> + * old style constructor
> + ******************************************************************************/
> +
> +// to copy attributes across on nsCookie creation

"Helper function to copy attributes from one hashtable to another"

is more descriptive since it doesn't really have to do with creation, right?

@@ +254,5 @@
> +{
> +  nsresult rv= NS_OK;
> +  nsIMutableArray *arr = (nsIMutableArray *) aArr;
> +  nsCOMPtr<nsISupportsCString> keyString(
> +              do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID, &rv));

2 spaces

@@ +272,5 @@
> +  *_retval = 0;
> +
> +  // create an array that's scriptable
> +  nsCOMPtr<nsIMutableArray> mutArray =
> +  do_CreateInstance(NS_ARRAY_CONTRACTID);

fits in 80 chars? if not, indent 2 spaces

::: netwerk/cookie/nsCookie.h
@@ +65,4 @@
>  
>      virtual ~nsCookie() {}
> +    void GetAttributesAsString(nsCString &aAttrString) const;
> +    bool GetAttribute(nsCString aKey, nsCString **aValue) const;

This deserves a description. I personally believe that function comments are most useful in a header file instead of the implementation, but ymmv.

@@ +80,5 @@
>      inline bool IsDomain()                const { return *mHost == '.'; }
>      inline bool IsSecure()                const { return mIsSecure; }
>      inline bool IsHttpOnly()              const { return mIsHttpOnly; }
> +    inline uint32_t CountAttributes() const { return mAttributes.Count(); }
> +    inline void SetAttribute(nsCString &key, nsCString *value) { mAttributes.Put(key, value); }

This is misplaced, if you want to keep setters separate (as the comment below)

@@ +107,5 @@
>      int64_t      mCreationTime;
>      bool mIsSession;
>      bool mIsSecure;
>      bool mIsHttpOnly;
> +    nsClassHashtable<nsCStringHashKey, nsCString> mAttributes;

This deserves a description, or a link to where you describe extended attributes.

Comment 26

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 731463 [details] [diff] [review]
Parse, persists and allow access via xpcom to extended attributes (with tests)

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

Hey Mark,

It would help me out to get a brief, English description of what changed in the patch when you make a new revision. Also, please say if you choose to ignore certain feedback, so I know that it's a deliberate choice instead of just an overlooked comment.

Thanks,
Monica

::: extensions/cookie/test/unit/head_cookies.js
@@ +121,4 @@
>  // Set four cookies; with & without channel, http and non-http; and test
>  // the cookie count against 'expected' after each set.
> +function do_set_cookies(uri, channel, session, expected, attrs="") {
> +  let suffix = (attrs ? "; "+attrs : "") + (session ? "" : "; max-age=1000");

spaces around + please

::: extensions/cookie/test/unit/test_cookies_attributes.js
@@ +4,5 @@
> +function run_test() {
> +  var spec1 = "http://foo.com/foo.html";
> +  var uri1 = NetUtil.newURI(spec1);
> +  var channel1 = NetUtil.newChannel(uri1);
> +  var httpchannel1 = channel1.QueryInterface(Ci.nsIHttpChannelInternal);

since there is no httpchannel2, please don't call this httpchannel1. Similarly for spec1, uri1, channel1.

::: netwerk/cookie/nsCookie.cpp
@@ +165,5 @@
> +}
> +
> +/******************************************************************************
> + * nsCookie:
> + * Append an attribute to the attribute string (see use of EnumerateRead in 

trailing whitespace

::: netwerk/cookie/nsCookie.h
@@ +65,2 @@
>  
>      virtual ~nsCookie() {}

Comments belong in .h

::: netwerk/cookie/nsCookieService.cpp
@@ +3208,5 @@
> +  aAttrString.EndReading(attrEnd);
> +
> +  nsDependentCSubstring tokenString(attrStart, attrStart);
> +  nsDependentCSubstring tokenValue (attrStart, attrStart);
> +  bool newAttr, equalsFound;

don't rely on implicit initialization to false here.

::: netwerk/cookie/nsCookieService.h
@@ +277,5 @@
>      void                          AddCookieToList(const nsCookieKey& aKey, nsCookie *aCookie, DBState *aDBState, mozIStorageBindingParamsArray *aParamsArray, bool aWriteToDB = true);
>      void                          UpdateCookieInList(nsCookie *aCookie, int64_t aLastAccessed, mozIStorageBindingParamsArray *aParamsArray);
>      static bool                   GetTokenValue(nsASingleFragmentCString::const_char_iterator &aIter, nsASingleFragmentCString::const_char_iterator &aEndIter, nsDependentCSubstring &aTokenString, nsDependentCSubstring &aTokenValue, bool &aEqualsFound);
>      static bool                   ParseAttributes(nsDependentCString &aCookieHeader, nsCookieAttributes &aCookie);
> +    static bool                   ParseExtendedAttributes(const nsACString &aAttributes, nsClassHashtable<nsCStringHashKey, nsCString> &aParsed);

How about ending the reign of zero comments in .h files, and writing one liners for everything that you've discovered about nsCookieService? If you don't want to do that, then please write a comment for at least ParseExtendedAttributes.

Comment 27

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 731463 [details] [diff] [review]
Parse, persists and allow access via xpcom to extended attributes (with tests)

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

::: extensions/cookie/test/unit/test_cookies_attributes.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

I think this is supposed to be MPL

1 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
2 /* vim: set ts=2 sw=2 et: */
3 /* This Source Code Form is subject to the terms of the Mozilla Public
4  * License, v. 2.0. If a copy of the MPL was not distributed with this
5  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
6

Comment 28

[Jason Duell]

vim modeline looks like this 

  /* vim: set ts=8 sts=2 et sw=2 tw=80: */

Lifted from see mozilla coding style guidelines

  https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

(Not sure why we provide textwidth=80 for vim but not emacs.  But you definitely want smarttabspace=2 and ts=8)

Comment 29

[Mark Goodwin [:mgoodwin]]

Attachment: Parse, persists and allow access via xpcom to extended attributes (with tests)

(In reply to Monica Chew [:mmc] from comment #25)
> Comment on attachment 730312 [details] [diff] [review]

Changes made:

test_cookie_attributes.js
Formatting fixed (spaces added for constistency and readability)
Renamed to httpchannel1 to httpchannel, spec1 to spec, etc.
Fixed license and modelines - I'd copied the original (incorrect) license line from head_cookies.js - is this a problem too?

head_cookies.js
Formatting fixed (spaces added for constistency and readability).

nsCookie.cpp
Fixed misplaced and poorly worded comments for the constructor and the helper function for copying attributes.
Fixed indentation issues and misplaced line breaks.
Fixed some trailing whitespace.

nsCookie.h
Moved the misplaced SetAttribute below the //setters comment
Added descriptions of GetAttributesAsString and GetAttribute - note, there are now duplicated comments in nsCookie.h and nsCookie.cpp (left in nsCookie.cpp for consistency) - is this OK?
Added comment explaining what mAttributes is for. Currently I'm referencing the bug; would it make sense to have a document elsewhere explaining this?
nsCookieService.cpp
Modified newAttr and equalsFound to be explicitly initialised to false (and as a result tests now pass on all platforms)

nsCookieService.h
Added a comment for ParseExtendedAttributed

Questions / feedback discussion:
>> +  // test case insensitivity on attribute names
>> +  do_check_eq(cookie.getAttributeValue('Otherattr'),'');
>
>// test case sensitivity on value names too?

Do we want this?  I know we discussed case insensitivity on attribute names (and decided it was a good idea) but I've not given attribute values any thought. Is there precedent here? Are there use cases where sensitivity or insentivity might be desirable?

> How about ending the reign of zero comments in .h files, and writing one liners for everything that you've discovered about nsCookieService?

I can do this; do you think this work belongs here or in a separate patch?

Comment 30

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 736248 [details] [diff] [review]
Parse, persists and allow access via xpcom to extended attributes (with tests)

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

It's looking better! Can you please include a link to the try output?

> Added descriptions of GetAttributesAsString and GetAttribute - note, there are now duplicated comments in nsCookie.h and nsCookie.cpp (left in nsCookie.cpp for consistency) - is this OK?

If they're your own code, I would move all the comments to .h. It is hard to keep comments in sync.

>> How about ending the reign of zero comments in .h files, and writing one liners for everything that you've discovered about nsCookieService?
> I can do this; do you think this work belongs here or in a separate patch?

I am always happy to review patches that improve comments in surrounding code. I doubt anyone would mind if you did it in this patch (speaking as a non-peer :) )

> Do we want this?  I know we discussed case insensitivity on attribute names (and decided it was a good idea) but I've not given attribute values any thought. Is there precedent here? Are there use cases where sensitivity
> or insensitivity might be desirable?

You are right, insensitivity on values seems harder to justify. To me, treating attributes foo=BAR and foo=bar differently seems like a mistake. However, the cookie RFC specifically calls out attribute names as case insensitive (and therefore implicitly values *are* sensitive). So I think that your patch is OK, as is.

4.1  Syntax:  General

   The two state management headers, Set-Cookie and Cookie, have common
   syntactic properties involving attribute-value pairs.  The following
   grammar uses the notation, and tokens DIGIT (decimal digits) and
   token (informally, a sequence of non-special, non-white space
   characters) from the HTTP/1.1 specification [RFC 2068] to describe
   their syntax.

   av-pairs        =       av-pair *(";" av-pair)
   av-pair         =       attr ["=" value]        ; optional value
   attr            =       token
   value           =       word
   word            =       token | quoted-string

   Attributes (names) (attr) are case-insensitive.  White space is
   permitted between tokens.  Note that while the above syntax
   description shows value as optional, most attrs require them.

   NOTE: The syntax above allows whitespace between the attribute and
   the = sign.

::: netwerk/cookie/nsCookie.cpp
@@ +10,3 @@
>  #include "nsUTF8ConverterService.h"
>  #include <stdlib.h>
> +#include <nsPrintfCString.h>

Can you audit the new .h's and make sure you are only including the ones you need? I doubt you are using nsPrintfCString

@@ +190,5 @@
> + * Flatten the attributes hashtable and return the resulting string as
> + * key=value; pairs
> + ******************************************************************************/
> +
> +void

More useful here to return uint32_t, i.e.

return mAttributes.EnumerateRead

@@ +275,5 @@
> +  // create an array that's scriptable
> +  nsCOMPtr<nsIMutableArray> mutArray = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  NS_ENSURE_STATE(mutArray);
> +
> +  // copy the keynames into the array

Reading through the code for EnumerateRead, I don't think you need this if(count > 0) check. I think it just doesn't do anything if count == 0.

@@ +280,5 @@
> +  if (mAttributes.Count() > 0) {
> +    mAttributes.EnumerateRead(AttributeToArray, mutArray);
> +  }
> +
> +  if (true) {

no need to if(true) :)

Then you can get rid of RV and just return NS_OK.

::: netwerk/cookie/nsCookie.h
@@ +110,5 @@
>      int64_t      mCreationTime;
>      bool mIsSession;
>      bool mIsSecure;
>      bool mIsHttpOnly;
> +    // rather than growing nsCookie every time we add attributes, it makes

"growing nsCookie" -> "modifying the cookie IDL"

Or simply, "Keep a key=value hashtable of cookie attributes to avoid changing the cookie IDL"

::: netwerk/cookie/nsCookieService.cpp
@@ +144,5 @@
>    int64_t expiryTime;
>    bool isSession;
>    bool isSecure;
>    bool isHttpOnly;
> +  nsClassHashtable<nsCStringHashKey, nsCString>  extendedAttributes;

I would prefer a consistent naming of the same data structure across files, e.g. use mExtendedAttributes in nsCookie.{cpp, h}. It is easier for future forgetful developers :)

::: netwerk/cookie/nsICookieManager2.idl
@@ +86,5 @@
> +   *        expiration date, in seconds since midnight (00:00:00), January 1,
> +   *        1970 UTC. note that expiry time will also be honored for session cookies;
> +   *        in this way, the more restrictive of the two will take effect.
> +   * @param aAttributes
> +   *        a string containing additional attributes to set on the cookie.

Please say the expected format of the string.

Comment 31

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.