Oilpan: Prepare moving InspectorAgent related classes to oilpan

Oilpan: Prepare moving InspectorAgent related classes to oilpan

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

[tkent, 2014-06-10 09:19:46]

It looks Patch Set 1 depends on another CL.

[keishi, 2014-06-10 09:20:59]

On 2014/06/10 09:19:46, tkent wrote:
> It looks Patch Set 1 depends on another CL.

This depends on Patch set 7 of https://codereview.chromium.org/307943002/

[keishi, 2014-06-11 05:32:22]

This CL depends on https://codereview.chromium.org/307943002/. PTAL

[tkent, 2014-06-11 05:48:50]

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

https://codereview.chromium.org/323043002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorTimelineAgent.cpp:160: class TimelineRecordStack
: public NoBaseWillBeGarbageCollectedFinalized<TimelineRecordStack> {
You don't need to make TimelineRecordStack on-heap because it's always used as a
part object of TimelineThreadState.

- Add DISALLOW_ALLOCATION() to TimelineRecordStack
- Add TimelineRecordStack::trace().
- TimelineRecordStack (not OwnPtrWillBeMember<>) should be a data member of
TimelineThreadState as ever.
- Trace it in TimelineThreadState::trace().

[keishi, 2014-06-19 04:36:22]

Also fixed FIXMEs from https://codereview.chromium.org/307943002/

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

https://codereview.chromium.org/323043002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorTimelineAgent.cpp:160: class TimelineRecordStack
: public NoBaseWillBeGarbageCollectedFinalized<TimelineRecordStack> {
On 2014/06/11 05:48:49, tkent wrote:
> You don't need to make TimelineRecordStack on-heap because it's always used as
a
> part object of TimelineThreadState.
> 
> - Add DISALLOW_ALLOCATION() to TimelineRecordStack
> - Add TimelineRecordStack::trace().
> - TimelineRecordStack (not OwnPtrWillBeMember<>) should be a data member of
> TimelineThreadState as ever.
> - Trace it in TimelineThreadState::trace().

Done.

[haraken, 2014-06-19 05:49:54]

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.h:159: typedef WillBeHeapHashMap<Node*,
RefPtrWillBeMember<InspectorStyleSheetForInlineStyle> >
NodeToInspectorStyleSheet; // bogus "stylesheets" with elements' inline styles

I think this Node* can be a strong pointer. So you can use RawPtrWillBeMember.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:186:
RevalidateStyleAttributeTask(InspectorDOMAgent*);

Add explicit.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorStyleSheet.h (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorStyleSheet.h:145: class Listener {

You can make this a GC-mixin and use a Member for
InspectorStyleSheetBase::m_listener.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:182:
TimelineRecordStack(InspectorTimelineAgent*);

Add explicit.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:201: struct TimelineThreadState
: public NoBaseWillBeGarbageCollectedFinalized<TimelineThreadState> {

I think you can use ALLOW_ONLY_INLINE_ALLOCATION instead of making it
GC-allocated.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:201: struct TimelineThreadState
: public NoBaseWillBeGarbageCollectedFinalized<TimelineThreadState> {

Nit: struct => class

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:1333:
recordStack.trace(visitor);

visitor->trace(recordStack) should work.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.h:317: typedef
WillBeHeapHashMap<ThreadIdentifier, OwnPtrWillBeMember<TimelineThreadState> >
ThreadStateMap;

Can we keep using TimelineThreadState (instead of making it a
OwnPtrWillBeMember) by making TimelineThreadState ALLOW_ONLY_INLINE_ALLOCATION?

[keishi, 2014-07-24 02:15:02]

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorCSSAgent.h:159: typedef WillBeHeapHashMap<Node*,
RefPtrWillBeMember<InspectorStyleSheetForInlineStyle> >
NodeToInspectorStyleSheet; // bogus "stylesheets" with elements' inline styles
On 2014/06/19 05:49:53, haraken wrote:
> 
> I think this Node* can be a strong pointer. So you can use RawPtrWillBeMember.

Done.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:186:
RevalidateStyleAttributeTask(InspectorDOMAgent*);
On 2014/06/19 05:49:53, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorTimelineAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:182:
TimelineRecordStack(InspectorTimelineAgent*);
On 2014/06/19 05:49:53, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:201: struct TimelineThreadState
: public NoBaseWillBeGarbageCollectedFinalized<TimelineThreadState> {
On 2014/06/19 05:49:53, haraken wrote:
> 
> I think you can use ALLOW_ONLY_INLINE_ALLOCATION instead of making it
> GC-allocated.

Done.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:201: struct TimelineThreadState
: public NoBaseWillBeGarbageCollectedFinalized<TimelineThreadState> {
On 2014/06/19 05:49:53, haraken wrote:
> 
> Nit: struct => class

Should this be a class? The accessors are public.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.cpp:1333:
recordStack.trace(visitor);
On 2014/06/19 05:49:53, haraken wrote:
> 
> visitor->trace(recordStack) should work.

Done.

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/323043002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorTimelineAgent.h:317: typedef
WillBeHeapHashMap<ThreadIdentifier, OwnPtrWillBeMember<TimelineThreadState> >
ThreadStateMap;
On 2014/06/19 05:49:53, haraken wrote:
> 
> Can we keep using TimelineThreadState (instead of making it a
> OwnPtrWillBeMember) by making TimelineThreadState
ALLOW_ONLY_INLINE_ALLOCATION?

Done.

[haraken, 2014-07-25 01:38:45]

Mostly looks good.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorConsoleAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorConsoleAgent.cpp:79: m_state = nullptr;

If you want to clear m_state, it's better to clear it in ~InspectorAgent()
because m_state is a member of InspectorAgent.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:183: class
RevalidateStyleAttributeTask : public
NoBaseWillBeGarbageCollectedFinalized<RevalidateStyleAttributeTask> {

Add FINAL.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:590:
m_revalidateStyleAttrTask->reset();

discardFrontendBindings() is called in a destructor, so you cannot touch
m_revalidateStyleAttrTask. I don't think you need to call
m_revalidateStyleAttrTask->reset() because it is just stopping a timer.

Maybe can we simply remove discardFrontendBindings() from oilpan builds?
Probably we can just remove reset() from ~InspectorDOMAgent().

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorHeapProfilerAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorHeapProfilerAgent.cpp:51: class
InspectorHeapProfilerAgent::HeapStatsUpdateTask : public
NoBaseWillBeGarbageCollectedFinalized<InspectorHeapProfilerAgent::HeapStatsUpdateTask>
{

Add FINAL.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorHeapProfilerAgent.cpp:53:
HeapStatsUpdateTask(InspectorHeapProfilerAgent*);

Add explicit.

[keishi, 2014-07-25 03:56:28]

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorConsoleAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorConsoleAgent.cpp:79: m_state = nullptr;
On 2014/07/25 01:38:44, haraken wrote:
> 
> If you want to clear m_state, it's better to clear it in ~InspectorAgent()
> because m_state is a member of InspectorAgent.

Done.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:183: class
RevalidateStyleAttributeTask : public
NoBaseWillBeGarbageCollectedFinalized<RevalidateStyleAttributeTask> {
On 2014/07/25 01:38:44, haraken wrote:
> 
> Add FINAL.

Done.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.cpp:590:
m_revalidateStyleAttrTask->reset();
On 2014/07/25 01:38:44, haraken wrote:
> 
> discardFrontendBindings() is called in a destructor, so you cannot touch
> m_revalidateStyleAttrTask. I don't think you need to call
> m_revalidateStyleAttrTask->reset() because it is just stopping a timer.
> 
> Maybe can we simply remove discardFrontendBindings() from oilpan builds?
> Probably we can just remove reset() from ~InspectorDOMAgent().

reset() was wrapped in #!ENABLE(OILPAN) in r178895 so I think this is ok.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorHeapProfilerAgent.cpp (right):

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorHeapProfilerAgent.cpp:51: class
InspectorHeapProfilerAgent::HeapStatsUpdateTask : public
NoBaseWillBeGarbageCollectedFinalized<InspectorHeapProfilerAgent::HeapStatsUpdateTask>
{
On 2014/07/25 01:38:44, haraken wrote:
> 
> Add FINAL.

Done.

https://codereview.chromium.org/323043002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorHeapProfilerAgent.cpp:53:
HeapStatsUpdateTask(InspectorHeapProfilerAgent*);
On 2014/07/25 01:38:45, haraken wrote:
> 
> Add explicit.

Done.

[haraken, 2014-07-25 04:17:52]

LGTM

[keishi, 2014-07-25 07:34:15]

+yurys +pfeldman

[yurys, 2014-07-26 04:28:15]

lgtm

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

https://codereview.chromium.org/323043002/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorConsoleAgent.cpp:78: m_instrumentingAgents =
nullptr;
I don't think lines 78-79 make much sense, it should be safe to drop them.

[keishi, 2014-07-28 08:36:46]

Thanks.

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

https://codereview.chromium.org/323043002/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorConsoleAgent.cpp:78: m_instrumentingAgents =
nullptr;
On 2014/07/26 04:28:15, yurys wrote:
> I don't think lines 78-79 make much sense, it should be safe to drop them.

Done.

[keishi, 2014-07-28 08:36:50]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-07-28 08:37:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/323043002/80001

[commit-bot: I haz the power, 2014-07-28 09:43:02]

Change committed as 179015