Add frameId to executeScript/insertCSS

extensions/renderer/script_injection_manager.cc

Index: extensions/renderer/script_injection_manager.cc
diff --git a/extensions/renderer/script_injection_manager.cc b/extensions/renderer/script_injection_manager.cc
index 93862dd04bdfe1109bf23c223ccef4cb734ec90e..ca2f0178a18cf81610d0626295243202d7d5acc7 100644
--- a/extensions/renderer/script_injection_manager.cc
+++ b/extensions/renderer/script_injection_manager.cc
@@ -8,6 +8,7 @@
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
+#include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/render_view.h"
 #include "content/public/renderer/render_view_observer.h"
@@ -67,6 +68,29 @@ scoped_ptr<ExtensionInjectionHost> GetExtensionInjectionHost(
       new ExtensionInjectionHost(extension));
 }
 
+// TODO(robwu): ScriptInjectionManager should use RenderFrame instead of
+// RenderView. Then the browser should direct messages to specific frames and
+// this check becomes redundant.
+blink::WebLocalFrame* GetFrameByFrameId(blink::WebFrame* parent_frame,

[nasko, 2015/02/23 18:39:02]

Since this is a new function trying to work on the frame tree and using routing
ids, it makes more sense to use RenderFrame instead of WebFrame.

Also, have you considered exposing RenderFrameImpl::FromRoutingID on the public
interface and using that instead of traversing? It seems a bit more clean to me.

[robwu, 2015/02/23 18:59:53]

RenderFrames definitely make more sense, but currently IPC is routed via
RenderView, not RenderFrame. I plan on fixing the IPC in a separate patch, which
will make this function redundant.
Importing from non-public content is forbidden by the PRESUBMIT.

+                                        int frame_id) {
+  DCHECK(parent_frame);

[not at google - send to devlin, 2015/02/23 20:41:24]

This seems more like a CHECK situation to me.

[robwu, 2015/02/23 21:54:06]

I've only put the DCHECK over there to document that the precondition is that
the pointer exists.
In the way that the logic is used now, this DCHECK can never get triggered
(main_frame && GetFrameById(main_frame)).

+  blink::WebLocalFrame* result = nullptr;
+  for (blink::WebFrame* frame = parent_frame->firstChild(); !result && frame;

[not at google - send to devlin, 2015/02/23 20:41:24]

Pulling this comment out of the thread below: we already have incorrect
traversal logic somewhere. Is it possible to pull the two of these out into a
common utility, to contain the damage that introducing more assumption would
cause?

I'm thinking, to minimise structural changes, a FrameWalker class which you'd
use like:

FrameWalker frame_walker(parent_frame);
while (frame_walker.HasNext()) {
  content::RenderFrame* rf = content::RenderFrame::FromWebFrame(
      frame_walker.Current());
  if (rf && rf->GetRoutingID() == frame_id)
    return rf.Current();
}
return nullptr;

Then in the other place where we do this (I can't remember, sorry) use
FrameWalker. FrameWalker can then implement the logic for skipping over remote
frames.

[robwu, 2015/02/23 21:54:06]

This one: https://cs.chromium.org/AppendAllChildFrames?
"My" usage here allows stopping whenever the target frame has been found,
regardless of whether the frame is remote (on average, visiting 50% of the
frames gives the right (remote/local) frame back). The other one has to walk
over every frame and skip remote ones.

Considering that these two use cases are not identical, and "my" usage is only
temporary (until RenderView -> RenderFrame has been completed), do you still
want to merge them?

[not at google - send to devlin, 2015/02/23 22:04:15]

That's it. Its call site would be nicer with an iterator IMO.
They seem the same to me? On one hand, you iterate over all the frames. On the
other, you stop somewhere along the way.

I'm imagining that the code, eventually, will look like the browser sending the
IPC to topmost RenderFrame in each tree, iterating over all local children,
doing <whatever>, stopping at remote frames.
- In the content script injection case, <whatever> is injecting a script.
- In this case, <whatever> is finding the frame ID, and the browser should know
exactly what RenderFrame to send it to.

+       frame = frame->nextSibling()) {

[dcheng, 2015/02/23 16:16:01]

We definitely shouldn't be looping through all the WebFrames to try to find the
one with the matching routing ID.

We should probably fix ScriptInjectionManager so we can just route the IPC
correctly.

[not at google - send to devlin, 2015/02/23 16:50:17]

This is a project unto itself. I just filed crbug.com/460950 for this, you can
reference it here - it'd be good not to entrench that bug any further than it
is, if possible.

[dcheng, 2015/02/23 16:56:42]

I realize this is going to be a non-trivial amount of work, but taking this
route is just deferring even more work to later. Given that the out-of-process
iframe work is pretty far along, I think it's a mistake to keep writing code
like this.

[robwu, 2015/02/23 17:13:40]

Even after fixing the IPC, this feature will still not work with OOPI because of
crbug.com/432875.

Is the absence of a good integration with OOPI really a blocker for landing this
CL?
I'm not too happy about the need for traversing frames to identify the frameId,
but in practice this is not much different from the present state. Currently,
extension authors have to insert in every frame and then use JavaScript to
filter the result. With frameId, behind the scenes all frames will still be
traversed, but at least the code will be inserted in one frame only. The end
result is that extensions could become a bit more efficient (or at the very
least become less complex due to the ability to target specific frames).

[robwu, 2015/02/23 17:44:36]

I'd take the 32 least significant bits. This truncation seems ok, the odds of
having an ID collision is very low, especially within the same tab. Last time I
checked, these IDs were not easily accessible via RenderFrame(Host)s. I'll take
another look.
In my view, the fact that this feature request has been around for so long and
the number of stars means that it should get a higher priority than new bugs.
Most extensions do not need this feature, but those that do will benefit from
this functionality, even if it is implemented in a way that is not perfect.

content::RenderFrameImpl::FromID gives the desired answer in a cleaner way, but
access to that method was disallowed by the PRESUBMIT. I reckon that publicising
the functionality via content::RenderFrame is worse than traversing.

+    content::RenderFrame* rf = content::RenderFrame::FromWebFrame(frame);
+    if (rf && rf->GetRoutingID() == frame_id) {
+      // TODO(robwu): This does not work with OOPIF. That doesn't really matter
+      // for now, because ScriptInjectionManager is not OOPIF-friendly anyway.
+      // This helper function will be obsolete when this class is converted to
+      // RenderFrame.
+      result = frame->toWebLocalFrame();
+    } else {
+      result = GetFrameByFrameId(frame, frame_id);
+    }
+  }
+  return result;
+}
+
 }  // namespace
 
 class ScriptInjectionManager::RVOHelper : public content::RenderViewObserver {
@@ -418,16 +442,18 @@ void ScriptInjectionManager::InjectScripts(
 void ScriptInjectionManager::HandleExecuteCode(
     const ExtensionMsg_ExecuteCode_Params& params,
     content::RenderView* render_view) {
+  blink::WebLocalFrame* main_frame =
+      render_view->GetWebView()->mainFrame()->toWebLocalFrame();

[nasko, 2015/02/23 18:39:02]

This will definitely fail with --site-per-process. Can we write the code a bit
more OOPIF friendly? Why operate on WebLocalFrame instead of RenderFrame here?

[robwu, 2015/02/23 18:59:53]

This whole class does not work with OOPIF at the moment. Getting it to be OOPIF
friendly requires the IPC to be rewritten (https://crbug.com/460950).
Because everything else in this class uses WebFrame*. I guess that the cast to a
WebLocalFrame is here because the existing logic doesn't work with cross-process
frames.

+  if (main_frame && params.frame_id > 0)
+    main_frame = GetFrameByFrameId(main_frame, params.frame_id);
   // TODO(dcheng): Not sure how this can happen today. In an OOPI world, it
   // would indicate a logic error--the browser must direct this request to the
   // right renderer process to begin with.
-  blink::WebLocalFrame* main_frame =
-      render_view->GetWebView()->mainFrame()->toWebLocalFrame();
   if (!main_frame) {
     render_view->Send(
         new ExtensionHostMsg_ExecuteCodeFinished(render_view->GetRoutingID(),
                                                  params.request_id,
-                                                 "No main frame",
+                                                 "Frame not found.",
                                                  GURL(std::string()),
                                                  base::ListValue()));
     return;