Avoid showing the user a garbage path attribute when the cookie doesn't have ...

Avoid showing the user a garbage path attribute when the cookie doesn't have an
explicit path set.  This CL also ensures that we set the expiry info properly.

This means using CanonPath and CanonExpiration from cookie_monster.cc.  Instead
of exposing those methods, I just expose a CanonicalCookie constructor that
takes a ParsedCookie.

R=pkasting
BUG=none
TEST=none


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

[Peter Kasting, 2010-02-11 00:52:03]

LGTM

[Peter Kasting, 2010-02-11 00:53:49]

On Wed, Feb 10, 2010 at 4:52 PM, <pkasting@chromium.org> wrote:

> LGTM


Actually maybe we should show / to match the main cookie dialog?  Or am I
testing the wrong thing?

Whatever we do, hopefully we can share code...

http://codereview.chromium.org/597031
>

[darin (slow to review), 2010-02-11 08:27:20]

Ah yes... it should be "/" instead of "".  I'll look into sharing code for
this.  I suspect CookieMonster must be initializing the CanonicalCookie with
a path of "/", which means that I'll just have to replicate the same thing
when I manually construct a CanonicalCookie inside CookieInfoView.

Thanks!
-Darin

On Wed, Feb 10, 2010 at 4:53 PM, Peter Kasting <pkasting@chromium.org>wrote:

> On Wed, Feb 10, 2010 at 4:52 PM, <pkasting@chromium.org> wrote:
>
>> LGTM
>
>
> Actually maybe we should show / to match the main cookie dialog?  Or am I
> testing the wrong thing?
>
> Whatever we do, hopefully we can share code...
>
> http://codereview.chromium.org/597031
>>
>
>

[darin (slow to review), 2010-02-11 08:30:57]

Actually, it looks like we need to use the CanonPath function defined in
cookie_monster.cc.  It is a function static, so I'll need to expose it
somehow.  Maybe we should just expose a function to construct a
CanonicalCookie from a ParsedCookie.

-Darin

On Thu, Feb 11, 2010 at 12:27 AM, Darin Fisher <darin@chromium.org> wrote:

> Ah yes... it should be "/" instead of "".  I'll look into sharing code for
> this.  I suspect CookieMonster must be initializing the CanonicalCookie with
> a path of "/", which means that I'll just have to replicate the same thing
> when I manually construct a CanonicalCookie inside CookieInfoView.
>
> Thanks!
> -Darin
>
>
> On Wed, Feb 10, 2010 at 4:53 PM, Peter Kasting <pkasting@chromium.org>wrote:
>
>> On Wed, Feb 10, 2010 at 4:52 PM, <pkasting@chromium.org> wrote:
>>
>>> LGTM
>>
>>
>> Actually maybe we should show / to match the main cookie dialog?  Or am I
>> testing the wrong thing?
>>
>> Whatever we do, hopefully we can share code...
>>
>> http://codereview.chromium.org/597031
>>>
>>
>>
>

[darin (slow to review), 2010-02-11 22:13:25]

Hi Peter,

This change got much more interesting.  Please take another look :)

-Darin


On Thu, Feb 11, 2010 at 2:09 PM, <darin@chromium.org> wrote:

> http://codereview.chromium.org/597031
>

[Peter Kasting, 2010-02-11 22:20:58]

LGTM

http://codereview.chromium.org/597031/diff/1003/15
File chrome/browser/net/chrome_cookie_policy.cc (right):

http://codereview.chromium.org/597031/diff/1003/15#newcode149
chrome/browser/net/chrome_cookie_policy.cc:149: const std::string& host =
data.url.host();
You're welcome to also add a const GURL& url temp here if you want.

http://codereview.chromium.org/597031/diff/1003/11
File chrome/browser/views/cookie_prompt_view.cc (right):

http://codereview.chromium.org/597031/diff/1003/11#newcode286
chrome/browser/views/cookie_prompt_view.cc:286: if (cookie_ui_) {
Nit: No {}.  I'd just do
host_ = cookie_ui_ ? url_.host() : local_storage_info_.host;

http://codereview.chromium.org/597031/diff/1003/11#newcode291
chrome/browser/views/cookie_prompt_view.cc:291: DCHECK(host_.empty() || host_[0]
!= '.');
If the host is empty, doesn't the GetStringF() call return a strange-reading
string?

http://codereview.chromium.org/597031/diff/1003/6
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/597031/diff/1003/6#newcode1
net/base/cookie_monster.cc:1: // Copyright (c) 2006-2008 The Chromium Authors.
All rights reserved.
Nit: 2010

http://codereview.chromium.org/597031/diff/1003/5
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/597031/diff/1003/5#newcode1
net/base/cookie_monster.h:1: // Copyright (c) 2006-2008 The Chromium Authors.
All rights reserved.
Nit: 2010

[darin (slow to review), 2010-02-12 06:24:02]

http://codereview.chromium.org/597031/diff/1003/15
File chrome/browser/net/chrome_cookie_policy.cc (right):

http://codereview.chromium.org/597031/diff/1003/15#newcode149
chrome/browser/net/chrome_cookie_policy.cc:149: const std::string& host =
data.url.host();
On 2010/02/11 22:20:58, Peter Kasting wrote:
> You're welcome to also add a const GURL& url temp here if you want.

naw, that wouldn't buy much.  data.url is just as cheap.  the host temp avoids
having to recreate the string object returned by url.host().

http://codereview.chromium.org/597031/diff/1003/11
File chrome/browser/views/cookie_prompt_view.cc (right):

http://codereview.chromium.org/597031/diff/1003/11#newcode286
chrome/browser/views/cookie_prompt_view.cc:286: if (cookie_ui_) {
On 2010/02/11 22:20:58, Peter Kasting wrote:
> Nit: No {}.  I'd just do
> host_ = cookie_ui_ ? url_.host() : local_storage_info_.host;

Done.

http://codereview.chromium.org/597031/diff/1003/11#newcode291
chrome/browser/views/cookie_prompt_view.cc:291: DCHECK(host_.empty() || host_[0]
!= '.');
The host shouldn't be empty.  I'm actually going to remove this assert.  It is
leftovers from a time when this code thought it was displaying a domain string.

http://codereview.chromium.org/597031/diff/1003/6
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/597031/diff/1003/6#newcode1
net/base/cookie_monster.cc:1: // Copyright (c) 2006-2008 The Chromium Authors.
All rights reserved.
On 2010/02/11 22:20:58, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/597031/diff/1003/5
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/597031/diff/1003/5#newcode1
net/base/cookie_monster.h:1: // Copyright (c) 2006-2008 The Chromium Authors.
All rights reserved.
On 2010/02/11 22:20:58, Peter Kasting wrote:
> Nit: 2010

Done.