[webkit-reviews] review denied: [Bug 91535] [Chromium] Out of Memory is observed when a large object is passed to a Web Worker : [Attachment 152837] Fixed style error.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dmitry Titov
<dimich at chromium.org>'s request for review:
Bug 91535: [Chromium] Out of Memory is observed when a large object is passed
to a Web Worker
https://bugs.webkit.org/show_bug.cgi?id=91535

Attachment 152837: Fixed style error.
https://bugs.webkit.org/attachment.cgi?id=152837&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152837&action=review


> Source/WebCore/ChangeLog:8
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

Please remove.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2332
> +    if ((m_externallyAllocatedMemory = m_data.length()))

Whenever I see "if (a=b)" my bug sense goes off. I know you put in the double
(( to show it as intention, but it feels like it would be clearer just to do it
on another line.

Also there is no need for the "if" really.

m_externallyAllocatedMemory = m_data.length();
v8::V8::AdjustAmountOfExternalAllocatedMemory(m_externallyAllocatedMemory);

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2338
> +    if (m_externallyAllocatedMemory)

No need for if here.

> Source/WebCore/dom/MessageEvent.cpp:77
> +    if (m_dataAsSerializedScriptValue)

Can we do this any other place? So that everyone who uses SerializedScriptValue
will get this for free without having to know to do this.

If we do need to do something like this, is it possible to add asserts in key
places (methods?) in SerializedScriptValue to make sure that this function is
called when appropriate?