Top level navigations are not sent to the host browser in the ChromeFrame NPA...

Top level navigations are not sent to the host browser in the ChromeFrame NPAPI implementation.
These include window.open requests, hrefs with target _blank, etc.

The ChromeFrame NPAPI plugin needs to set the renderer pref which indicates that the host browser
can handle top level requests. 

We also pass an additional flag in the CreateExternal tab IPC which indicates whether the host
browser supports full tab mode browsing or not. This defaults to true for IE, as the activex 
implementation relies on being able to launch a new window with the attach external tab prefix.
which assumes support for full tab mode for IE being available. If the host browser does not
support full tab browsing the window.open request which comes into the ExternalTabContainer
instance creates a dummy ExternalTabContainer instance which is destroyed when the top level
navigation info is sent back to the host browser.

This CL is a short term fix for the issues with the NPAPI ChromeFrame plugin. Will do some
investigation into a better approach.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=57319

Test=Covered by existing ChromeFrame widget mode tests WidgetModeIE_SrcProperty and
WidgetModeFF_SrcProperty

Fixed incorrect assumptions in the test files used by these tests.

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

[Jeff Timanus, 2010-09-29 21:01:31]

From a cursory pass, LGTM.

Thanks for making this change, Ananta.  You certainly threw it together much
faster than I would have!

Jeff

[amit, 2010-09-29 21:49:28]

Better approach for this is to route all the webkit navigations due to 'link
clicks with non-self targets' or 'window.open in a different tab' get out via
RenderView::OpenURL code path. 

I'd like to get a sense of difficulty involved in doing this the right way
before giving in to workarounds :)

http://codereview.chromium.org/3549006/diff/17001/18002
File chrome/browser/external_tab_container_win.cc (right):

http://codereview.chromium.org/3549006/diff/17001/18002#newcode65
chrome/browser/external_tab_container_win.cc:65: supports_full_tab_mode_(false),
In general our design goal has been to shield chrome side of tab from
unnecessary details of chrome frame. So instead of 'is_widget_mode' etc we have
preferred fine grained features like "infobars_enabled". 
So please consider "route_all_top_level_navigations" or something better in
place of "supports_full_tab_mode_"

http://codereview.chromium.org/3549006/diff/17001/18002#newcode314
chrome/browser/external_tab_container_win.cc:314: if (dummy_tab_for_navigation_
&& disposition == CURRENT_TAB) {
what happens if we get anything other than CURRENT_TAB in case of dummy tab?
When does the tab go away?

http://codereview.chromium.org/3549006/diff/17001/18002#newcode385
chrome/browser/external_tab_container_win.cc:385: automation_,
automation_resource_message_filter_.get());
This could cause side effects. Any IPCs (say UpdateNavigationState due to
intermediate navigations) sent from the new tab will be treated as if they are
coming from original tab. No IPCs should be sent over from this tab on the wire
except the final navigation and maybe cookie IPCs?

[ananta, 2010-09-29 23:03:01]

While hooking this in WebKit so that we can basically translate the window.open
request into an open URL with the right target seems like the right approach, it
does not seem to be that straightforward from my debugging.

The window.open requests going through V8 eventually comes into FrameLoader
where it calls into the client to create a window(RenderView). If you return
NULL here it is treated as a popup blocker failed the call etc. Eventually it
calls into the new view to create a window followed by a call to show and then a
policy check to open the URL.

Will discuss this with the relevant webkit folks to see if we can do this a
better way.

http://codereview.chromium.org/3549006/diff/17001/18002
File chrome/browser/external_tab_container_win.cc (right):

http://codereview.chromium.org/3549006/diff/17001/18002#newcode65
chrome/browser/external_tab_container_win.cc:65: supports_full_tab_mode_(false),
On 2010/09/29 21:49:29, amit wrote:
> In general our design goal has been to shield chrome side of tab from
> unnecessary details of chrome frame. So instead of 'is_widget_mode' etc we
have
> preferred fine grained features like "infobars_enabled". 
> So please consider "route_all_top_level_navigations" or something better in
> place of "supports_full_tab_mode_"

Done.

http://codereview.chromium.org/3549006/diff/17001/18002#newcode314
chrome/browser/external_tab_container_win.cc:314: if (dummy_tab_for_navigation_
&& disposition == CURRENT_TAB) {
On 2010/09/29 21:49:29, amit wrote:
> what happens if we get anything other than CURRENT_TAB in case of dummy tab?
> When does the tab go away?

The other dispositions are handled correctly on the other side. The tab goes
away on the call to DestroyWindow below.

http://codereview.chromium.org/3549006/diff/17001/18002#newcode385
chrome/browser/external_tab_container_win.cc:385: automation_,
automation_resource_message_filter_.get());
On 2010/09/29 21:49:29, amit wrote:
> This could cause side effects. Any IPCs (say UpdateNavigationState due to
> intermediate navigations) sent from the new tab will be treated as if they are
> coming from original tab. No IPCs should be sent over from this tab on the
wire
> except the final navigation and maybe cookie IPCs?

Done. Added checks in the other places which send ipcs and DCHECKs where they
make sense. Cookie IPCs don't need to be sent as the page will never be rendered
in ChromeFrame if it is a dummy tab.

[amit, 2010-09-30 19:09:12]

I have a feeling that I am going to regret this but lgtm :)

Consider isolating this work around in a separate class like
TemporaryPopupExternalTab that derives from ExternalTabContainer. If you want,
it can be addressed in a separate CL based on the urgency.

http://codereview.chromium.org/3549006/diff/32001/23
File chrome/browser/external_tab_container_win.cc (right):

http://codereview.chromium.org/3549006/diff/32001/23#newcode450
chrome/browser/external_tab_container_win.cc:450: if (!automation_ ||
dummy_tab_for_navigation_)
This is  not good :)
Ideally, we need a subclass of ExternalTabcontainer and filter stuff there.

[ananta, 2010-09-30 20:05:45]

http://codereview.chromium.org/3549006/diff/32001/23
File chrome/browser/external_tab_container_win.cc (right):

http://codereview.chromium.org/3549006/diff/32001/23#newcode450
chrome/browser/external_tab_container_win.cc:450: if (!automation_ ||
dummy_tab_for_navigation_)
On 2010/09/30 19:09:12, amit wrote:
> This is  not good :)
> Ideally, we need a subclass of ExternalTabcontainer and filter stuff there. 

Done.