DevTools: use per-LocalFrame instrumenting agents instead of per-Page ones.

DevTools: use per-LocalFrame instrumenting agents instead of per-Page ones.
This change also migrates all the agents to the new inspector's InspectorPageAgent::mainFrame() notion which is now localFrameRoot().

BUG=451004
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189412

[pfeldman, 2015-02-02 21:19:08]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org, kozyatinskiy@chromium.org, yurys@chromium.org

[yurys, 2015-02-03 06:44:01]

https://codereview.chromium.org/898593002/diff/1/Source/bindings/core/v8/Page...
File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/bindings/core/v8/Page...
Source/bindings/core/v8/PageScriptDebugServer.cpp:121: void
PageScriptDebugServer::addListener(ScriptDebugListener* listener, LocalFrame*
localFrame)
It'd be less ambiguous it were called localFrameRoot rather that plain
localFrame. WDYT? It would also make sense to add (frame ==
frame->localFrameRoot()) assertions in some of the places being changed.

https://codereview.chromium.org/898593002/diff/1/Source/core/frame/LocalFrame...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.cpp:224: visitor->trace(m_instrumentingAgents);
Should be guarded with the macro below.

https://codereview.chromium.org/898593002/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.cpp:430: return m_instrumentingAgents.get();
Why not  "return host()->instrumentingAgents();" ? Or better yet to move this
logic in instrumentingAgentsFor.

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMStorageAgent.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMStorageAgent.cpp:191: if (!m_frontend ||
!isEnabled() || !frame->isMainFrame())
How are we supposed to track dom storage events in child frame's processes?

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMStorageAgent.cpp:229: return
StorageNamespaceController::from(frame->page())->sessionStorage()->storageArea(frame->document()->securityOrigin());
Frame::page() is being deprecated, you shouldn't add new usages, can we simply
revert this?

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorInputAgent.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorInputAgent.cpp:86: *globalPoint =
frame->page()->chrome().rootViewToScreen(blink::IntRect(point, blink::IntSize(0,
0))).location();
ditto.

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorPageAgent.cpp:1237: if
(!mainFrame()->isMainFrame())
How can main frame be not not main frame?

https://codereview.chromium.org/898593002/diff/1/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/page/Page.cpp#ne...
Source/core/page/Page.cpp:101:
InspectorInstrumentation::networkStateChanged(frames[i].get(), online);
Don't we expect this event to be dispatched once per FrameHost?

https://codereview.chromium.org/898593002/diff/1/Source/core/storage/StorageA...
File Source/core/storage/StorageArea.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/storage/StorageA...
Source/core/storage/StorageArea.cpp:177:
InspectorInstrumentation::didDispatchDOMStorageEvent(localFrame, key, oldValue,
newValue, LocalStorage, securityOrigin);
Same question.

https://codereview.chromium.org/898593002/diff/1/Source/core/storage/StorageA...
Source/core/storage/StorageArea.cpp:210:
InspectorInstrumentation::didDispatchDOMStorageEvent(localFrame, key, oldValue,
newValue, SessionStorage, securityOrigin);
Ditto.

[pfeldman, 2015-02-03 07:09:16]

https://codereview.chromium.org/898593002/diff/1/Source/bindings/core/v8/Page...
File Source/bindings/core/v8/PageScriptDebugServer.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/bindings/core/v8/Page...
Source/bindings/core/v8/PageScriptDebugServer.cpp:121: void
PageScriptDebugServer::addListener(ScriptDebugListener* listener, LocalFrame*
localFrame)
On 2015/02/03 06:44:00, yurys wrote:
> It'd be less ambiguous it were called localFrameRoot rather that plain
> localFrame. WDYT? It would also make sense to add (frame ==
> frame->localFrameRoot()) assertions in some of the places being changed.

Done.

https://codereview.chromium.org/898593002/diff/1/Source/core/frame/LocalFrame...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.cpp:224: visitor->trace(m_instrumentingAgents);
I think instrumenting agents is already oilpan-allocated.

https://codereview.chromium.org/898593002/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.cpp:430: return m_instrumentingAgents.get();
That's because in a subsequent change, we'll have multiple local frames with the
same host having different instrumenting agents.

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDOMStorageAgent.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMStorageAgent.cpp:191: if (!m_frontend ||
!isEnabled() || !frame->isMainFrame())
I'll replace it with the check for being pageAgent's mainFrame. Other processes
will be inspected separately.

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDOMStorageAgent.cpp:229: return
StorageNamespaceController::from(frame->page())->sessionStorage()->storageArea(frame->document()->securityOrigin());
This part is particularly stupid. I'll follow up. There should be a direct way
of getting storage area.

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorInputAgent.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorInputAgent.cpp:86: *globalPoint =
frame->page()->chrome().rootViewToScreen(blink::IntRect(point, blink::IntSize(0,
0))).location();
The only way if getting into chrome is page for now. This will need to change
along with other oopif changes.

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorPageAgent.cpp:1237: if
(!mainFrame()->isMainFrame())
On 2015/02/03 06:44:00, yurys wrote:
> How can main frame be not not main frame?

That is the largest controversy of this change. mainFrame is now localFrameRoot,
not necessarily the main one. I can try renaming it into inspectedFrame.

