New Task Manager - Phase 1.3.5: Implement Extension Task Providing

chrome/browser/task_management/web_contents_tags.cc

Index: chrome/browser/task_management/web_contents_tags.cc
diff --git a/chrome/browser/task_management/web_contents_tags.cc b/chrome/browser/task_management/web_contents_tags.cc
index bb4d1fec880f8b7acd31d27aa6ed8d02b2b168fd..f5f603437048b8824d061ba3481d8d9c1485ffb2 100644
--- a/chrome/browser/task_management/web_contents_tags.cc
+++ b/chrome/browser/task_management/web_contents_tags.cc
@@ -6,12 +6,19 @@
 
 #include "chrome/browser/task_management/providers/web_contents/background_contents_tag.h"
 #include "chrome/browser/task_management/providers/web_contents/devtools_tag.h"
+#include "chrome/browser/task_management/providers/web_contents/extension_tag.h"
 #include "chrome/browser/task_management/providers/web_contents/panel_tag.h"
 #include "chrome/browser/task_management/providers/web_contents/prerender_tag.h"
 #include "chrome/browser/task_management/providers/web_contents/printing_tag.h"
 #include "chrome/browser/task_management/providers/web_contents/tab_contents_tag.h"
 #include "chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h"
+#include "components/guest_view/browser/guest_view_base.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/view_type_utils.h"
+#include "extensions/common/constants.h"
 
 #if defined(ENABLE_EXTENSIONS)
 #include "chrome/browser/ui/panels/panel.h"
@@ -35,6 +42,33 @@ void TagWebContents(content::WebContents* contents,
   WebContentsTagsManager::GetInstance()->AddTag(tag);
 }
 
+bool IsExtensionWebContents(content::WebContents* contents) {
+  DCHECK(contents);
+
+  if (guest_view::GuestViewBase::IsGuest(contents))
+    return false;
+
+  extensions::ViewType view_type = extensions::GetViewType(contents);
+  return (view_type != extensions::VIEW_TYPE_INVALID &&
+          view_type != extensions::VIEW_TYPE_TAB_CONTENTS &&
+          view_type != extensions::VIEW_TYPE_BACKGROUND_CONTENTS &&
+          view_type != extensions::VIEW_TYPE_PANEL);
+}
+
+const extensions::Extension* GetExtensionFromWebContents(
+    content::WebContents* contents) {
+  DCHECK(contents);
+
+  const GURL& site =
+      contents->GetRenderViewHost()->GetSiteInstance()->GetSiteURL();

[ncarter (slow), 2015/08/03 21:12:18]

question for devlin -- how well-established is the pattern of extracting the
extension ID from the siteinstance url?

I'm somewhat worried about the fragility of this lookup. It relies on the caller
having created their initial SiteInstance via SiteInstance::CreateForURL with a
chrome-extension extension URL. Generally speaking (that is, for the drive-by
web), GetSiteURL can return surprising values that have nothing to do with the
current page. The way that extensions use it is a special case where it's
typically correct, but only because of fairly strong assumptions about the way
that extension processes are isolated from other types of processes.

So it's probably best to let the existing function do the lifting here:

extensions::ProcessManager::Get(web_contents->GetBrowserContext())->GetExtensionForWebContents(web_contents)

Or, even better, we can just pass the extension_id in. It is in easy reach for
all callers of SetViewType that we care about. Doing that likely means plumbing
a new function that's not SetViewType(), but that's called in the same places as
SetViewType() is today (and not called in the background contents/panel cases).

[afakhry, 2015/08/03 23:36:45]

This is the exact way followed in this code here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
And also by the method you pointed out above:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
But you're right, I think we might be better off by using an existing method
like the one you pointed out. I'll leave this question to devlin.

rdevlin.cronin@chromium.org: Could you please take a look at this part and
Nick's comment?

[Devlin, 2015/08/04 16:05:07]

For better or worse, we get the extension id from the site instance rather a
lot.  I'm not keen on passing in the extension id, because it doesn't make sense
for all view types (e.g., tab), and the idea of having yet another semi-mystical
function to call when creating an extension web contents is less attractive to
me.  I don't mind this approach, modulo using the existing functionality in
ProcessManager (if we need to change from using site instance some day, may as
well try and confine the usages as much as possible).

[afakhry, 2015/08/04 18:31:07]

Done.

+
+  DCHECK(site.SchemeIs(extensions::kExtensionScheme));
+
+  const std::string extension_id = site.host();
+  return extensions::ExtensionRegistry::Get(contents->GetBrowserContext())->
+      GetExtensionById(extension_id, extensions::ExtensionRegistry::ENABLED);
+}
+
 }  // namespace
 #endif  // defined(ENABLE_TASK_MANAGER)
 
@@ -114,6 +148,21 @@ void WebContentsTags::CreateForPrintingContents(
 #endif  // defined(ENABLE_TASK_MANAGER) && defined(ENABLE_PRINT_PREVIEW)
 }
 
+void WebContentsTags::CreateForExtension(content::WebContents* web_contents) {
+#if defined(ENABLE_TASK_MANAGER) && defined(ENABLE_EXTENSIONS)
+  DCHECK(IsExtensionWebContents(web_contents));
+
+  const extensions::Extension* extension =
+      GetExtensionFromWebContents(web_contents);
+
+  if (!WebContentsTag::FromWebContents(web_contents)) {
+    TagWebContents(web_contents,
+                   new ExtensionTag(web_contents, extension),
+                   WebContentsTag::kTagKey);
+  }
+#endif  // defined(ENABLE_TASK_MANAGER) && defined(ENABLE_EXTENSIONS)
+}
+
 // static
 void WebContentsTags::ClearTag(content::WebContents* web_contents) {
 #if defined(ENABLE_TASK_MANAGER)