Remove GetActiveEntry usage from content.

Remove GetActiveEntry usage from content.

This is a copy of https://chromiumcodereview.appspot.com/23022006, which was reverted due to perf regression. The root cause was WebContentsImpl::GetWebkitPrefs using GetLastCommittedEntry, which caused compositing to be disabled and higher memory usage on Mac. I've removed that part of the change and will investigate separately how to best fix it.

BUG=273710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227665

[nasko, 2013-10-04 16:58:37]

Hey Charlie,
I've found the root cause of the perf regression and fixed it. Can you review
this again for me?
Thanks!
Nasko

[Charlie Reis, 2013-10-04 18:42:46]

I've only looked at the GetWebKitPrefs case.  I assume the rest is the same?

https://codereview.chromium.org/25654005/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/25654005/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:3504: // We want to base the
page config off of the real URL, rather than the
This "real URL" vs "display URL" is a bit confusing, given the issues involved
below.  Can we change this to "actual URL" and "virtual URL," since it seems to
be referring to the fact that GetURL() returns the virtual URL?

https://codereview.chromium.org/25654005/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:3508: return
GetWebkitPrefs(GetRenderViewHost(), url);
I had a comment on the original review that this looked wrong, since
GetRenderViewHost (i.e., current) does not necessarily match GetVisibleEntry
(i.e., pending).

Can you explain a bit more about why using GetLastCommittedEntry leads to
compositing being disabled?  If the caller is expecting this to be the pending
URL, then GetVisibleEntry will be wrong as well (e.g., in cases that we decide
it's not safe to show the pending URL in the omnibox).

[nasko, 2013-10-08 16:27:27]

As we chatted offline, I've removed the GetWebkitPrefs() change, since it will
require some more investigation.

[Charlie Reis, 2013-10-08 20:23:28]

LGTM with the comment below.  Also please update the CL description (since we're
not using GetVisibleEntry there).

(I'm assuming the rest hasn't changed since the previous CL.  Is there anything
else I should take a closer look at?)

https://codereview.chromium.org/25654005/diff/11001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/25654005/diff/11001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:3505: // display URL.
I think this comment is still worth updating because of the confusion about real
vs display (see my previous comment).

Also, let's add a TODO to investigate how to replace GetActiveEntry here, since
it's definitely a problem but we don't know what the correct answer is yet.

[nasko, 2013-10-08 21:45:54]

Fixed comment, added TODO, rebased on ToT.

https://codereview.chromium.org/25654005/diff/11001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/25654005/diff/11001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:3505: // display URL.
On 2013/10/08 20:23:28, creis wrote:
> I think this comment is still worth updating because of the confusion about
real
> vs display (see my previous comment).
> 
> Also, let's add a TODO to investigate how to replace GetActiveEntry here,
since
> it's definitely a problem but we don't know what the correct answer is yet.

Done.

[Charlie Reis, 2013-10-08 21:53:57]

LGTM

[commit-bot: I haz the power, 2013-10-08 22:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/25654005/17001

[commit-bot: I haz the power, 2013-10-09 02:14:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/25654005/17001

[commit-bot: I haz the power, 2013-10-09 04:03:34]

Change committed as 227665