Add support for gcf:about:plugins in chrome frame full tab mode. The URL vali...

Add support for gcf:about:plugins in chrome frame full tab mode. The URL validation code path
in ChromeFrame now takes in an interface NavigationConstraints which allows the delegateslike
the ActiveX, ActiveDocument and the NPAPI plugins to control the navigation decisions.

We no longer refer to the InternetSecurityManager interface which is IE only for performing
zone decisions in the ChromeFrame NPAPI plugin.

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

BUG=66118
TEST=Covered by additional CanNavigate unit tests.


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

[amit, 2010-12-10 23:56:17]

very nice...

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_active_docu...
File chrome_frame/chrome_active_document.h (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_active_docu...
chrome_frame/chrome_active_document.h:212: public IMonikerProp {
Please get rid of unrelated changes from this CL.

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_activ...
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_activ...
chrome_frame/chrome_frame_activex_base.h:576: virtual bool IsSchemeAllowed(const
GURL&url) {
nit: space after &

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_deleg...
File chrome_frame/chrome_frame_delegate.h (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_deleg...
chrome_frame/chrome_frame_delegate.h:219: virtual bool IsSchemeAllowed(const
GURL&url) = 0;
nit: spacing after &

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_deleg...
chrome_frame/chrome_frame_delegate.h:224: class NavigationConstraintsImpl :
public NavigationConstraints {
why not move this into its own source file and include IInternetSecurityManager
in the implementation?

http://codereview.chromium.org/5698005/diff/1/chrome_frame/test/util_unittest...
File chrome_frame/test/util_unittests.cc (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/test/util_unittest...
chrome_frame/test/util_unittests.cc:294: SetConfigBool(kAllowUnsafeURLs, true);
not needed anymore! please get rid of other instances as well.

http://codereview.chromium.org/5698005/diff/1/chrome_frame/test/util_unittest...
chrome_frame/test/util_unittests.cc:305: TEST(UtilTests, CanNavigateTestDenyAll)
{
nice new tests

http://codereview.chromium.org/5698005/diff/1/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/utils.cc#newcode1437
chrome_frame/utils.cc:1437: if (!navigation_constraints)
We must have navigation_constraints, so crash if we don't?

[ananta, 2010-12-11 02:11:11]

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_active_docu...
File chrome_frame/chrome_active_document.h (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_active_docu...
chrome_frame/chrome_active_document.h:212: public IMonikerProp {
On 2010/12/10 23:56:17, amit wrote:
> Please get rid of unrelated changes from this CL.

Done.

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_activ...
File chrome_frame/chrome_frame_activex_base.h (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_activ...
chrome_frame/chrome_frame_activex_base.h:576: virtual bool IsSchemeAllowed(const
GURL&url) {
On 2010/12/10 23:56:17, amit wrote:
> nit: space after &

Done.

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_deleg...
File chrome_frame/chrome_frame_delegate.h (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_deleg...
chrome_frame/chrome_frame_delegate.h:219: virtual bool IsSchemeAllowed(const
GURL&url) = 0;
On 2010/12/10 23:56:17, amit wrote:
> nit: spacing after &

Done and moved these classes to the navigation_constraints.h/.cc files

http://codereview.chromium.org/5698005/diff/1/chrome_frame/chrome_frame_deleg...
chrome_frame/chrome_frame_delegate.h:224: class NavigationConstraintsImpl :
public NavigationConstraints {
On 2010/12/10 23:56:17, amit wrote:
> why not move this into its own source file and include
IInternetSecurityManager
> in the implementation? 

I thought about that. Decided against that for the timebeing as the only clients
of the IInternetSecurityManager interface are the activex and active document
combination.

http://codereview.chromium.org/5698005/diff/1/chrome_frame/test/util_unittest...
File chrome_frame/test/util_unittests.cc (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/test/util_unittest...
chrome_frame/test/util_unittests.cc:294: SetConfigBool(kAllowUnsafeURLs, true);
On 2010/12/10 23:56:17, amit wrote:
> not needed anymore! please get rid of other instances as well.

Done.

http://codereview.chromium.org/5698005/diff/1/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/5698005/diff/1/chrome_frame/utils.cc#newcode1437
chrome_frame/utils.cc:1437: if (!navigation_constraints)
On 2010/12/10 23:56:17, amit wrote:
> We must have navigation_constraints, so crash if we don't?
Replaced with a NOTREACHED and return false.

[amit, 2010-12-13 23:45:27]

lgtm

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
File chrome_frame/test/util_unittests.cc (right):

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:246: MOCK_METHOD1(IsSchemeAllowed,
bool(const GURL&url));
nit: spacing around &

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:258: url_prefix, false))
nit: fits on the line above

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:335: char *urls[] = {
would it make sense to use the same 'test_cases' list from the above in these
tests as well?

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:377: TEST_F(UtilTests,
CanNavigateTestAllowAllUnsafeUrs) {
nit:CanNavigateTestAllowAllUnsafeUr'l's

[ananta, 2010-12-14 00:36:27]

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
File chrome_frame/test/util_unittests.cc (right):

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:246: MOCK_METHOD1(IsSchemeAllowed,
bool(const GURL&url));
On 2010/12/13 23:45:27, amit wrote:
> nit: spacing around &

Done.

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:258: url_prefix, false))
On 2010/12/13 23:45:27, amit wrote:
> nit: fits on the line above

Done.

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:335: char *urls[] = {
On 2010/12/13 23:45:27, amit wrote:
> would it make sense to use the same 'test_cases' list from the above in these
> tests as well?

Done.

http://codereview.chromium.org/5698005/diff/44003/chrome_frame/test/util_unit...
chrome_frame/test/util_unittests.cc:377: TEST_F(UtilTests,
CanNavigateTestAllowAllUnsafeUrs) {
On 2010/12/13 23:45:27, amit wrote:
> nit:CanNavigateTestAllowAllUnsafeUr'l's

Done.