Force MimeHandlerView to always use BrowserPlugin

content/public/browser/guest_mode.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_PUBLIC_BROWSER_GUEST_MODE_H_
+#define CONTENT_PUBLIC_BROWSER_GUEST_MODE_H_
+
+#include "base/macros.h"
+#include "content/common/content_export.h"
+
+namespace content {
+
+class WebContents;
+
+namespace guest_mode {

[Charlie Reis, 2016/09/07 00:22:29]

Hmm, I'm not a fan of introducing a new namespace and file for this.  Why not
put it in BrowserPluginGuestMode instead?

[EhsanK, 2016/09/08 18:14:58]

That one lives in content/public/common while this is in content/public/browser.
Should I introduce a new class?
Meanwhile, I am investigating the removal of UseCrossProcessFramesForGuest().

[Charlie Reis, 2016/09/15 20:56:10]

Yes, let's at least use the same pattern, even if we can't put it in
content/public/common.

[EhsanK, 2016/09/21 21:34:53]

Sure. I am calling the class GuestMode now.

+
+// TODO(ekaramad): This method is temporarily added to provide a workaround for
+// BrowserPlugin-based MimeHandlerView to coexit with the rest of guests when

[Charlie Reis, 2016/09/07 00:22:29]

nit: coexist

[EhsanK, 2016/09/08 18:14:58]

Acknowledged.

+// they are OOPIF-based. This will be removed once https://crbug.com/642826 is
+// fixed.
+bool CONTENT_EXPORT IsCrossProcessFrameGuest(WebContents* web_contents);

[Charlie Reis, 2016/09/07 00:22:29]

I'm finding this confusing, both in name and how it relates to
UseCrossProcessFramesForGuests.

Is it ever safe to call UseCrossProcessFramesForGuests directly anymore?  Maybe
we should just be replacing that.

[EhsanK, 2016/09/08 18:14:58]

Hmm, this method is supposed to return true when |web_contents| is a guest and
is OOPIF. That is why I named it like this. I am however open to any better
suggestions for naming. I am also investigating removing the older method
BrowserPluginGuestMode::UseCrossProcessFramesForGuests().

[EhsanK, 2016/09/08 18:14:58]

As lfg@ was suggesting offline, on the renderer side there are two usages of
UserCrossProcessFramesForGuests().

But still, I will try to figure out if we can replace all the browser side
occurrences with the new method which verifies it for
an individual WebContents.

[Charlie Reis, 2016/09/15 20:56:10]

Yes, maybe we can have a renderer-side version for those two cases, and use this
for all the browser-side cases (with nothing left in content/public/common)?

[EhsanK, 2016/09/21 21:34:53]

1- GuestViewIframeContainer, which cannot have a non-OOPIF implementation.
2- Dispatcher::GetJsResources(), which is static
3- Dispatcher::RequireGuestViewModules() which AFAICT is only for guests. But
this is just two js bindings which are not necessary if the guest is not using
<iframes>.

For that reason, we might be better off with having
BrowserPluginGuestMode::UseCrossProcessFramesForGuests() for those applications.
As far as its existence in content/public/common goes, I think it is necessary
because we use it in browser test.

We could remove it, but instead, add the same method to both
content/public/renderer and content/public/browser. WDYT?

[Charlie Reis, 2016/09/21 22:18:21]

The property we're looking for is that these places in the renderer won't break
in the PDF case when UseCrossProcessFramesForGuests is on.  Is that true?  (I'm
not sure from your description.)
Let's just check the flag directly in the browser tests, since they're just
skipping the test if we're in that mode.  We shouldn't keep the method in
content/public/browser for that case.
Having it in content/public/browser would defeat the point.  It seems like any
code that calls UseCrossProcessFramesForGuests() will be wrong in the PDF case,
so I'd like to remove it completely from the browser process so no one tries to
call it (and be sure that the uses in the renderer are safe).

I noticed there's a call in SiteIsolationPolicy::AreCrossProcessFramesPossible,
but we can probably check the flag directly there.  (Technically that method
returns true on all desktop platforms today anyway, now that
--isolate-extensions is enabled.  I suppose it's still false on Android,
though.)

[EhsanK, 2016/09/22 22:55:43]

Thanks for clarification/suggestions. I removed the class from content/common. I
have now added another class to renderer/ which will check the flag. I could
instead check for the switch directly in those 3 usages. WDYT?

+
+}  // namespace guest_mode
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_GUEST_MODE_H_