Create withheld permissions

extensions/renderer/script_injection.cc

Index: extensions/renderer/script_injection.cc
diff --git a/extensions/renderer/script_injection.cc b/extensions/renderer/script_injection.cc
index 3c77f9af5ed88fb3c7794bb2f3ee4c4be1cf3ab8..a1ae1e27a478f2d1603a4b6c62cd1f0e754ee6de 100644
--- a/extensions/renderer/script_injection.cc
+++ b/extensions/renderer/script_injection.cc
@@ -40,8 +40,8 @@ const int kInvalidRequestId = -1;
 // The id of the next pending injection.
 int64 g_next_pending_id = 0;
 
-bool ShouldDelayForPermission() {
-  return FeatureSwitch::scripts_require_action()->IsEnabled();
+bool NotifyBrowserOfInjections() {

[not at google - send to devlin, 2014/06/27 23:24:34]

this should start with Should otherwise it sounds like it's actually notifying.

[Devlin, 2014/06/30 17:06:11]

Done.

+  return !FeatureSwitch::scripts_require_action()->IsEnabled();
 }
 
 // Append all the child frames of |parent_frame| to |frames_vector|.
@@ -145,15 +145,13 @@ bool ScriptInjection::TryToInject(UserScript::RunLocation current_location,
 
   switch (injector_->CanExecuteOnFrame(
       extension, web_frame_, tab_id_, web_frame_->top()->document().url())) {
-    case ScriptInjector::DENY_ACCESS:
+    case PermissionsData::DENY_ACCESS:
       NotifyWillNotInject(ScriptInjector::NOT_ALLOWED);
       return true;  // We're done.
-    case ScriptInjector::REQUEST_ACCESS:
+    case PermissionsData::REQUEST_ACCESS:
       RequestPermission();
-      if (ShouldDelayForPermission())
-        return false;  // Wait around for permission.
-      // else fall through
-    case ScriptInjector::ALLOW_ACCESS:
+      return false;  // Wait around for permission.
+    case PermissionsData::ALLOW_ACCESS:

[not at google - send to devlin, 2014/06/27 23:24:34]

I think it's fine if this class just interprets what it means for a permission
to be withheld or whatever, as in,

case PermissionsData::kAccessAllowed:
  ...
case PermissionsData::kAccessDenied:
  ...
case PermissionsData::kAccessWithheld:
  ...

looks good.

[Devlin, 2014/06/30 17:06:11]

Done.

       Inject(extension, scripts_run_info);
       return true;  // We're done!
   }
@@ -178,13 +176,14 @@ void ScriptInjection::RequestPermission() {
   content::RenderView* render_view =
       content::RenderView::FromWebView(web_frame()->top()->view());
 
-  // If the feature to delay for permission isn't enabled, then just send an
+  // If we are just notifying the browser of the injection, then send an
   // invalid request (which is treated like a notification).
-  request_id_ =
-      ShouldDelayForPermission() ? g_next_pending_id++ : kInvalidRequestId;
+  request_id_ = NotifyBrowserOfInjections() ? kInvalidRequestId
+                                            : g_next_pending_id++;
   render_view->Send(new ExtensionHostMsg_RequestScriptInjectionPermission(
       render_view->GetRoutingID(),
       extension_id_,
+      injector_->script_type(),

[not at google - send to devlin, 2014/06/27 23:24:34]

I think that exposing the "script type" isn't quite the right idiom. this is
eventually just "the type of permission check to make" which amounts to either
CanAccessPage vs CanRunContentScriptOnPage... so I think that's what it should
be. have an enum like PermissionData::AccessCheckType with values "can access
page" vs "can run content script on page. remove that script type from
extension_misc.

[Devlin, 2014/06/30 17:06:11]

My thought was that it's not unlikely (and perhaps even likely) that we'll want
to UMA this stuff at some point, in which case ScriptType will be pretty
useful... Are you sure you want it gone?

[not at google - send to devlin, 2014/07/01 00:28:35]

maybe. but we don't need it now, and like I said it's the wrong idiom for the
check.

[not at google - send to devlin, 2014/07/01 02:56:50]

I should explain more.

My "wrong idiom" comment is because it would be repeating the logic of what to
do given the script type. The renderer has already chosen - either check hosts
or scripts - so why does the browser also need to decide? I think it's better if
the browser's job is to simply call the right method, not decide.

Of course, right now all it's really just a rename. However, when the
declarative content scripts come into play it won't be. We would need to add
another script type.

If the enum was somewhere better than constants.h I'd object less. Can you think
of anything? It's not obvious, because these script injection things are an API
concern, whereas permissions checks are a platform concern.

Your point about UMA is good though. I'd definitely be ok with it if not for it
being in constants.h.

[Devlin, 2014/07/01 16:27:05]

Yeah, constants.h is kind of a "I give up" location.

What would actually make sense is a specific "script_constants" file, because we
have items that relate to multiple scripts that we kinda just throw anywhere. 
Case in point: UserScript::RunLocation.  It applies to UserScripts and
tabs.executeScript(), but we chucked it in UserScript.  We could grab that and
the script type and make a new constants file for them (so that it's at least
more clear where it's coming from, and, of course, namespace it to be clear what
file it's in - script_constants::RunLocation, script_constants::ScriptType).

This has two added advantages:
- Not everything has to include user_script.h only for its enums (though we
still replace it with a [smaller] constants file).
- We can make the world a better place by renaming it from RunLocation.
(If we go this route, I'd wanna move/rename RunLocation in a separate patch, but
it'd be an immediate follow-up.)

Or we could just throw it in UserScript because it's easy.

[not at google - send to devlin, 2014/07/01 17:02:11]

ok, let's throw it in UserScript.

then maybe pull it out into its own file later.

[Devlin, 2014/07/01 18:34:08]

Done.

       render_view->GetPageId(),
       request_id_));
 }
@@ -201,6 +200,9 @@ void ScriptInjection::Inject(const Extension* extension,
   DCHECK(scripts_run_info);
   DCHECK(!complete_);
 
+  if (NotifyBrowserOfInjections())
+    RequestPermission();
+
   std::vector<blink::WebFrame*> frame_vector;
   frame_vector.push_back(web_frame_);
   if (injector_->ShouldExecuteInChildFrames())
@@ -230,7 +232,7 @@ void ScriptInjection::Inject(const Extension* extension,
     // surface a request for a child frame.
     // TODO(rdevlin.cronin): We should ask for permission somehow.
     if (injector_->CanExecuteOnFrame(extension, frame, tab_id_, top_url) ==
-        ScriptInjector::DENY_ACCESS) {
+        PermissionsData::DENY_ACCESS) {
       DCHECK(frame->parent());
       continue;
     }