[webkit-reviews] review denied: [Bug 29080] Geolocation Coordinates::toString() prints bogus values for unspecified properties. : [Attachment 39271] Patch 2 for bug 29080

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Steve Block <steveblock at google.com>'s
request for review:
Bug 29080: Geolocation Coordinates::toString() prints bogus values for
unspecified properties.
https://bugs.webkit.org/show_bug.cgi?id=29080

Attachment 39271: Patch 2 for bug 29080
https://bugs.webkit.org/attachment.cgi?id=39271&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
These results assume that the mock makes a synchronous callback.  Probably
warrants a comment.

Also:
+    window.layoutTestController.notifyDone();
is not needed if this is sync.

Nit 4space indent:
+function shouldContainString(expression, string)
+{
+  var expressionResult = eval(expression);
+
+  if (expressionResult.indexOf(string) != -1)
+    testPassed(expression + ' contains "' + string + '".');
+  else
+    testFailed(expression + ' should contain "' + string + '". Was "' +
stringify(expressionResult) + '".');
+}

window. are not technically needed here, but certainly don't hurt:
+window.layoutTestController.setGeolocationPermission(true);
+window.layoutTestController.setMockGeolocationPosition(mockLatitude,

Probably needs a comment:
+    position = p;  // shouldBe can't use local variables yet.

Oh, I see.  You modified the template to not include js-post.  In that case,
you really should add this script to the exclusion list for
make-script-test-wrappers.  And possibly ignore my comments above about this
being sync.

In general this looks good, but could use one more round.