Fix uses of NavigationEntries in RenderViewContextMenu.

Fix uses of NavigationEntries in RenderViewContextMenu.

Add null checks where needed and ensure that the right type of entry is used
for each use case.

BUG=370863, 273710
TEST=See bug, comment 3 for repro steps.

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

[Charlie Reis, 2014-05-09 22:48:50]

Nasko and John, can you take a look?

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left):

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1285: //
Instead of using GetURL here, we use url() (which is the "real" url of
John: It looks like you changed this behavior in http://crrev.com/115766 but
didn't update the comment.  I'm not sure if it has been buggy since that change,
but the code matches the comment now.

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1289: //
TODO(nasko): Audit all GetActiveEntry calls in this file.
Nasko: I think this takes care of the rest of them.

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1149: if
(source_web_contents_->GetController().GetVisibleEntry() == NULL)
This needed to match the VIEWPAGEINFO case below, so that we don't show "View
Page Info" when there's no visible entry (e.g., during a spoof attempt).

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1285: // We
save the last committed entry (which the user is looking at), as
I think it makes sense to save the page you're viewing rather than the one
that's pending, since you got to it by right clicking.

[nasko, 2014-05-09 22:56:07]

I have one question to ensure I understand the test, but overall LGTM.

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1285: // We
save the last committed entry (which the user is looking at), as
On 2014/05/09 22:48:50, Charlie Reis wrote:
> I think it makes sense to save the page you're viewing rather than the one
> that's pending, since you got to it by right clicking.

Agreed.

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:233:
browser()->tab_strip_model()->GetActiveWebContents(), "window.open();"));
Will the about:blank synchronous navigation issue a didCommitProvisionalLoad? If
yes, then I assume we aren't creating a navigation entry then?

[jam, 2014-05-09 23:06:53]

lgtm

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left):

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1285: //
Instead of using GetURL here, we use url() (which is the "real" url of
On 2014/05/09 22:48:50, Charlie Reis wrote:
> John: It looks like you changed this behavior in http://crrev.com/115766 but
> didn't update the comment.  I'm not sure if it has been buggy since that
change,
> but the code matches the comment now.

I just looked at that cl, GetURL() there returned url_, which is what url() used
to do before. so i think that old comment was wrong?

[Charlie Reis, 2014-05-09 23:34:42]

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left):

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1285: //
Instead of using GetURL here, we use url() (which is the "real" url of
On 2014/05/09 23:06:54, jam wrote:
> On 2014/05/09 22:48:50, Charlie Reis wrote:
> > John: It looks like you changed this behavior in http://crrev.com/115766 but
> > didn't update the comment.  I'm not sure if it has been buggy since that
> change,
> > but the code matches the comment now.
> 
> I just looked at that cl, GetURL() there returned url_, which is what url()
used
> to do before. so i think that old comment was wrong?

Huh.  That confused me as well, but now I think "GetURL()" in the old comment
referred to TabContents::GetURL(), which returned the virtual URL.

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/273163003/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:233:
browser()->tab_strip_model()->GetActiveWebContents(), "window.open();"));
On 2014/05/09 22:56:08, nasko wrote:
> Will the about:blank synchronous navigation issue a didCommitProvisionalLoad?
If
> yes, then I assume we aren't creating a navigation entry then?

Good question.  Looks like it does send a didCommitProvisionalLoad, but it does
so with a page_id of -1.  That makes us classify it as
NAVIGATION_TYPE_NAV_IGNORE in NavigationController, and thus we have no
committed entry afterward.

[Charlie Reis, 2014-05-10 00:02:02]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-05-10 00:05:31]

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

[commit-bot: I haz the power, 2014-05-10 02:06:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-10 03:20:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 03:20:15]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Charlie Reis, 2014-05-12 05:37:32]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-05-12 05:37:45]

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

[commit-bot: I haz the power, 2014-05-12 06:46:20]

Change committed as 269746