[webkit-reviews] review granted: [Bug 174478] Web Inspector: Add memoryCost to Inspector Protocol objects : [Attachment 315766] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 174478: Web Inspector: Add memoryCost to Inspector Protocol objects
https://bugs.webkit.org/show_bug.cgi?id=174478

Attachment 315766: Patch

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




--- Comment #7 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 315766
  --> https://bugs.webkit.org/attachment.cgi?id=315766
Patch

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

r=me

> Source/JavaScriptCore/inspector/InspectorValues.h:111
> +    virtual size_t memoryCost() const;

I feel like somewhere in this header should be a comment that this class
expects non-cyclic values, as we cannot serialize cyclic structures to JSON.
Right now we would infinite loop / hang attempting to either toJSONString() or
memoryCost() an InspectorValue with cyclic references.

At this point I think we should include Apple in the copyright of this header /
implementation. We've made enough changes over the years and this is a new API
on the class.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:2719
>		FE217ECB1640A54A0052988B /* JavaScriptCore */ = {

This group already existed? How peculiar!

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3275
> +				95646E5B1F1DB60E00DE0EB9 /* InspectorValue.cpp
in Sources */,

I suggest running:

    $ sort-Xcode-project-file TestWebKitAPI.xcodeproj/project.pbxproj
    $ git add -p TestWebKitAPI.xcodeproj/project.pbxproj
    # Either add the entire file, or only the lines with this new
InspectorValue file so it is sorted appropriately.

> Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:35
> +TEST(InspectorValue, MemoryCost_Null)

I think its more common to have names like "MemoryCostNull" than
"MemoryCost_Null", but I'll leave that up to you.

> Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:40
> +    EXPECT_LT(0U, memoryCost);
> +    EXPECT_GE(8U, memoryCost);

Gosh this is hard to read... It would read so much better if it was flipped.
For example:

    EXPECT_LE(memoryCost, 8U);
    EXPECT_GT(memoryCost, 0U);

It looks like other tests use this kind of formatting as well. So lets switch.

> Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:97
> +    EXPECT_LT(valueA->memoryCost(), valueB->memoryCost());

Notice here it reads naturally. A is less than B, B is less than C, ...

> Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:106
> +    Ref<InspectorValue> valueA = InspectorValue::create("t");
> +    Ref<InspectorValue> valueB = InspectorValue::create("ð");
> +    EXPECT_LE(valueA->memoryCost(), valueB->memoryCost());

Are the actual values here equal or different? (Keep the test the same, I just
want to verify that non-8 bit strings are larger.

> Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:189
> +    Ref<InspectorArray> valueA = InspectorArray::create();
> +    valueA->pushValue(InspectorValue::null());
> +
> +    Ref<InspectorArray> valueB = InspectorArray::create();
> +    valueB->pushValue(InspectorValue::null());
> +    valueB->pushValue(InspectorValue::null());
> +
> +    Ref<InspectorArray> valueC = InspectorArray::create();
> +    valueC->pushValue(InspectorValue::null());
> +    valueC->pushValue(InspectorValue::null());
> +    valueC->pushValue(InspectorValue::null());
> +
> +    Ref<InspectorArray> valueD = InspectorArray::create();
> +    valueD->pushValue(InspectorValue::null());
> +    valueD->pushValue(InspectorValue::null());
> +    valueD->pushValue(InspectorValue::null());
> +    valueD->pushValue(InspectorValue::null());

You could simplify this test quite a bite by using a single array and grabbing
its memory size as you grow it:

    Ref<InspectorArray> value = InspectorArray::create();
    size_t memoryCost0 = value->memoryCost();
    value->pushValue(InspectorValue::null());
    size_t memoryCost1 = value->memoryCost();
    value->pushValue(InspectorValue::null());
    size_t memoryCost2 = value->memoryCost();
    value->pushValue(InspectorValue::null());
    size_t memoryCost3 = value->memoryCost();

    EXPECT_LT(memoryCost0, memoryCost1);
    EXPECT_LT(memoryCost1, memoryCost2);
    EXPECT_LT(memoryCost2, memoryCost3);

You can do the same for the object growing test.