Add a mutable version of CookieMonster::ParsedCookie

Add a mutable version of CookieMonster::ParsedCookie

This CL implements CookieMonster::MutableParsedCookie which is derived from CookieMonster::ParsedCookie. This allows parsing a cookie and modifying it afterwards.

BUG=112155
TEST=no


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149622

[battre, 2012-06-29 15:18:44]

Hi Erik,

this is the foundation for a CL that will introduce the ability to modify
cookies of URLRequests via the declarative WebRequest API
(https://chromiumcodereview.appspot.com/10447090/).

Given that there has been a lot of work done for the
CookieMonster::ParsedCookie, I think it makes sense to reuse this in the
declarative WebRequest API. Could you please review this CL?

Thanks,
Dominic

[erikwright (departed), 2012-06-29 15:57:10]

FYI I'm on leave til the 9th. If it can wait, please do, otherwise wtc can
recommend a reviewer from the core net team.
On Jun 29, 2012 11:18 AM, <battre@chromium.org> wrote:

> Reviewers: erikwright,
>
> Message:
> Hi Erik,
>
> this is the foundation for a CL that will introduce the ability to modify
> cookies of URLRequests via the declarative WebRequest API
>
(https://chromiumcodereview.**appspot.com/10447090/<https://chromiumcodereview...
> ).
>
> Given that there has been a lot of work done for the
> CookieMonster::ParsedCookie, I think it makes sense to reuse this in the
> declarative WebRequest API. Could you please review this CL?
>
> Thanks,
> Dominic
>
>
> Description:
> Add a mutable version of CookieMonster::ParsedCookie
>
> This CL implements CookieMonster::**MutableParsedCookie which is derived
> from
> CookieMonster::ParsedCookie. This allows parsing a cookie and modifying it
> afterwards.
>
> BUG=112155
> TEST=no
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10697035/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M net/cookies/cookie_monster.h
>  M net/cookies/cookie_monster.cc
>  M net/cookies/cookie_monster_**unittest.cc
>
>
>

[battre, 2012-06-29 16:01:20]

Thanks for the heads up. I can wait. Enjoy your vacation.

Best regards,
Dominic


On 2012/06/29 15:57:10, erikwright wrote:
> FYI I'm on leave til the 9th. If it can wait, please do, otherwise wtc can
> recommend a reviewer from the core net team.
> On Jun 29, 2012 11:18 AM, <mailto:battre@chromium.org> wrote:
> 
> > Reviewers: erikwright,
> >
> > Message:
> > Hi Erik,
> >
> > this is the foundation for a CL that will introduce the ability to modify
> > cookies of URLRequests via the declarative WebRequest API
> >
>
(https://chromiumcodereview.**appspot.com/10447090/%3Chttps://chromiumcoderevi...>
> > ).
> >
> > Given that there has been a lot of work done for the
> > CookieMonster::ParsedCookie, I think it makes sense to reuse this in the
> > declarative WebRequest API. Could you please review this CL?
> >
> > Thanks,
> > Dominic
> >
> >
> > Description:
> > Add a mutable version of CookieMonster::ParsedCookie
> >
> > This CL implements CookieMonster::**MutableParsedCookie which is derived
> > from
> > CookieMonster::ParsedCookie. This allows parsing a cookie and modifying it
> > afterwards.
> >
> > BUG=112155
> > TEST=no
> >
> >
> > Please review this at
>
https://chromiumcodereview.**appspot.com/10697035/%3Chttps://chromiumcoderevi...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >  M net/cookies/cookie_monster.h
> >  M net/cookies/cookie_monster.cc
> >  M net/cookies/cookie_monster_**unittest.cc
> >
> >
> >

[erikwright (departed), 2012-07-10 13:55:49]

I don't see any benefit to doing this as a subclass vs. just making the base
class mutable.

For example, it doesn't stop someone from passing in a 'ParsedCookie' and
changing it later. They can just as easily pass in a MutableParsedCookie cast
down to a ParsedCookie. The CookieMonster will be none the wiser.

I'd suggest, instead, simply making ParsedCookie mutable. As it happens, no one
ever holds onto a parsed cookie that they've been given, anyway, so they won't
be surprised if the caller changes it later.

If someone _does_ want to hold onto a parsed cookie, they should probably copy
it. (If that's the case for your CL, remove the DISALLOW and verify that copying
works as expected).

Please also move this to its own .h/.cc (parsed_cookie.*). Either do that as a
separate CL before this one, or do the move (with no other changes other than to
make it compile properly) and then upload subsequent patchsets for further
modifications. This will make it easier to review.

[battre, 2012-07-11 15:30:34]

PTAL

Thanks,
Dominic

[erikwright (departed), 2012-07-12 15:50:56]

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie.cc
File net/cookies/parsed_cookie.cc (right):

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:99: static inline bool CharIsA(const char c, const
char* chars) {
I guess all of these should go into an anonymous namespace?

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:460: // Reserve first spot for cooie name and
value.
Seems to me it would be better to fail if there is no name/value already.

Previously there was no way to have a parsed cookie where IsValid() == true and
name/value pair is empty.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:487: is_valid_ = pairs_.empty();
It seems that is_valid_ can never be mutated by this method, because you never
delete the first pair, and the definition of is_valid_ is basically "Is
pairs_.length() > 0?".

In fact, perhaps it makes sense to actually remove the variable and redefine
IsValid() to "return pairs_.length() > 0;"

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:488: return true;
If you won't ever return false, change return type to void.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie.h
File net/cookies/parsed_cookie.h (right):

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.h:73: // ToCookieLine() is the new DebugString();
Remove the "new DebugString()" note.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie_...
File net/cookies/parsed_cookie_unittest.cc (right):

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie_...
net/cookies/parsed_cookie_unittest.cc:289: TEST(ParsedCookieTest, SetValues) {
Add checks for invalid name/value.
Add construction of a ParsedCookie with no name/value (i.e., invalid) and ensure
correctness of IsValid(), including correct handling of setting attributes and
setting the name/value.
Add check for clearing an attribute that was previously unset.

[battre, 2012-07-12 18:03:10]

Thanks for the quick review.

Best regards,
Dominic

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie.cc
File net/cookies/parsed_cookie.cc (right):

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:99: static inline bool CharIsA(const char c, const
char* chars) {
On 2012/07/12 15:50:57, erikwright wrote:
> I guess all of these should go into an anonymous namespace?

Done.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:460: // Reserve first spot for cooie name and
value.
On 2012/07/12 15:50:57, erikwright wrote:
> Seems to me it would be better to fail if there is no name/value already.
> 
> Previously there was no way to have a parsed cookie where IsValid() == true
and
> name/value pair is empty.

Done.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:487: is_valid_ = pairs_.empty();
On 2012/07/12 15:50:57, erikwright wrote:
> It seems that is_valid_ can never be mutated by this method, because you never
> delete the first pair, and the definition of is_valid_ is basically "Is
> pairs_.length() > 0?".
> 
> In fact, perhaps it makes sense to actually remove the variable and redefine
> IsValid() to "return pairs_.length() > 0;"

I have redefined IsValid() as you have suggested.

Done.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.cc:488: return true;
On 2012/07/12 15:50:57, erikwright wrote:
> If you won't ever return false, change return type to void.

Done, even thought this comes at the cost of making all Set... functions
somewhat uglier.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie.h
File net/cookies/parsed_cookie.h (right):

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie....
net/cookies/parsed_cookie.h:73: // ToCookieLine() is the new DebugString();
On 2012/07/12 15:50:57, erikwright wrote:
> Remove the "new DebugString()" note.

Done.

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie_...
File net/cookies/parsed_cookie_unittest.cc (right):

http://codereview.chromium.org/10697035/diff/12001/net/cookies/parsed_cookie_...
net/cookies/parsed_cookie_unittest.cc:289: TEST(ParsedCookieTest, SetValues) {
On 2012/07/12 15:50:57, erikwright wrote:
> Add checks for invalid name/value.
> Add construction of a ParsedCookie with no name/value (i.e., invalid) and
ensure
> correctness of IsValid(), including correct handling of setting attributes and
> setting the name/value.
> Add check for clearing an attribute that was previously unset.

Done.

[battre, 2012-07-18 17:08:10]

The refactoring of the cookie monster is done and landed. Could you please have
another look at this CL?

Thanks,
Dominic

[erikwright (departed), 2012-07-18 17:20:31]

LGTM.

https://chromiumcodereview.appspot.com/10697035/diff/14001/net/cookies/parsed...
File net/cookies/parsed_cookie.cc (right):

https://chromiumcodereview.appspot.com/10697035/diff/14001/net/cookies/parsed...
net/cookies/parsed_cookie.cc:199: bool ParsedCookie::SetPath(const std::string&
path) {
If you're concerned about these being ugly/repetitive, I guess you could factor
this out into a private method SetString:

bool ParsedCookie::SetString(size_t* index, const std::string& key, const
std::string& value) {
  if (value.empty()) {
    ClearAttributePair(*index);
    return true;
  } else {
    return SetAttributePair(index, key, value);
  }
}

bool ParsedCookie::SetPath(const std::string& path) {
  return SetString(&path_index_, kPathTokenName, path);
}

Only the two bool ones differ from that.

I'll leave it up to you to decide if that's better or not.

[commit-bot: I haz the power, 2012-08-02 11:57:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10697035/25001

[commit-bot: I haz the power, 2012-08-02 13:37:43]

Change committed as 149622