[webkit-reviews] review denied: [Bug 52216] GeolocationController should call stopUpdating on destruction : [Attachment 78695] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied John Knottenbelt
<jknotten at chromium.org>'s request for review:
Bug 52216: GeolocationController should call stopUpdating on destruction
https://bugs.webkit.org/show_bug.cgi?id=52216

Attachment 78695: Patch
https://bugs.webkit.org/attachment.cgi?id=78695&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78695&action=review

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:12
> +    debug('This test can not be run without the LayoutTestController');

testFailed

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:15
> +  debug("Received Geoposition.");

testPassed

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:17
> +  window.setTimeout(waitForWindowToClose, 1);

Why 1?	 Only use 0 to keep things more deterministic.

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:22
> +    window.setTimeout(waitForWindowToClose, 1);

ditto

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:25
> +  debug("Success - no crash!");

testPassed

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:30
> +  debug("Failed to create watch: " + e);

Use the function that prints it out in red text and suhc.....testFialed maybe?

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1545
> +	  
windowList[i]->geolocationClientMock()->setError(arguments[0].toInt32(),
cppVariantToWebString(arguments[1]));

Will this work as expected?  For example, if one window sets one thing and
another sets another thing, only the last one wins.

Should this be a per-window thing?  Can it be?