[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 bugzilla-daemon at webkit.org
Mon Aug 5 13:31:55 PDT 2019

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

Attachment 375543: Patch


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

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

    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()));


> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4686
> +		   $rootString	= "    auto* root =


> 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; }


More information about the webkit-reviews mailing list