Refactored out JS specific part of modal dialog stack into its own class, exp...

Refactored out JS specific part of modal dialog stack into its own class, exposed cookie/storage prompt as a modal dialog.

BUG=32719
TEST=none, requires Darin to hook this with his code.


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

[zel, 2010-02-03 09:50:23]

modal app dialog in Chrome has an unintuitive OOD, my changes don't fix that
issue they just expose cookie prompt there

[darin (slow to review), 2010-02-03 21:28:29]

good stuff!  some nits:

http://codereview.chromium.org/560030/diff/4012/5011
File chrome/browser/app_modal_dialog.cc (right):

http://codereview.chromium.org/560030/diff/4012/5011#newcode30
chrome/browser/app_modal_dialog.cc:30: if (tab_contents_)
so it looks like the tab_contents_ parameter to the AppModalDialog constructor
is optional?  that seems helpful, especially in the cookie prompt case, in which
we do not want to change the current tab when a cookie prompt should be shown. 
if this is the intent, please document that tab_contents may be null in the
app_modal_dialog.h.

http://codereview.chromium.org/560030/diff/4012/5012
File chrome/browser/app_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/4012/5012#newcode13
chrome/browser/app_modal_dialog.h:13: #include "net/base/cookie_monster.h"
no need for this include

http://codereview.chromium.org/560030/diff/4012/5012#newcode30
chrome/browser/app_modal_dialog.h:30: class CookiesPromptViewDelegate;
this forward declaration is unnecessary... same goes for some of the others.

http://codereview.chromium.org/560030/diff/4012/5012#newcode38
chrome/browser/app_modal_dialog.h:38: // A controller+model base class for modal
dialogs. |NativeDialog| is a
It seems like the NativeDialog comment should be moved up to where NativeDialog
is defined.

http://codereview.chromium.org/560030/diff/4012/5012#newcode43
chrome/browser/app_modal_dialog.h:43: // show.  |dialog_flags| is a MessageBox
flag.
this comment refers to a parameter named dialog_flags, which doesn't seem to
exist.

http://codereview.chromium.org/560030/diff/4012/5020
File chrome/browser/app_modal_dialog_win.cc (right):

http://codereview.chromium.org/560030/diff/4012/5020#newcode8
chrome/browser/app_modal_dialog_win.cc:8: #include
"chrome/browser/tab_contents/tab_contents.h"
it looks like there are some unnecessary includes here.

http://codereview.chromium.org/560030/diff/4012/5028
File chrome/browser/browser_browsertest.cc (left):

http://codereview.chromium.org/560030/diff/4012/5028#oldcode232
chrome/browser/browser_browsertest.cc:232:
EXPECT_FALSE(alert->is_before_unload_dialog());
It seems like this was a critical element of the test.  See the description of
the bug.  It mentions that we had a problem with two beforeunload dialogs being
shown.  With this line of code removed, the test is now effectively useless.

http://codereview.chromium.org/560030/diff/4012/5019
File chrome/browser/cookie_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/4012/5019#newcode21
chrome/browser/cookie_modal_dialog.h:21: const std::string& domain,
I think the "domain" and "cookie" parameters should be changed to:

  const std::string& host,
  const std::string& cookie_line,

This matches the information being passed up from the network layer.  We can
then in the cookie info view code crack the cookie_line using ParsedCookie. 
There's no need to compute ParsedCookie any earlier.

http://codereview.chromium.org/560030/diff/4012/5019#newcode26
chrome/browser/cookie_modal_dialog.h:26: const std::string& domain,
is this domain parameter needed?  LocalStorageInfo has a host parameter. 
shouldn't that be what we display to the user?

http://codereview.chromium.org/560030/diff/4012/5021
File chrome/browser/js_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/4012/5021#newcode5
chrome/browser/js_modal_dialog.h:5: #ifndef CHROME_BROWSER_JS_MODAL_DIALOG_H_
the name of the file should match the name of the class defined within.  also,
it is JavaScript not Javascript :-)

http://codereview.chromium.org/560030/diff/4012/5017
File chrome/browser/jsmessage_box_handler.h (right):

http://codereview.chromium.org/560030/diff/4012/5017#newcode6
chrome/browser/jsmessage_box_handler.h:6: #define
CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
this file is now misnamed, right?

http://codereview.chromium.org/560030/diff/4012/5024
File chrome/browser/views/cookie_info_view.cc (right):

