[webkit-reviews] review granted: [Bug 206509] API::(User)ContentWorld cleanup : [Attachment 388253] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 20 16:12:39 PST 2020


Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 206509: API::(User)ContentWorld cleanup
https://bugs.webkit.org/show_bug.cgi?id=206509

Attachment 388253: Patch

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




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

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

> Source/WebKit/UIProcess/API/APIContentWorld.cpp:73
> +    : ContentWorldBase(API::UserContentWorld::generateIdentifier(), name)

Can write this more simply:

    : ContentWorldBase(generateIdentifier(), name)

Same applies to other calls to generateIdentifier() in other class members.

> Source/WebKit/UIProcess/API/APIContentWorld.h:51
> +    ContentWorldBase(uint64_t identifier, const WTF::String& name)
> +	   : m_identifier(identifier)
> +	   , m_name(name)
> +    {
> +    }

I suggest considering a constructor that takes just the name and generates the
identifier. Could be used by both derived classes, each of which has a
constructor that wants to do the same thing.

Similarly, could consider having this default the name to null string so the
caller doesn’t have to explicitly say { }, or maybe don’t provide one that lets
you specify both name and identifier, since I don’t think any caller needs
that.

> Source/WebKit/UIProcess/API/APIContentWorld.h:68
> +    void ref() const {
API::ObjectImpl<API::Object::Type::ContentWorld>::ref(); }
> +    void deref() const {
API::ObjectImpl<API::Object::Type::ContentWorld>::deref(); }

I suggest this instead:

    void ref() const final { ObjectImpl::ref(); }
    void deref() const final { ObjectImpl::deref(); }

They should be marked final to emphasize they are intended to override, and
give an error if they don’t. They don’t need to name the entire base template
class. Just the name ObjectImpl should work.

> Source/WebKit/UIProcess/API/APIContentWorld.h:72
>      ContentWorld(const WTF::String&);
>      ContentWorld(uint64_t identifier);

These should be marked explicit.

> Source/WebKit/UIProcess/API/APIUserContentWorld.cpp:45
> +    : ContentWorldBase(ContentWorldBase::generateIdentifier(), name)

Can write this more simply:

    : ContentWorldBase(generateIdentifier(), name)

> Source/WebKit/UIProcess/API/APIUserContentWorld.h:41
> +    void ref() const {
API::ObjectImpl<API::Object::Type::UserContentWorld>::ref(); }
> +    void deref() const {
API::ObjectImpl<API::Object::Type::UserContentWorld>::deref(); }

Same as above. I suggest:

    void ref() const final { ObjectImpl::ref(); }
    void deref() const final { ObjectImpl::deref(); }

> Source/WebKit/UIProcess/API/APIUserContentWorld.h:44
>      UserContentWorld(const WTF::String&);

Should be marked explicit.

> Source/WebKit/UIProcess/API/APIUserContentWorld.h:47
>      UserContentWorld(ForNormalWorldOnly);

Should be marked explicit.

> Source/WebKit/UIProcess/UserContent/WebScriptMessageHandler.h:27
>  #ifndef WebScriptMessageHandler_h
>  #define WebScriptMessageHandler_h

Noticed you moved to #pragma once on the other header. I suggest doing that
here too.

> Source/WebKit/UIProcess/UserContent/WebScriptMessageHandler.h:64
> +    API::ContentWorldBase& world() { return m_world.get(); }

Not sure why this is returning a base class reference; if this becomes a
virtual function in the future that might make sense, or if the concrete
classes are private. For now it mainly will just un-optimize virtual functions
we might call, making them dynamic.

> Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.cpp:167
>      for (unsigned i = 0; i < numberOfUsesToRemove; ++i) {

Patterns like this where we use a HashCountedSet, but need a loop to subtract
multiple times in a row, are intrinsically inefficient, with O(n) hash table
lookups for no good reason. We should return to this kind of code (here and
elsewhere in WebKit) and add a removeMultiple function to HashCountedSet
instead of looping and calling remove.


More information about the webkit-reviews mailing list