[DevTools] Console Message Storage moved from top local frame to frame host

[DevTools] Console Message Storage moved from top local frame to frame host

Console Message Storage was moved to frame host because remote frame which used in site-per-process mode not contains Frame Console.
Instrumentation Code generator supports agent dispatching by different objects e.g. messagesCleared dispatch by execution context for workers and by FrameHost for pages.

R=vsevik@chromium.org
BUG=421948
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183735

[kozyatinskiy1, 2014-10-14 14:00:42]

Vsevolod, please take a look!

[vsevik, 2014-10-14 14:34:08]

lgtm

https://codereview.chromium.org/645693003/diff/20001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/645693003/diff/20001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:314:
InspectorInstrumentation::frameWindowDiscarded(this, m_domWindow.get());
I think this should not be called when host() is null as well.
However we'd better change this in a separate patch as this is kind of scary.

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/Co...
File Source/core/inspector/CodeGeneratorInstrumentation.py (right):

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/Co...
Source/core/inspector/CodeGeneratorInstrumentation.py:339: generated =
template_outofline.substitute(
Please give it a bit more descriptive name

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorInstrumentation.idl:487: void
addMessageToConsole(FrameHost* frameHost, ConsoleMessage* consoleMessage);
This one should be before the worker one.

[kozyatinskiy1, 2014-10-14 17:17:01]

https://codereview.chromium.org/645693003/diff/20001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/645693003/diff/20001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:314:
InspectorInstrumentation::frameWindowDiscarded(this, m_domWindow.get());
On 2014/10/14 14:34:08, vsevik wrote:
> I think this should not be called when host() is null as well.
> However we'd better change this in a separate patch as this is kind of scary.

Acknowledged.

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/Co...
File Source/core/inspector/CodeGeneratorInstrumentation.py (right):

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/Co...
Source/core/inspector/CodeGeneratorInstrumentation.py:339: generated =
template_outofline.substitute(
On 2014/10/14 14:34:08, vsevik wrote:
> Please give it a bit more descriptive name

Done.

[kozyatinskiy1, 2014-10-14 17:17:47]

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://codereview.chromium.org/645693003/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorInstrumentation.idl:487: void
addMessageToConsole(FrameHost* frameHost, ConsoleMessage* consoleMessage);
On 2014/10/14 14:34:08, vsevik wrote:
> This one should be before the worker one.

Done.

[kozyatinskiy1, 2014-10-14 17:18:00]

The CQ bit was checked by kozyatinskiy@google.com

[commit-bot: I haz the power, 2014-10-14 17:18:45]

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

[commit-bot: I haz the power, 2014-10-14 17:29:13]

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

[commit-bot: I haz the power, 2014-10-14 17:29:18]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)

[vsevik, 2014-10-15 08:06:22]

https://codereview.chromium.org/645693003/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/645693003/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:312: if (host())
we should move inspector instrumentation call under this if as well.

[kozyatinskiy1, 2014-10-15 08:38:03]

https://codereview.chromium.org/645693003/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/645693003/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:312: if (host())
On 2014/10/15 08:06:22, vsevik wrote:
> we should move inspector instrumentation call under this if as well.

Done.

[kozyatinskiy1, 2014-10-15 08:38:12]

The CQ bit was checked by kozyatinskiy@google.com

[commit-bot: I haz the power, 2014-10-15 08:38:41]

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

[commit-bot: I haz the power, 2014-10-15 10:59:19]

Committed patchset #4 (id:60001) as 183735

[Charlie Reis, 2014-10-15 17:09:17]

creis@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org

[Charlie Reis, 2014-10-15 17:09:18]

[+dcheng for FYI]

Thanks for taking care of this so quickly!  A few post-commit nits, which
perhaps Daniel can elaborate on.

https://codereview.chromium.org/645693003/diff/60001/Source/core/frame/FrameC...
File Source/core/frame/FrameConsole.cpp (right):

https://codereview.chromium.org/645693003/diff/60001/Source/core/frame/FrameC...
Source/core/frame/FrameConsole.cpp:169: return
&m_frame->page()->frameHost().consoleMessageStorage();
nit: Can we use m_frame->host() instead of m_frame->page()->frameHost()?  Page
is eventually going to go away.

https://codereview.chromium.org/645693003/diff/60001/Source/core/inspector/In...
File Source/core/inspector/InspectorInstrumentation.cpp (right):

https://codereview.chromium.org/645693003/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorInstrumentation.cpp:234: return
instrumentationForPage(&host->page());
This might be ok for now, but there's a comment on FrameHost::page() warning
that it will eventually be removed (since Page is going away).

[Charlie Reis, 2014-10-15 17:17:41]

https://codereview.chromium.org/645693003/diff/60001/Source/core/inspector/In...
File Source/core/inspector/InspectorInstrumentation.cpp (right):

https://codereview.chromium.org/645693003/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorInstrumentation.cpp:234: return
instrumentationForPage(&host->page());
On 2014/10/15 17:09:17, Charlie Reis wrote:
> This might be ok for now, but there's a comment on FrameHost::page() warning
> that it will eventually be removed (since Page is going away).

(And to be clear, nothing needs to change for this at the moment-- it's just a
heads up for what will eventually happen.  Lots of things from Page will move
elsewhere.)

[vsevik, 2014-10-15 20:23:41]

>
https://codereview.chromium.org/645693003/diff/60001/Source/core/frame/FrameC...
> Source/core/frame/FrameConsole.cpp:169: return
> &m_frame->page()->frameHost().consoleMessageStorage();
> nit: Can we use m_frame->host() instead of m_frame->page()->frameHost()?  Page
> is eventually going to go away.
That's right, we'll fix this.

>
https://codereview.chromium.org/645693003/diff/60001/Source/core/inspector/In...
> Source/core/inspector/InspectorInstrumentation.cpp:234: return
> instrumentationForPage(&host->page());
> This might be ok for now, but there's a comment on FrameHost::page() warning
> that it will eventually be removed (since Page is going away).
The thing is right now InspectorController object that is orchestrating all
inspector instrumentation is created per Page, not per FrameHost.
We are aware that we will need to move it to FrameHost eventually and the code
above will change naturally as well at the same moment.
We are not directly working on this yet though.