[webkit-reviews] review granted: [Bug 200436] navigator.geolocation wrapper should not become GC-collectable once its frame is detached : [Attachment 375543] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 200436: navigator.geolocation wrapper should not become GC-collectable once
its frame is detached
https://bugs.webkit.org/show_bug.cgi?id=200436

Attachment 375543: Patch

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




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 375543
  --> https://bugs.webkit.org/attachment.cgi?id=375543
Patch

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

Looks great.

> Source/WebCore/Modules/geolocation/Geolocation.h:77
> +    WEBCORE_EXPORT Frame* frame() const;

I guess there must be important clients of this function, but seems annoying
that we have it.

> Source/WebCore/Modules/geolocation/Geolocation.h:163
> +    } m_allowGeolocation { Unknown };

Not sure you will agree, but I think this would read slightly better all on one
line:

    enum { Unknown, InProgress, Yes, No } m_allowGeolocation { Unknown };

> Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:76
> +	   m_geolocation = Geolocation::create(*m_navigator);

Is this correct for the case where m_navigator is nullptr? I assume it can be
null because it’s a WeakPtr.

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:33
> +    visitor.addOpaqueRoot(static_cast<NavigatorBase*>(&wrapped()));

I wish there was a safer cast for upcasts rather than static_cast, which can
also do other kinds of casting like downcasts. I also wish there was less void*
danger here. If we passed &wrapped() without casting to NavigatorBase*, how
would we know we had it wrong?

> Source/WebCore/bindings/js/JSWorkerNavigatorCustom.cpp:33
> +    visitor.addOpaqueRoot(static_cast<NavigatorBase*>(&wrapped()));

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4686
> +		   $rootString	= "    auto* root =
static_cast<NavigatorBase*>(WTF::getPtr(js${interfaceName}->wrapped().navigator
()));\n";

Ditto.

> Source/WebCore/plugins/DOMMimeTypeArray.h:43
> +    Navigator* navigator() { return m_navigator.get(); }

Inconsistent that the other getter functions are const, but not this.

> Source/WebCore/plugins/DOMMimeTypeArray.h:44
> +    Frame* frame() const { return m_navigator ? m_navigator->frame() :
nullptr; }

I assume we need these because there was a DOMWindowProperty::frame function.
But since it’s not new I can’t see the call sites in this patch. Seems like it
would be more elegant if we didn’t need these, but maybe if I saw the call
sites I would understand why we do.

> Source/WebCore/plugins/DOMPluginArray.h:47
> +    Frame* frame() const { return m_navigator ? m_navigator->frame() :
nullptr; }

Ditto.