Decouple memory() from Console

Decouple memory() from Console

in preparation for moving core/timing/ to modules

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

[pilgrim_google, 2015-02-24 14:28:54]

pilgrim@chromium.org changed reviewers:
+ dsinclair@chromium.org, jsbell@chromium.org, mkwst@chromium.org

[pilgrim_google, 2015-02-24 14:28:54]

Stuck on compile error, please send help

In file included from
../../third_party/WebKit/Source/core/timing/ConsoleMemory.cpp:6:
In file included from
../../third_party/WebKit/Source/core/timing/ConsoleMemory.h:8:
In file included from
../../third_party/WebKit/Source/platform/Supplementable.h:29:
../../third_party/WebKit/Source/platform/heap/Handle.h:332:9: error:
static_assert failed "non-garbage collected object should not be in Persistent"
        STATIC_ASSERT_IS_GARBAGE_COLLECTED(T, "non-garbage collected object
should not be in Persistent");
       
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/platform/heap/Visitor.h:71:5: note: expanded
from macro 'STATIC_ASSERT_IS_GARBAGE_COLLECTED'
    static_assert(IsGarbageCollectedType<T>::value, ErrorMessage)
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/platform/heap/InlinedGlobalMarkingVisitor.h:95:15:
note: in instantiation of member function 'blink::Persistent<blink::MemoryInfo,
blink::ThreadLocalPersistents<0> >::trace' requested here
        self->trace(visitor);
              ^
../../third_party/WebKit/Source/platform/heap/Visitor.h:452:39: note: in
instantiation of member function
'blink::TraceCompatibilityAdaptor<blink::Persistent<blink::MemoryInfo,
blink::ThreadLocalPersistents<0> >, false>::trace' requested here
        TraceCompatibilityAdaptor<T>::trace(Derived::fromHelper(this),
&const_cast<T&>(t));
                                      ^
../../third_party/WebKit/Source/platform/heap/Visitor.h:603:33: note: in
instantiation of function template specialization
'blink::VisitorHelper<blink::Visitor>::trace<blink::Persistent<blink::MemoryInfo,
blink::ThreadLocalPersistents<0> > >' requested here
        VisitorHelper<Visitor>::trace(t);
                                ^
../../third_party/WebKit/Source/core/timing/ConsoleMemory.cpp:17:14: note: in
instantiation of function template specialization
'blink::Visitor::trace<blink::Persistent<blink::MemoryInfo,
blink::ThreadLocalPersistents<0> > >' requested here
    visitor->trace(m_memory);

[dsinclair, 2015-02-24 14:33:31]

+kouhei is this due to tracing macros?

[kouhei (in TOK), 2015-02-24 14:45:21]

On 2015/02/24 14:33:31, dsinclair wrote:
> +kouhei is this due to tracing macros?

I think so. Let me revert the patch. Sorry for the trouble.

[kouhei (in TOK), 2015-02-24 14:50:57]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2015-02-24 14:50:58]

On 2015/02/24 14:45:21, kouhei wrote:
> On 2015/02/24 14:33:31, dsinclair wrote:
> > +kouhei is this due to tracing macros?
> 
> I think so. Let me revert the patch. Sorry for the trouble.

Sorry, I was wrong. TOT seems OK, so the build issue is from the patch.

https://codereview.chromium.org/950343002/diff/20001/Source/core/timing/Conso...
File Source/core/timing/ConsoleMemory.h (right):

https://codereview.chromium.org/950343002/diff/20001/Source/core/timing/Conso...
Source/core/timing/ConsoleMemory.h:30: PersistentWillBeMember<MemoryInfo>
m_memory;
Please use RefPtrWillBeMember here, as MemoryInfo is RefCounted in non-Oilpan
builds.

https://codereview.chromium.org/950343002/diff/20001/Source/core/timing/Memor...
File Source/core/timing/MemoryInfo.h (right):

https://codereview.chromium.org/950343002/diff/20001/Source/core/timing/Memor...
Source/core/timing/MemoryInfo.h:46: static MemoryInfo* create() { return new
MemoryInfo(); }
I think we should continue to use PassRefPtrWillBeRawPtr & adoptRefWillBeNoop
here, as MemoryInfo is RefCounted in !ENABLE(OILPAN)

