[webkit-reviews] review granted: [Bug 206191] Web Inspector: crash in DumpRenderTree at com.apple.JavaScriptCore: WTF::RefCountedBase::hasOneRef const : [Attachment 387562] 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 206191: Web Inspector: crash in DumpRenderTree at com.apple.JavaScriptCore:
WTF::RefCountedBase::hasOneRef const
https://bugs.webkit.org/show_bug.cgi?id=206191

Attachment 387562: Patch

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




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

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

r=me

> Source/JavaScriptCore/debugger/Debugger.cpp:127
> +		       if (function->scope()->globalObject() == globalObject &&
function->executable()->isFunctionExecutable() &&
!function->isHostOrBuiltinFunction())

I would find this easier to read and modify in the future if this used early
return conditions like it was before. For example:

    if (function->scope()->globalObject() != m_globalObject)
	return IterationStatus::Continue;

    if (!function->executable()->isFunctionExecutable())
	return IterationStatus::Continue;

    if (function->isHostOrBuiltinFunction())
	return IterationStatus::Continue;

But I'll leave that style choice up to you.

> Source/JavaScriptCore/debugger/Debugger.cpp:135
> +	   sourceParsed(globalObject, sourceProvider.get(), -1, nullString());

Is there a meaningful difference between switching to `nullString` here?