Prevent bindings escalation on an existing NavigationEntry (attempt 2).

Prevent bindings escalation on an existing NavigationEntry (attempt 2).

BUG=173672
TEST=See bug for repro steps.

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

[nasko, 2013-02-13 23:44:02]

lgtm with one simplifying change.

https://chromiumcodereview.appspot.com/12210048/diff/1/content/browser/web_co...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/12210048/diff/1/content/browser/web_co...
content/browser/web_contents/render_view_host_manager.cc:110:
!entry.GetContentState().empty() &&
This seems redundant, since we already check the bindings on the entry. If an
entry is existing, its bindings shouldn't be kInvalidBindings.

[Charlie Reis, 2013-02-13 23:56:18]

Thanks!

Darin, can you take a look for chrome OWNER approval?  This is the same CL as
https://chromiumcodereview.appspot.com/12086109, except I've changed the check
in RenderViewHostManager to handle the case that we restore a NavigationEntry
with no bindings (http://crbug.com/174306).  I've added a test for session
restore as well.

Still plan to follow up with something that stores the bindings in session
history, but I want to land this part first.

https://chromiumcodereview.appspot.com/12210048/diff/1/content/browser/web_co...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/12210048/diff/1/content/browser/web_co...
content/browser/web_contents/render_view_host_manager.cc:110:
!entry.GetContentState().empty() &&
On 2013/02/13 23:44:02, nasko wrote:
> This seems redundant, since we already check the bindings on the entry. If an
> entry is existing, its bindings shouldn't be kInvalidBindings.

Good point!  Updated.

[darin (slow to review), 2013-02-14 00:39:01]

LGTM

[Charlie Reis, 2013-02-14 18:21:32]

Minor change in the latest patch set: I changed the RestoreWebUI test from using
chrome://settings to chrome://newtab, since restoring a page with iframes was
turning out to be flaky (independent of this patch).  I've filed
http://crbug.com/176304 for that issue and I'll proceed with landing this.

[commit-bot: I haz the power, 2013-02-14 18:29:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/12210048/7003

[darin (slow to review), 2013-02-14 18:35:08]

OK

On Thu, Feb 14, 2013 at 10:21 AM, <creis@chromium.org> wrote:

> Minor change in the latest patch set: I changed the RestoreWebUI test from
> using
> chrome://settings to chrome://newtab, since restoring a page with iframes
> was
> turning out to be flaky (independent of this patch).  I've filed
> http://crbug.com/176304 for that issue and I'll proceed with landing this.
>
>
https://chromiumcodereview.**appspot.com/12210048/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2013-02-14 22:15:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/12210048/7003