[webkit-reviews] review canceled: [Bug 52877] Detach Geolocation from Frame when Page destroyed. : [Attachment 82672] Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has canceled  review:
Bug 52877: Detach Geolocation from Frame when Page destroyed.
https://bugs.webkit.org/show_bug.cgi?id=52877

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82672&action=review

Looking at the patch from transferrable iframe point of view.

When we transfer iframe, the main idea of the feature is that it continues all
operations. If it is being loaded - it continues to load, if it has timers or
XHRs in flight - they continue to work. Basically, since the iframe has its own
origin, the security checks and permissions of various kind can continue to be
checked against that origin. It would be unfortunate if the geolocation will
stop working when iframe is transferred. It would be nice to figure out how to
make it work, unless it contradicts the was Geolocation permissions are
granted.

Couple of random nits below:

> Source/WebCore/page/DOMWindow.cpp:712
> +void DOMWindow::detachFromPage()

This patch introduces several Foo::detachFromPage() methods that all call each
other in chain Frame->DOMWindow->Navigator->Geolocation.

In general, it is better to have a specific method name that describes 'what
method is doing' rather then 'when method is called'. The too-generic methods
like detachFromPage can get very confusing as people add more unrelated stuff
to them. 

For example, DOMWindow will now have detachFromPage(), pageDestroyed(),
disconnectFrame() and clear(). To understand the difference one has to figure
out all the callers.

Please consider having a specific method, like resetGeolocationPermissions() or
something like this instead of creating another generic callback like
detachFromPage()

> Source/WebCore/page/Geolocation.cpp:706
> +    Page* thePage = page();

Does adding 'the' in front of the name add anything to the meaning here? In
most of WebKit, 'the' or 'a' are not used as prefixes...