[webkit-reviews] review canceled: [Bug 193796] Web Inspector: Exclude Debugger Threads from CPU Usage values in Web Inspector : [Attachment 360078] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 193796: Web Inspector: Exclude Debugger Threads from CPU Usage values in
Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=193796

Attachment 360078: [PATCH] Proposed Fix

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




--- Comment #9 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 360078
  --> https://bugs.webkit.org/attachment.cgi?id=360078
[PATCH] Proposed Fix

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

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:143
>> +static Vector<MachSendRight> filterThreads(Function<bool (mach_port_t)>&&
filterFunction)
> 
> Is this the best name for this?  I feel like the old `threadSendRights` was a
bit clearer as to what this function actually did.  Maybe we should just have a
better name for `filterFunction`, like `sendPredicate`?
> 
> Alternatively, couldn't you filter the list previously returned by
`threadSendRights` after it's been called (create a `Vector::copyMatching`),
this way we can avoid some of the other logic in this function (e.g.
`threadSendRights` is guaranteed to be a superset of
`threadSendRightsExcludingDebuggerThreads`, so we shouldn't have to call
`task_threads` again)?

That sounds good.

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:181
>> +static float cpuUsage(Vector<MachSendRight> machThreads)
> 
> NIT: should this be a & to avoid copying the vector?

Yep!

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:227
>> +#if ENABLE(SAMPLING_PROFILER)
> 
> NIT: shouldn't only the `m_samplingProfilerMachThread = ...` line be wrapped
in the `#if`?  This would let you avoid defining the function twice.
> 
>     void ResourceUsageThread::platformSaveStateBeforeStarting()
>     {
>     #if ENABLE(SAMPLING_PROFILER)
>	  m_samplingProfilerMachThread = m_vm->samplingProfiler() ?
m_vm->samplingProfiler()->machThread() : MACH_PORT_NULL;
>     #endif
>     }

Will do.