View source after POST command isn't what you expected.

View source after POST command isn't what you expected.

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

[pfeldman, 2010-12-13 19:50:19]

Hi guys.

This is a wip patch that I did not run tests against yet - just drafted it and
it seems to work fine. Would like to know what you think about it to shorten the
over-the-ocean turnaround.

@darin: I followed your advice in
http://code.google.com/p/chromium/issues/detail?id=523#c97. It seems to work
fine.
@sky: I needed to change the PruneAllButActive as we discussed offline.
@rsesek: I did not come across any of the issues you pointed out in 
 http://code.google.com/p/chromium/issues/detail?id=523#c99. Could you provide
more details?

Thanks
Pavel

[sky, 2010-12-13 20:14:38]

The navcontroller change LGTM. Be sure and add test coverage.

[darin (slow to review), 2010-12-13 21:45:37]

Looks good, but...

http://codereview.chromium.org/5716003/diff/2001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/5716003/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1545: active_entry->set_url(url);
I was not expecting this set_url call.  Is that necessary?

[pfeldman1, 2010-12-14 16:27:50]

> chrome/browser/ui/browser.cc:1545: active_entry->set_url(url);
> I was not expecting this set_url call.  Is that necessary?

Nuked. It did not work for me at first, but now it does. Added unit tests for
PruneAllButActive.

[yurys, 2010-12-14 16:37:06]

LGTM

http://codereview.chromium.org/5716003/diff/8001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/5716003/diff/8001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:479: void ViewSource(TabContentsWrapper* tab);
Can you make this method private?

[apavlov, 2010-12-14 16:44:44]

Only one comment for the issue.

http://codereview.chromium.org/5716003/diff/8001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/5716003/diff/8001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/navigation_controller.cc:995:
entries_.erase(entries_.begin(), entries_.begin() + transient_entry_index_);
transient_entry_index_ is always 0 (see line 992), so this line makes no sense
to me

[Robert Sesek, 2010-12-14 17:23:56]

On 2010/12/13 19:50:19, pfeldman wrote:
> @rsesek: I did not come across any of the issues you pointed out in 
>  http://code.google.com/p/chromium/issues/detail?id=523#c99. Could you provide
> more details?
 
Your use of TabContents::Clone() should take care of any issues with the
SiteInstance. The issue I had with the WebHistoryItem may be resolved with
PruneAllButActive() -- that was the piece that I didn't have. Other than that,
this is pretty similar to my WIP. LGTM.

[Paweł Hajdan Jr., 2010-12-15 08:26:22]

Drive-by with a test comment. Feel free to do in a separate CL. No need to wait
for another review by me.

http://codereview.chromium.org/5716003/diff/16001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/navigation_controller_unittest.cc (right):

http://codereview.chromium.org/5716003/diff/16001/chrome/browser/tab_contents...
chrome/browser/tab_contents/navigation_controller_unittest.cc:1954: /*
TODO(brettw) These test pass on my local machine but fail on the XP buildbot
Could you DISABLE the tests below instead of having them inside a commented-out
block? If they have already rotted, i.e. no longer compile, please ignore this
comment.

[pfeldman1, 2010-12-15 11:29:58]

http://codereview.chromium.org/5716003/diff/16001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/navigation_controller_unittest.cc (right):

http://codereview.chromium.org/5716003/diff/16001/chrome/browser/tab_contents...
chrome/browser/tab_contents/navigation_controller_unittest.cc:1954: /*
TODO(brettw) These test pass on my local machine but fail on the XP buildbot
On 2010/12/15 08:26:22, Paweł Hajdan Jr. wrote:
> Could you DISABLE the tests below instead of having them inside a
commented-out
> block? If they have already rotted, i.e. no longer compile, please ignore this
> comment.

I don't see how it relates to the change I am applying + have no reason not to
trust Brett's judgement.

[Paweł Hajdan Jr., 2010-12-15 11:52:58]

On Wed, Dec 15, 2010 at 12:29, <pfeldman@google.com> wrote:

> I don't see how it relates to the change I am applying + have no reason
> not to trust Brett's judgement.


Sorry, it was a "while you're here" type of drive-by comment. :) It was
generally optional.