Do not reset the content settings delegate's cookies when a network error occurred.

Do not reset the content settings delegate's cookies when a network error occurred.

BUG=63649
TEST=RenderViewHostTest.RedirectLoopCookies

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

[jochen (gone - plz use gerrit), 2010-11-24 14:46:38]

please review

[darin (slow to review), 2010-11-24 17:14:07]

http://codereview.chromium.org/5254005/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5254005/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.cc:2143: if (url.is_valid())
it seems like we should have a more direct signal that we're loading an error
page.  i think it could also be reasonable for the url parameter to contain a
valid URL in the case that we are loading an error page, so it seems fairly
fragile to rely on this signal here.  also, can you add a regression test?

[jochen (gone - plz use gerrit), 2010-11-25 12:27:35]

I made the signal more explicit, and added an ui test.

[Paweł Hajdan Jr., 2010-11-25 17:59:41]

Drive-by with an automation comment. No need to wait for another review by me.

By the way, it would be easier to write this as a browser test.

http://codereview.chromium.org/5254005/diff/7001/chrome/common/automation_mes...
File chrome/common/automation_messages_internal.h (right):

http://codereview.chromium.org/5254005/diff/7001/chrome/common/automation_mes...
chrome/common/automation_messages_internal.h:1402:
IPC_SYNC_MESSAGE_ROUTED2_2(AutomationMsg_IsContentBlocked,
Always add new automation messages at the end of the file, to make sure this
won't break tests using reference build (the message IDs shift when adding in
the middle).

[jochen (gone - plz use gerrit), 2010-11-25 18:47:22]

On 2010/11/25 17:59:41, Paweł Hajdan Jr. wrote:
> Drive-by with an automation comment. No need to wait for another review by me.
> 
> By the way, it would be easier to write this as a browser test.

Could you please explain way this would be easier?

[Paweł Hajdan Jr., 2010-11-25 19:00:52]

On Thu, Nov 25, 2010 at 19:47, <jochen@chromium.org> wrote:

> On 2010/11/25 17:59:41, Paweł Hajdan Jr. wrote:
>
>> Drive-by with an automation comment. No need to wait for another review by
>> me.
>>
>
>  By the way, it would be easier to write this as a browser test.
>>
>
> Could you please explain way this would be easier?


Sure. Instead of adding new automation messages and some boilerplate, you
could access the needed data directly. This also means modifying less files,
etc.

[jochen (gone - plz use gerrit), 2010-11-25 19:05:58]

On 2010/11/25 19:00:52, Paweł Hajdan Jr. wrote:
> On Thu, Nov 25, 2010 at 19:47, <mailto:jochen@chromium.org> wrote:
> 
> > On 2010/11/25 17:59:41, Paweł Hajdan Jr. wrote:
> >
> >> Drive-by with an automation comment. No need to wait for another review by
> >> me.
> >>
> >
> >  By the way, it would be easier to write this as a browser test.
> >>
> >
> > Could you please explain way this would be easier?
> 
> 
> Sure. Instead of adding new automation messages and some boilerplate, you
> could access the needed data directly. This also means modifying less files,
> etc.

Ah, thanks

I'll update the CL tomorrow

[jochen (gone - plz use gerrit), 2010-11-26 10:37:29]

now with a browser test instead of an uitest

[Paweł Hajdan Jr., 2010-11-26 11:15:07]

Code I commented in the drive-by LGTM. Thanks!

http://codereview.chromium.org/5254005/diff/16001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/test/render_view_host_browsertest.cc (right):

http://codereview.chromium.org/5254005/diff/16001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/test/render_view_host_browsertest.cc:107:
ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
nit: Just NavigateToURL. It's the same as ...BlockUntilNavigationsComplete with
one navigation.

[darin (slow to review), 2010-11-30 18:28:56]

LGTM

http://codereview.chromium.org/5254005/diff/22001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5254005/diff/22001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:2132: bool is_unreachable_web_data,
nit: "is_error_page" seems like a better choice for this parameter name.  it
better matches what you wrote in the comment below.