[webkit-reviews] review granted: [Bug 172848] Web Inspector: Improve ES6 Class instances in Heap Snapshot instances view : [Attachment 362394] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 172848: Web Inspector: Improve ES6 Class instances in Heap Snapshot
instances view
https://bugs.webkit.org/show_bug.cgi?id=172848

Attachment 362394: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=362394&action=review




--- Comment #49 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 362394
  --> https://bugs.webkit.org/attachment.cgi?id=362394
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=362394&action=review

r=me with issues resolved.

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:262
> +typedef enum {
> +    Internal      = 1 << 0,
> +    ObjectSubtype = 1 << 1,
> +} NodeFlags;

Why not use an enum class?  With unified sources, it's conceivable that an enum
named "Internal" can easily conflict with another (if not presently, then
perhaps in the future).

> Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:417
> +	       if (!structure || !structure->globalObject())

I think the only time structure can be null is if the cell has been GCed.  Are
you seeing this in the wild? m It's OK to leave this null check for now, but I
would be surprised if this is actually needed.

> Source/JavaScriptCore/runtime/JSObject.cpp:528
> +    auto structure = object->structure();

It's clearer to use auto* here.

> Source/JavaScriptCore/runtime/JSObject.cpp:529
> +    auto globalObject = structure->globalObject();

Ditto.

> Source/JavaScriptCore/runtime/JSObject.cpp:532
> +    auto exec = globalObject->globalExec();

Ditto.

> Source/JavaScriptCore/runtime/JSObject.cpp:538
>	   if (slot.isValue()) {

I think you need an "EXCEPTION_ASSERT(!scope.exception());" above this line to
placate the exception check verifier.

> Source/JavaScriptCore/runtime/JSObject.cpp:562
> +		   if (protoObject->getPropertySlot(exec,
vm.propertyNames->constructor, slot)) {
> +		       if (slot.isValue()) {

Ditto: you'll need an "EXCEPTION_ASSERT(!scope.exception());" between these
lines to placate the exception check verifier.