[pilgrim_google, 2015-02-24 16:08:10]

Thanks for the help. I incorporated your feedback and made some other final
fixes, and it seems to be working now. Re-review?

[dsinclair, 2015-02-24 16:18:43]

https://codereview.chromium.org/950343002/diff/50001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/950343002/diff/50001/Source/core/core.gypi#ne...
Source/core/core.gypi:422: 'timing/MemoryInfo.idl',
nit: weird spacing.

https://codereview.chromium.org/950343002/diff/50001/Source/core/timing/Conso...
File Source/core/timing/ConsoleMemory.cpp (right):

https://codereview.chromium.org/950343002/diff/50001/Source/core/timing/Conso...
Source/core/timing/ConsoleMemory.cpp:21: const char*
ConsoleMemory::supplementName()
If left here, should this have a // static to be consistent?

https://codereview.chromium.org/950343002/diff/50001/Source/core/timing/Conso...
Source/core/timing/ConsoleMemory.cpp:23: return "ConsoleMemory";
Any reason to not put this in the header?

[pilgrim_google, 2015-02-24 17:06:33]

On 2015/02/24 at 16:18:43, dsinclair wrote:
>
https://codereview.chromium.org/950343002/diff/50001/Source/core/core.gypi#ne...
> Source/core/core.gypi:422: 'timing/MemoryInfo.idl',
> nit: weird spacing.

Fixed.

>
https://codereview.chromium.org/950343002/diff/50001/Source/core/timing/Conso...
> Source/core/timing/ConsoleMemory.cpp:23: return "ConsoleMemory";
> Any reason to not put this in the header?

No. Moved.

[dsinclair, 2015-02-24 17:10:26]

lgtm. The description needs to be updated.

[pilgrim_google, 2015-02-24 17:14:44]

On 2015/02/24 at 17:10:26, dsinclair wrote:
> lgtm. The description needs to be updated.

Would feel more secure with an explicit l-g-t-m from kouhei for the memory stuff
and macros that I don't fully understand.

[jsbell, 2015-02-24 18:32:17]

lgtm

https://codereview.chromium.org/950343002/diff/70001/Source/core/frame/Consol...
File Source/core/frame/Console.cpp (left):

https://codereview.chromium.org/950343002/diff/70001/Source/core/frame/Consol...
Source/core/frame/Console.cpp:82: // FIXME: Because we create a new object here
each time,
This FIXME was fixed by the new ConsoleMemory::memory(), correct?

Can you add a layout test that shows console.memory === console.memory ?

https://codereview.chromium.org/950343002/diff/70001/Source/core/timing/Conso...
File Source/core/timing/ConsoleMemory.h (right):

https://codereview.chromium.org/950343002/diff/70001/Source/core/timing/Conso...
Source/core/timing/ConsoleMemory.h:16: : public
NoBaseWillBeGarbageCollectedFinalized<ConsoleMemory>
Nit: blink style would keep these all on one line

[kouhei (in TOK), 2015-02-24 23:19:46]

oilpan lgtm. Thanks!

[pilgrim_google, 2015-02-25 16:24:43]

Nits addressed and layout test added. Thanks all.

[pilgrim_google, 2015-02-25 16:41:40]

The CQ bit was checked by pilgrim@chromium.org

[pilgrim_google, 2015-02-25 16:41:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, jsbell@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/950343002/#ps110001
(title: "nits")

[commit-bot: I haz the power, 2015-02-25 16:42:44]

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

[commit-bot: I haz the power, 2015-02-25 17:58:10]

Committed patchset #7 (id:110001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190844

[sof, 2015-07-13 14:02:47]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-07-13 14:02:48]

https://codereview.chromium.org/950343002/diff/110001/Source/core/timing/Cons...
File Source/core/timing/ConsoleMemory.cpp (right):

https://codereview.chromium.org/950343002/diff/110001/Source/core/timing/Cons...
Source/core/timing/ConsoleMemory.cpp:43: return m_memory.get();
Sharing MemoryInfo across accesses is subtly incorrect, as it will report
immutable heap values. Switching back to previous via
https://codereview.chromium.org/1230203003/