Remove WRAPPER_VISITOR_SPECIAL_CLASSES

Remove WRAPPER_VISITOR_SPECIAL_CLASSES

This patch removes the need of WRAPPER_VISITOR_SPECIAL_CLASSES. All of these classes are classes in core/ and WrapperVisitor and ScriptWrappableVisitor shouldn't really have any knowledge about them. It takes advantage of the fact that the various overloads of dispatchTraceWrappers(SpecialClass* t) all have the same implementation (t->traceWrappers(this)), and just adds that implementation directly into the special classes

BUG=682322
Review-Url: https://codereview.chromium.org/2728203002
Cr-Commit-Position: refs/heads/master@{#455298}
Committed: https://chromium.googlesource.com/chromium/src/+/0235e672998b36b2bf5beb45ce1e24ae3ac729af

[adithyas, 2017-03-03 20:05:34]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 20:05:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 21:39:39]

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

[commit-bot: I haz the power, 2017-03-03 21:39:40]

Dry run: This issue passed the CQ dry run.

[adithyas, 2017-03-03 21:40:07]

adithyas@chromium.org changed reviewers:
+ jbroman@chromium.org

[adithyas, 2017-03-03 21:40:08]

https://codereview.chromium.org/2728203002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/GarbageCollected.h (right):

https://codereview.chromium.org/2728203002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/GarbageCollected.h:250: class
CanTraceWrappers {
I don't know if this is useful anymore, I think the compiler would warn that "T"
does not have markAndDispatchTraceWrappers defined regardless.

[haraken, 2017-03-04 01:47:48]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, hlopko@chromium.org, mlippautz@chromium.org

[haraken, 2017-03-04 01:47:49]

+hlopko +mlippautz Do you think it's now fine to remove
WRAPPER_VISITOR_SPECIAL_CLASSES?

[Michael Lippautz, 2017-03-04 07:08:13]

On 2017/03/04 01:47:49, haraken wrote:
> +hlopko +mlippautz Do you think it's now fine to remove
> WRAPPER_VISITOR_SPECIAL_CLASSES?

Thanks for working on the unification. The change itself makes me a bit unhappy
as it breaks the basic principle of traced objects only providing tracing, while
WrapperVisitor/ScriptWrappableVisitor provide the GC infrastructure. Is this a
preparation to move WrapperVisitor and ScriptWrappableVisitor in a common
component where we cannot have access to core/ to dispatch?

At the very least I think we should have a macro 
  DECLARE_TRACE_WRAPPERS_WITHOUT_BASE() 
though that additionally hides the 
  void markAndDispatchTraceWrappers(const WrapperVisitor* visitor) const {
    traceWrappers(visitor);
  }
on anything that is non-ScriptWrappable or TraceWrapperBase.

Also, bindings/core/v8/TraceWrapperReference.md#Memory_footprint-critical-uses
requires an update.

[adithyas, 2017-03-06 23:27:30]

On 2017/03/04 at 07:08:13, mlippautz wrote:
> On 2017/03/04 01:47:49, haraken wrote:
> > +hlopko +mlippautz Do you think it's now fine to remove
> > WRAPPER_VISITOR_SPECIAL_CLASSES?
> 
> Thanks for working on the unification. The change itself makes me a bit
unhappy as it breaks the basic principle of traced objects only providing
tracing, while WrapperVisitor/ScriptWrappableVisitor provide the GC
infrastructure. Is this a preparation to move WrapperVisitor and
ScriptWrappableVisitor in a common component where we cannot have access to
core/ to dispatch?

This is in preparation to move ScriptWrappableVisitor into platform/bindings/
(where we don't have access to core/).

> 
> At the very least I think we should have a macro 
>   DECLARE_TRACE_WRAPPERS_WITHOUT_BASE() 
> though that additionally hides the 
>   void markAndDispatchTraceWrappers(const WrapperVisitor* visitor) const {
>     traceWrappers(visitor);
>   }
> on anything that is non-ScriptWrappable or TraceWrapperBase.
> 
> Also, bindings/core/v8/TraceWrapperReference.md#Memory_footprint-critical-uses
requires an update.

Added the macro and updated the documentation :)

[Michael Lippautz, 2017-03-07 07:59:51]

On 2017/03/06 23:27:30, adithyas wrote:
> On 2017/03/04 at 07:08:13, mlippautz wrote:
> > On 2017/03/04 01:47:49, haraken wrote:
> > > +hlopko +mlippautz Do you think it's now fine to remove
> > > WRAPPER_VISITOR_SPECIAL_CLASSES?
> > 
> > Thanks for working on the unification. The change itself makes me a bit
> unhappy as it breaks the basic principle of traced objects only providing
> tracing, while WrapperVisitor/ScriptWrappableVisitor provide the GC
> infrastructure. Is this a preparation to move WrapperVisitor and
> ScriptWrappableVisitor in a common component where we cannot have access to
> core/ to dispatch?
> 
> This is in preparation to move ScriptWrappableVisitor into platform/bindings/
> (where we don't have access to core/).

I am fine with the change since this might open up some new possibilities. The
reason I am careful here is that we've successfully switched to tri-color
marking (without the third color) without touching any of the DOM nodes. I'd
like to keep the possibility of changing GC internals without touching the DOM.

> 
> > 
> > At the very least I think we should have a macro 
> >   DECLARE_TRACE_WRAPPERS_WITHOUT_BASE() 
> > though that additionally hides the 
> >   void markAndDispatchTraceWrappers(const WrapperVisitor* visitor) const {
> >     traceWrappers(visitor);
> >   }
> > on anything that is non-ScriptWrappable or TraceWrapperBase.
> > 
> > Also,
bindings/core/v8/TraceWrapperReference.md#Memory_footprint-critical-uses
> requires an update.
> 
> Added the macro and updated the documentation :)

Thanks

[Michael Lippautz, 2017-03-07 08:00:01]

lgtm from my side

https://codereview.chromium.org/2728203002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/GarbageCollected.h (right):

https://codereview.chromium.org/2728203002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/GarbageCollected.h:250: class
CanTraceWrappers {
On 2017/03/03 21:40:07, adithyas wrote:
> I don't know if this is useful anymore, I think the compiler would warn that
"T"
> does not have markAndDispatchTraceWrappers defined regardless.

I would keep it as we should be able to generate better error messages using
this one.

[haraken, 2017-03-07 08:45:41]

LGTM

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeListsNodeData.h (right):

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeListsNodeData.h:172:
DECLARE_TRACE_WRAPPERS_WITHOUT_BASE();

Just to confirm: What happens if a developer uses DECLARE_TRACE_WRAPPERS()? Will
it lead to a compile error? I want to make sure that the new macro is not
error-prone.

[Marcel Hlopko, 2017-03-07 09:08:26]

lgtm

[adithyas, 2017-03-07 16:53:57]

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeListsNodeData.h (right):

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeListsNodeData.h:172:
DECLARE_TRACE_WRAPPERS_WITHOUT_BASE();
On 2017/03/07 at 08:45:40, haraken wrote:
> Just to confirm: What happens if a developer uses DECLARE_TRACE_WRAPPERS()?
Will it lead to a compile error? I want to make sure that the new macro is not
error-prone.

Yes, it leads to a compiler error as markAndDispatchTraceWrappers will not be
defined for the class.

[jbroman, 2017-03-07 17:46:12]

lgtm w/ nits

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/GarbageCollected.h (right):

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/GarbageCollected.h:250: class
CanTraceWrappers {
nit: Slightly more concisely (and happens to also check that it's a function
that returns void, not that it matters much):

template <typename T, typename = void>
struct CanTraceWrappers : std::false_type {};

template <typename T>
struct HasFoo<T, decltype(std::declval<T&>().markAndDispatchTraceWrappers())> :
std::true_type {};

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/GarbageCollected.h:261: static bool
const value =
nit: If you do keep this, prefer "const bool" to "bool const".

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:49: *    public:
nit: here (and above): "public:" should be indented 1 space, following lines 2
spaces and there should be a semicolon at the end of the class. Might as well
fix both to be consistent.

[jbroman, 2017-03-07 17:47:38]

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/GarbageCollected.h (right):

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/GarbageCollected.h:250: class
CanTraceWrappers {
On 2017/03/07 at 17:46:09, jbroman wrote:
> nit: Slightly more concisely (and happens to also check that it's a function
that returns void, not that it matters much):
> 
> template <typename T, typename = void>
> struct CanTraceWrappers : std::false_type {};
> 
> template <typename T>
> struct HasFoo<T, decltype(std::declval<T&>().markAndDispatchTraceWrappers())>
: std::true_type {};

Err, replace "HasFoo" with "CanTraceWrappers". Forgot to replace that when
editing.

[adithyas, 2017-03-07 18:40:45]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-03-07 18:40:46]

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/GarbageCollected.h (right):

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/GarbageCollected.h:250: class
CanTraceWrappers {
On 2017/03/07 at 17:47:38, jbroman wrote:
> On 2017/03/07 at 17:46:09, jbroman wrote:
> > nit: Slightly more concisely (and happens to also check that it's a function
that returns void, not that it matters much):
> > 
> > template <typename T, typename = void>
> > struct CanTraceWrappers : std::false_type {};
> > 
> > template <typename T>
> > struct HasFoo<T,
decltype(std::declval<T&>().markAndDispatchTraceWrappers())> : std::true_type
{};
> 
> Err, replace "HasFoo" with "CanTraceWrappers". Forgot to replace that when
editing.

Much better, thanks!

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2728203002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:49: *    public:
On 2017/03/07 at 17:46:10, jbroman wrote:
> nit: here (and above): "public:" should be indented 1 space, following lines 2
spaces and there should be a semicolon at the end of the class. Might as well
fix both to be consistent.

Fixed both.

[adithyas, 2017-03-07 18:40:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, mlippautz@chromium.org, jbroman@chromium.org,
hlopko@chromium.org
Link to the patchset: https://codereview.chromium.org/2728203002/#ps40001
(title: "Change CanTraceWrappers")

[commit-bot: I haz the power, 2017-03-07 18:41:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 23:55:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488912045831840,
"parent_rev": "8ea6b17294f2ce1ea405e020ca07d2e048a6a083", "commit_rev":
"0235e672998b36b2bf5beb45ce1e24ae3ac729af"}

[commit-bot: I haz the power, 2017-03-07 23:56:05]

Description was changed from

==========
Remove WRAPPER_VISITOR_SPECIAL_CLASSES

This patch removes the need of WRAPPER_VISITOR_SPECIAL_CLASSES. All of these
classes are classes in core/ and WrapperVisitor and ScriptWrappableVisitor
shouldn't really have any knowledge about them. It takes advantage of the fact
that the various overloads of dispatchTraceWrappers(SpecialClass* t) all have
the same implementation (t->traceWrappers(this)), and just adds that
implementation directly into the special classes

BUG=682322
==========

to

==========
Remove WRAPPER_VISITOR_SPECIAL_CLASSES

This patch removes the need of WRAPPER_VISITOR_SPECIAL_CLASSES. All of these
classes are classes in core/ and WrapperVisitor and ScriptWrappableVisitor
shouldn't really have any knowledge about them. It takes advantage of the fact
that the various overloads of dispatchTraceWrappers(SpecialClass* t) all have
the same implementation (t->traceWrappers(this)), and just adds that
implementation directly into the special classes

BUG=682322

Review-Url: https://codereview.chromium.org/2728203002
Cr-Commit-Position: refs/heads/master@{#455298}
Committed:
https://chromium.googlesource.com/chromium/src/+/0235e672998b36b2bf5beb45ce1e...
==========

[commit-bot: I haz the power, 2017-03-07 23:56:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0235e672998b36b2bf5beb45ce1e...