[webkit-reviews] review granted: [Bug 137131] Web Inspector: tests under LayoutTests/inspector/debugger are flaky : [Attachment 379462] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 137131: Web Inspector: tests under LayoutTests/inspector/debugger are flaky
https://bugs.webkit.org/show_bug.cgi?id=137131

Attachment 379462: Patch

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




--- Comment #12 from Brian Burg <bburg at apple.com> ---
Comment on attachment 379462
  --> https://bugs.webkit.org/attachment.cgi?id=379462
Patch

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

r=me, however I would also like Joe to also take a look.

> Source/JavaScriptCore/ChangeLog:19
> +	   legtmost one first.

nit: leftmost

> Source/WebCore/ChangeLog:28
> +	   in one batched but for now I'd like to keep it one per iteration.

nit: in one batch

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:51
> +	   // We are guarenteed to have a Leave for an Entry so we don't need
to bounds check.

nit: guaranteed

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:83
> +    for (++it; it != m_positions.end(); ++it) {

It's kind of unusual to put the ++ operator in the first clause, but I guess
it's okay?

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:102
> +	   if (m_hasTaskInQueue)

It would be more consistent to name this m_hasScheduledTask or similar. Whether
RunLoop uses a queue or not shouldn't leak out to its clients, as it's not
accessible.