[webkit-reviews] review denied: [Bug 119548] Refactoring of throw exception. : [Attachment 209914] patch 7

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 28 14:18:13 PDT 2013


Geoffrey Garen <ggaren at apple.com> has denied Chris Curtis
<chris_curtis at apple.com>'s request for review:
Bug 119548: Refactoring of throw exception.
https://bugs.webkit.org/show_bug.cgi?id=119548

Attachment 209914: patch 7
https://bugs.webkit.org/attachment.cgi?id=209914&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209914&action=review


I would normally mark this r+, and then let you resolve these comments before
landing, but since you're using the commit queue, you'll need to upload a new
patch to land.

> Source/JavaScriptCore/runtime/VM.cpp:595
> +	   // No range information, so give a few characters of context

Let's upgrade this to a complete sentence by adding a "." at the end.

> Source/JavaScriptCore/runtime/VM.cpp:601
> +	   // then strip whitespace.

Let's upgrade this to a complete sentence by capitalizing "Then".

> Source/JavaScriptCore/runtime/VM.cpp:637
> +	   // FIXME: should only really be adding these properties to VM
generated exceptions,

Let's upgrade this to a complete sentence by adding a "We".

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html:94
> +	       do{

Need a space after "do" here.

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html:98
> +		       hasCompensatedAlready++ ;	     
> +	       } while (hasCompensatedAlready == 1 && lastReadyStateObserved ==
1);

Why did hasCompensatedAlready change to a count when we're only trying to count
from 0 to 1? That seems like a boolean to me. If it is a real count, we should
rename hasCompensatedAlready to something like "compensationCount", to indicate
that it's a count.


More information about the webkit-reviews mailing list