http://codereview.chromium.org/560030/diff/4012/5024#newcode97
chrome/browser/views/cookie_info_view.cc:97:
domain_value_field_->SetText(UTF8ToWide(domain));
I think we should read the domain from the ParsedCookie if one is set. 
Otherwise, there is no domain property, and the cookie is a host cookie.

I also think it would be better if this function were passed a GURL for the url
associated with the cookie and a std::string cookie_line for the Set-Cookie
header being processed.

Then, inside this function we can use ParsedCookie to crack the cookie_line and
use the GURL's hostname if necessary.

http://codereview.chromium.org/560030/diff/4012/5025
File chrome/browser/views/cookie_info_view.h (right):

http://codereview.chromium.org/560030/diff/4012/5025#newcode52
chrome/browser/views/cookie_info_view.h:52: const
net::CookieMonster::ParsedCookie& cookie_node);
nit: indent so the args are in a column

http://codereview.chromium.org/560030/diff/4012/5013
File chrome/browser/views/jsmessage_box_dialog.h (right):

http://codereview.chromium.org/560030/diff/4012/5013#newcode20
chrome/browser/views/jsmessage_box_dialog.h:20: class ModalDialogDelegate :
public views::DialogDelegate {
we should break ModalDialogDelegate out into a separate haeder file named
modal_dialog_delegate.h

http://codereview.chromium.org/560030/diff/4012/5013#newcode35
chrome/browser/views/jsmessage_box_dialog.h:35: class JavascriptMessageBoxDialog
: public ModalDialogDelegate {
Javascript -> JavaScript

http://codereview.chromium.org/560030/diff/4012/5027
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/560030/diff/4012/5027#newcode249
net/base/cookie_monster.h:249: ParsedCookie() {}
these changes become unnecessary if we just pass around cookie_line as I
suggested above.

[zel, 2010-02-04 01:38:55]

this patch set covers most of Darin's comments and Linux build... I still have
Mac side to complete there.

http://codereview.chromium.org/560030/diff/4012/5011
File chrome/browser/app_modal_dialog.cc (right):

http://codereview.chromium.org/560030/diff/4012/5011#newcode30
chrome/browser/app_modal_dialog.cc:30: if (tab_contents_)
On 2010/02/03 21:28:29, darin wrote:
> so it looks like the tab_contents_ parameter to the AppModalDialog constructor
> is optional?  that seems helpful, especially in the cookie prompt case, in
which
> we do not want to change the current tab when a cookie prompt should be shown.

> if this is the intent, please document that tab_contents may be null in the
> app_modal_dialog.h.

Done. Having it optional caused some other minor changes as well.

http://codereview.chromium.org/560030/diff/4012/5012
File chrome/browser/app_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/4012/5012#newcode13
chrome/browser/app_modal_dialog.h:13: #include "net/base/cookie_monster.h"
On 2010/02/03 21:28:29, darin wrote:
> no need for this include

Done.

http://codereview.chromium.org/560030/diff/4012/5012#newcode30
chrome/browser/app_modal_dialog.h:30: class CookiesPromptViewDelegate;
On 2010/02/03 21:28:29, darin wrote:
> this forward declaration is unnecessary... same goes for some of the others.

Yeah, many cut-and-pastes in this file... Done.

http://codereview.chromium.org/560030/diff/4012/5012#newcode38
chrome/browser/app_modal_dialog.h:38: // A controller+model base class for modal
dialogs. |NativeDialog| is a
On 2010/02/03 21:28:29, darin wrote:
> It seems like the NativeDialog comment should be moved up to where
NativeDialog
> is defined.

Done.

http://codereview.chromium.org/560030/diff/4012/5012#newcode43
chrome/browser/app_modal_dialog.h:43: // show.  |dialog_flags| is a MessageBox
flag.
On 2010/02/03 21:28:29, darin wrote:
> this comment refers to a parameter named dialog_flags, which doesn't seem to
> exist.

Done.

http://codereview.chromium.org/560030/diff/4012/5020
File chrome/browser/app_modal_dialog_win.cc (right):

http://codereview.chromium.org/560030/diff/4012/5020#newcode8
chrome/browser/app_modal_dialog_win.cc:8: #include
"chrome/browser/tab_contents/tab_contents.h"
On 2010/02/03 21:28:29, darin wrote:
> it looks like there are some unnecessary includes here.

Done.

http://codereview.chromium.org/560030/diff/4012/5028
File chrome/browser/browser_browsertest.cc (left):

http://codereview.chromium.org/560030/diff/4012/5028#oldcode232
chrome/browser/browser_browsertest.cc:232:
EXPECT_FALSE(alert->is_before_unload_dialog());
On 2010/02/03 21:28:29, darin wrote:
> It seems like this was a critical element of the test.  See the description of
> the bug.  It mentions that we had a problem with two beforeunload dialogs
being
> shown.  With this line of code removed, the test is now effectively useless.

I see your point. The problem is that we are expecting one specific type of the
dialog here from a queue of generic dialogs.

http://codereview.chromium.org/560030/diff/4012/5019
File chrome/browser/cookie_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/4012/5019#newcode21
chrome/browser/cookie_modal_dialog.h:21: const std::string& domain,
On 2010/02/03 21:28:29, darin wrote:
> I think the "domain" and "cookie" parameters should be changed to:
> 
>   const std::string& host,
>   const std::string& cookie_line,
> 
> This matches the information being passed up from the network layer.  We can
> then in the cookie info view code crack the cookie_line using ParsedCookie. 
> There's no need to compute ParsedCookie any earlier.

Done.

http://codereview.chromium.org/560030/diff/4012/5019#newcode26
chrome/browser/cookie_modal_dialog.h:26: const std::string& domain,
On 2010/02/03 21:28:29, darin wrote:
> is this domain parameter needed?  LocalStorageInfo has a host parameter. 
> shouldn't that be what we display to the user?

Right, I guess we can get this from LocalStorageInfo.host instead. Done.

http://codereview.chromium.org/560030/diff/4012/5017
File chrome/browser/jsmessage_box_handler.h (right):

http://codereview.chromium.org/560030/diff/4012/5017#newcode6
chrome/browser/jsmessage_box_handler.h:6: #define
CHROME_BROWSER_JSMESSAGE_BOX_HANDLER_H_
On 2010/02/03 21:28:29, darin wrote:
> this file is now misnamed, right?

Yep. Renamed to message_box_handler.* .

http://codereview.chromium.org/560030/diff/4012/5024
File chrome/browser/views/cookie_info_view.cc (right):

http://codereview.chromium.org/560030/diff/4012/5024#newcode97
chrome/browser/views/cookie_info_view.cc:97:
domain_value_field_->SetText(UTF8ToWide(domain));
On 2010/02/03 21:28:29, darin wrote:
> I think we should read the domain from the ParsedCookie if one is set. 
> Otherwise, there is no domain property, and the cookie is a host cookie.
> 
> I also think it would be better if this function were passed a GURL for the
url
> associated with the cookie and a std::string cookie_line for the Set-Cookie
> header being processed.
> 
> Then, inside this function we can use ParsedCookie to crack the cookie_line
and
> use the GURL's hostname if necessary.

Done.

http://codereview.chromium.org/560030/diff/4012/5025
File chrome/browser/views/cookie_info_view.h (right):

http://codereview.chromium.org/560030/diff/4012/5025#newcode52
chrome/browser/views/cookie_info_view.h:52: const
net::CookieMonster::ParsedCookie& cookie_node);
On 2010/02/03 21:28:29, darin wrote:
> nit: indent so the args are in a column

Done.

http://codereview.chromium.org/560030/diff/4012/5013
File chrome/browser/views/jsmessage_box_dialog.h (right):

http://codereview.chromium.org/560030/diff/4012/5013#newcode20
chrome/browser/views/jsmessage_box_dialog.h:20: class ModalDialogDelegate :
public views::DialogDelegate {
On 2010/02/03 21:28:29, darin wrote:
> we should break ModalDialogDelegate out into a separate haeder file named
> modal_dialog_delegate.h

Done.

http://codereview.chromium.org/560030/diff/4012/5013#newcode35
chrome/browser/views/jsmessage_box_dialog.h:35: class JavascriptMessageBoxDialog
: public ModalDialogDelegate {
On 2010/02/03 21:28:29, darin wrote:
> Javascript -> JavaScript

Done.

http://codereview.chromium.org/560030/diff/4012/5027
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/560030/diff/4012/5027#newcode249
net/base/cookie_monster.h:249: ParsedCookie() {}
On 2010/02/03 21:28:29, darin wrote:
> these changes become unnecessary if we just pass around cookie_line as I
> suggested above.

Done.

[zel, 2010-02-04 01:43:05]

The latest patch should cover all Darin's comments and Linux build break. I am
still working on Mac build issues.

[zel, 2010-02-04 01:49:21]

http://codereview.chromium.org/560030/diff/4012/5028
File chrome/browser/browser_browsertest.cc (left):

http://codereview.chromium.org/560030/diff/4012/5028#oldcode232
chrome/browser/browser_browsertest.cc:232:
EXPECT_FALSE(alert->is_before_unload_dialog());
On 2010/02/04 01:38:55, zel wrote:
> On 2010/02/03 21:28:29, darin wrote:
> > It seems like this was a critical element of the test.  See the description
of
> > the bug.  It mentions that we had a problem with two beforeunload dialogs
> being
> > shown.  With this line of code removed, the test is now effectively useless.
> 
> I see your point. The problem is that we are expecting one specific type of
the
> dialog here from a queue of generic dialogs.

Btw, this part is still not fixed. I am working on it right now.

[zel, 2010-02-04 04:30:56]

Jochen is working on mac part of this change, the rest of it should be done.

http://codereview.chromium.org/560030/diff/4012/5028
File chrome/browser/browser_browsertest.cc (left):

http://codereview.chromium.org/560030/diff/4012/5028#oldcode232
chrome/browser/browser_browsertest.cc:232:
EXPECT_FALSE(alert->is_before_unload_dialog());
On 2010/02/04 01:49:21, zel wrote:
> On 2010/02/04 01:38:55, zel wrote:
> > On 2010/02/03 21:28:29, darin wrote:
> > > It seems like this was a critical element of the test.  See the
description
> of
> > > the bug.  It mentions that we had a problem with two beforeunload dialogs
> > being
> > > shown.  With this line of code removed, the test is now effectively
useless.
> > 
> > I see your point. The problem is that we are expecting one specific type of
> the
> > dialog here from a queue of generic dialogs.
> 
> Btw, this part is still not fixed. I am working on it right now.

This is taken care of now.

[darin (slow to review), 2010-02-04 06:20:47]

http://codereview.chromium.org/560030/diff/13015/12026
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/560030/diff/13015/12026#newcode233
chrome/browser/browser_browsertest.cc:233:
EXPECT_FALSE(reinterpret_cast<JavaScriptAppModalDialog*>(alert)->
please use static_cast here.

http://codereview.chromium.org/560030/diff/13015/12029
File chrome/browser/cookie_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/13015/12029#newcode43
chrome/browser/cookie_modal_dialog.h:43: GURL url_;
nit: fix indentation

http://codereview.chromium.org/560030/diff/13015/12021
File chrome/browser/views/cookie_prompt_view.cc (right):

http://codereview.chromium.org/560030/diff/13015/12021#newcode49
chrome/browser/views/cookie_prompt_view.cc:49:
InitializeViewResources(cookie_.HasDomain() ? cookie_.Domain() : url.host());
does this mean that we will claim that "google.com" is setting a cookie for
".google.com" when actually the user visited www.google.com?  shouldn't we
"blame" the hostname and then in the cookie details show the domain for which
the cookie is being set (provided one was specified)?

http://codereview.chromium.org/560030/diff/13015/12021#newcode303
chrome/browser/views/cookie_prompt_view.cc:303: display_domain =
display_domain.substr(1);
it looks like this line should have been indented.  would be good to fix this
while you're in this file.

http://codereview.chromium.org/560030/diff/13015/12014
File net/base/cookie_monster.h (left):

http://codereview.chromium.org/560030/diff/13015/12014#oldcode296
net/base/cookie_monster.h:296: DISALLOW_COPY_AND_ASSIGN(ParsedCookie);
ditto

http://codereview.chromium.org/560030/diff/13015/12014
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/560030/diff/13015/12014#newcode251
net/base/cookie_monster.h:251: ParsedCookie() {}
is this change really necessary?

[darin (slow to review), 2010-02-04 07:35:15]

http://codereview.chromium.org/560030/diff/13015/12020
File chrome/browser/views/cookie_prompt_view.h (right):

http://codereview.chromium.org/560030/diff/13015/12020#newcode33
chrome/browser/views/cookie_prompt_view.h:33: class CookiesPromptViewDelegate {
ben tells me that interfaces like this, which are shred by all ports of chrome
should not be defined within the chrome/browser/views directory.  this one
should probably live in chrome/browser instead.

in fact, it should probably be renamed CookiePromptModalDialogDelegate since
from an API perspective it corresponds better to CookiePromptModalDialog.

also, i notice that some of the cookie related interfaces start with "Cookie"
and others start with "Cookies".  can you pick one and use it consistently?

[Ben Goodger (Google), 2010-02-04 07:45:53]

http://codereview.chromium.org/560030/diff/13015/12029
File chrome/browser/cookie_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/13015/12029#newcode50
chrome/browser/cookie_modal_dialog.h:50: // Whether we're showing cookie UI as
opposed to other site data.
nit: NL above

http://codereview.chromium.org/560030/diff/13015/12029#newcode56
chrome/browser/cookie_modal_dialog.h:56: };
DISALLOW_COPY_AND_ASSIGN...

http://codereview.chromium.org/560030/diff/13015/12031
File chrome/browser/cookie_modal_dialog_win.cc (right):

http://codereview.chromium.org/560030/diff/13015/12031#newcode10
chrome/browser/cookie_modal_dialog_win.cc:10: #include "views/window/window.h"
This is likely to be ChromeOS applicable too. It should move into browser/views
abd be named CookieModalDialogViews.

http://codereview.chromium.org/560030/diff/13015/12031#newcode18
chrome/browser/cookie_modal_dialog_win.cc:18:
dialog_->window()->GetClientView()->AsDialogClientView();
nit: 4-space indent

http://codereview.chromium.org/560030/diff/13015/12031#newcode24
chrome/browser/cookie_modal_dialog_win.cc:24:
dialog_->window()->GetClientView()->AsDialogClientView();
nit: 4-space indent

http://codereview.chromium.org/560030/diff/13015/12030
File chrome/browser/js_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/13015/12030#newcode95
chrome/browser/js_modal_dialog.h:95: };
DISALLOW_COPY_AND_ASSIGN...

http://codereview.chromium.org/560030/diff/13015/12032
File chrome/browser/js_modal_dialog_win.cc (right):

http://codereview.chromium.org/560030/diff/13015/12032#newcode3
chrome/browser/js_modal_dialog_win.cc:3: // found in the LICENSE file.
Likewise, JSModalDialogViews, and move to browser/views dir. This should be
built on ChromeOS too.

[zel, 2010-02-04 21:57:26]

http://codereview.chromium.org/560030/diff/13015/12026
File chrome/browser/browser_browsertest.cc (right):

http://codereview.chromium.org/560030/diff/13015/12026#newcode233
chrome/browser/browser_browsertest.cc:233:
EXPECT_FALSE(reinterpret_cast<JavaScriptAppModalDialog*>(alert)->
On 2010/02/04 06:20:47, darin wrote:
> please use static_cast here.

Done.

http://codereview.chromium.org/560030/diff/13015/12029
File chrome/browser/cookie_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/13015/12029#newcode43
chrome/browser/cookie_modal_dialog.h:43: GURL url_;
On 2010/02/04 06:20:47, darin wrote:
> nit: fix indentation

Done.

http://codereview.chromium.org/560030/diff/13015/12029#newcode50
chrome/browser/cookie_modal_dialog.h:50: // Whether we're showing cookie UI as
opposed to other site data.
On 2010/02/04 07:45:53, Ben Goodger wrote:
> nit: NL above

Done.

http://codereview.chromium.org/560030/diff/13015/12029#newcode56
chrome/browser/cookie_modal_dialog.h:56: };
On 2010/02/04 07:45:53, Ben Goodger wrote:
> DISALLOW_COPY_AND_ASSIGN...

Done.

http://codereview.chromium.org/560030/diff/13015/12031
File chrome/browser/cookie_modal_dialog_win.cc (right):

http://codereview.chromium.org/560030/diff/13015/12031#newcode10
chrome/browser/cookie_modal_dialog_win.cc:10: #include "views/window/window.h"
On 2010/02/04 07:45:53, Ben Goodger wrote:
> This is likely to be ChromeOS applicable too. It should move into
browser/views
> abd be named CookieModalDialogViews.

This is not really a view class but model+controller for modal dialog. Ignoring
this comment after discussion with Ben.

http://codereview.chromium.org/560030/diff/13015/12031#newcode18
chrome/browser/cookie_modal_dialog_win.cc:18:
dialog_->window()->GetClientView()->AsDialogClientView();
On 2010/02/04 07:45:53, Ben Goodger wrote:
> nit: 4-space indent

Done.

http://codereview.chromium.org/560030/diff/13015/12031#newcode24
chrome/browser/cookie_modal_dialog_win.cc:24:
dialog_->window()->GetClientView()->AsDialogClientView();
On 2010/02/04 07:45:53, Ben Goodger wrote:
> nit: 4-space indent

Done.

http://codereview.chromium.org/560030/diff/13015/12030
File chrome/browser/js_modal_dialog.h (right):

http://codereview.chromium.org/560030/diff/13015/12030#newcode95
chrome/browser/js_modal_dialog.h:95: };
On 2010/02/04 07:45:53, Ben Goodger wrote:
> DISALLOW_COPY_AND_ASSIGN...

Done.

http://codereview.chromium.org/560030/diff/13015/12032
File chrome/browser/js_modal_dialog_win.cc (right):

http://codereview.chromium.org/560030/diff/13015/12032#newcode3
chrome/browser/js_modal_dialog_win.cc:3: // found in the LICENSE file.
On 2010/02/04 07:45:53, Ben Goodger wrote:
> Likewise, JSModalDialogViews, and move to browser/views dir. This should be
> built on ChromeOS too.

Ignoring this comment after discussion with Ben.

http://codereview.chromium.org/560030/diff/13015/12021
File chrome/browser/views/cookie_prompt_view.cc (right):

http://codereview.chromium.org/560030/diff/13015/12021#newcode49
chrome/browser/views/cookie_prompt_view.cc:49:
InitializeViewResources(cookie_.HasDomain() ? cookie_.Domain() : url.host());
On 2010/02/04 06:20:47, darin wrote:
> does this mean that we will claim that "google.com" is setting a cookie for
> ".google.com" when actually the user visited http://www.google.com?  shouldn't
we
> "blame" the hostname and then in the cookie details show the domain for which
> the cookie is being set (provided one was specified)?

This is taken care of in Darin's CL.

http://codereview.chromium.org/560030/diff/13015/12021#newcode303
chrome/browser/views/cookie_prompt_view.cc:303: display_domain =
display_domain.substr(1);
On 2010/02/04 06:20:47, darin wrote:
> it looks like this line should have been indented.  would be good to fix this
> while you're in this file.

Done.

http://codereview.chromium.org/560030/diff/13015/12020
File chrome/browser/views/cookie_prompt_view.h (right):

http://codereview.chromium.org/560030/diff/13015/12020#newcode33
chrome/browser/views/cookie_prompt_view.h:33: class CookiesPromptViewDelegate {
On 2010/02/04 07:35:15, darin wrote:
> ben tells me that interfaces like this, which are shred by all ports of chrome
> should not be defined within the chrome/browser/views directory.  this one
> should probably live in chrome/browser instead.
> 
> in fact, it should probably be renamed CookiePromptModalDialogDelegate since
> from an API perspective it corresponds better to CookiePromptModalDialog.
> 
> also, i notice that some of the cookie related interfaces start with "Cookie"
> and others start with "Cookies".  can you pick one and use it consistently?

Done. Renamed everything to *Cookie*

http://codereview.chromium.org/560030/diff/13015/12014
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/560030/diff/13015/12014#newcode251
net/base/cookie_monster.h:251: ParsedCookie() {}
On 2010/02/04 06:20:47, darin wrote:
> is this change really necessary?

Unfortunately, it is. CookiePromptView has the second constructor (one for local
storage info) that won't be able to instantiate ParsedCookie in any other way.

This is a long story. I originally had scoped_ptr<ParsedCookie> in
CookiePromptView instead, but Peter clubbed my original CL to death and insisted
that we add such constructors where needed.

[Ben Goodger (Google), 2010-02-05 21:17:53]

BTW from my perspective now this LGTM. Darin may have addtl comments.

[darin (slow to review), 2010-02-05 21:20:49]

> http://codereview.chromium.org/560030/diff/13015/12014#newcode251
> net/base/cookie_monster.h:251: ParsedCookie() {}
> On 2010/02/04 06:20:47, darin wrote:
> > is this change really necessary?
> 
> Unfortunately, it is. CookiePromptView has the second constructor (one for
local
> storage info) that won't be able to instantiate ParsedCookie in any other way.
> 
> This is a long story. I originally had scoped_ptr<ParsedCookie> in
> CookiePromptView instead, but Peter clubbed my original CL to death and
insisted
> that we add such constructors where needed.

I think there is no need for ParsedCookie to be a member variable.  It can just
be instantiated on the stack within the SetCookieString method.

-Darin

[darin (slow to review), 2010-02-05 21:39:57]

LGTM