https://codereview.chromium.org/898593002/diff/1/Source/core/page/Page.cpp
File Source/core/page/Page.cpp (right):

https://codereview.chromium.org/898593002/diff/1/Source/core/page/Page.cpp#ne...
Source/core/page/Page.cpp:101:
InspectorInstrumentation::networkStateChanged(frames[i].get(), online);
Present plan is to attach to connected localframe hierarchies, so one frame host
might have multiple inspectors attached.

[yurys, 2015-02-03 08:09:20]

lgtm

[pfeldman, 2015-02-03 08:10:55]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-02-03 08:12:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898593002/20001

[commit-bot: I haz the power, 2015-02-03 08:12:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 08:12:25]

Failed to apply patch for Source/core/frame/LocalFrame.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/frame/LocalFrame.cpp
  Hunk #1 FAILED at 51.
  Hunk #2 succeeded at 221 (offset 1 line).
  Hunk #3 succeeded at 425 (offset 1 line).
  Hunk #4 succeeded at 826 (offset 1 line).
  1 out of 4 hunks FAILED -- saving rejects to file
Source/core/frame/LocalFrame.cpp.rej

Patch:       Source/core/frame/LocalFrame.cpp
Index: Source/core/frame/LocalFrame.cpp
diff --git a/Source/core/frame/LocalFrame.cpp b/Source/core/frame/LocalFrame.cpp
index
59c68fb30f1656f1e87a4186e7e690d9dae676e4..fdf972a2b6c57e04725ee81a2f072a8fe6004658
100644
--- a/Source/core/frame/LocalFrame.cpp
+++ b/Source/core/frame/LocalFrame.cpp
@@ -51,6 +51,7 @@
 #include "core/html/HTMLPlugInElement.h"
 #include "core/inspector/ConsoleMessageStorage.h"
 #include "core/inspector/InspectorInstrumentation.h"
+#include "core/inspector/InstrumentingAgents.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/page/Chrome.h"
 #include "core/page/EventHandler.h"
@@ -220,6 +221,7 @@ LocalFrame::~LocalFrame()
 
 void LocalFrame::trace(Visitor* visitor)
 {
+    visitor->trace(m_instrumentingAgents);
 #if ENABLE(OILPAN)
     visitor->trace(m_destructionObservers);
     visitor->trace(m_loader);
@@ -423,6 +425,16 @@ LocalFrame* LocalFrame::localFrameRoot()
     return curFrame;
 }
 
+InstrumentingAgents* LocalFrame::instrumentingAgents()
+{
+    return m_instrumentingAgents.get();
+}
+
+void LocalFrame::setInstrumentingAgents(InstrumentingAgents*
instrumentingAgents)
+{
+    m_instrumentingAgents = instrumentingAgents;
+}
+
 bool LocalFrame::inScope(TreeScope* scope) const
 {
     ASSERT(scope);
@@ -814,6 +826,7 @@ inline LocalFrame::LocalFrame(FrameLoaderClient* client,
FrameHost* host, FrameO
     , m_pageZoomFactor(parentPageZoomFactor(this))
     , m_textZoomFactor(parentTextZoomFactor(this))
     , m_inViewSourceMode(false)
+    , m_instrumentingAgents(host->instrumentingAgents())
 {
 }

[pfeldman, 2015-02-03 11:00:53]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-02-03 11:02:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898593002/40001

[commit-bot: I haz the power, 2015-02-03 11:14:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 11:14:51]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/4...)

[dgozman, 2015-02-03 11:26:55]

mostly lgtm, but I'm not sure I understand the final direction.

Why is there |inspectedFrame| in some places, and |localFrameRoot| in others?

What's the future plan? I imagine one of the following three:
1. Always attached to localFrameRoot (there may be multiple such frames in a
single process for a single page).
2. Always attached to a single frame in the same process for any given page.
3. Attached to an arbitrary set of frames in the same process.

[pfeldman, 2015-02-03 11:29:43]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-02-03 11:30:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898593002/60001

[pfeldman, 2015-02-03 11:42:28]

On 2015/02/03 11:26:55, dgozman wrote:
> mostly lgtm, but I'm not sure I understand the final direction.
> 
> Why is there |inspectedFrame| in some places, and |localFrameRoot| in others?
> 
> What's the future plan? I imagine one of the following three:
> 1. Always attached to localFrameRoot (there may be multiple such frames in a
> single process for a single page).
> 2. Always attached to a single frame in the same process for any given page.
> 3. Attached to an arbitrary set of frames in the same process.

The target scenario is (1). In this case inspectedFrame->isLocalFrameRoot() is
always true.

[commit-bot: I haz the power, 2015-02-03 11:55:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-03 11:55:50]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/4...)

[pfeldman, 2015-02-03 12:31:37]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-02-03 12:32:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898593002/80001

[commit-bot: I haz the power, 2015-02-03 13:50:05]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189412

[dcheng, 2015-02-05 00:11:40]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/892693006/ by dcheng@chromium.org.

The reason for reverting is: Speculatively reverting to try to fix
telemetry_unittests failures in Blink roll:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...
DevtoolsTargetCrashException: Devtools target crashed.