[webkit-reviews] review denied: [Bug 194047] Integrate JSC bytecode cache with WebKit : [Attachment 360620] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 194047: Integrate JSC bytecode cache with WebKit
https://bugs.webkit.org/show_bug.cgi?id=194047

Attachment 360620: Patch

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




--- Comment #5 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 360620
  --> https://bugs.webkit.org/attachment.cgi?id=360620
Patch

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

Looks generally good but lets get it apply before r+ing.

>>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:850
>>> +	     Vector<IPC::DerivedData>* derivedData = new
Vector<IPC::DerivedData>();
>> 
>> No need to call new here.  Just WTFMove an instance into the lambda capture
list.
> 
> That would move it multiple times.

Please use smart pointers for memory management. Calling new (outside create()
type functions that return smart pointers) is discouraged. 

auto derivedData = std::make_unique<Vector<IPC::DerivedData>>();

>>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:857
>>> +			 return;
>> 
>> Why?
> 
> Not sure actually, this was part of the code that you had deleted and I added
back. I assume that if this is the only ref to the loader, then we don't care
about the response anymore?

This lambda is then the only ref keeping the loader alive which implies there
are no clients. A more stylish solution would use WeakPtr.