Bug 206509

Created attachment 388253 [details] Patch

Comment on attachment 388253 [details] 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.

All suggestions taken verbatum (In reply to Darin Adler from comment #2) > Comment on attachment 388253 [details] > > 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. This change is actually the entire motivating change behind this refactoring patch - It will need to be the base class in upcoming work where API is throwing both ContentWorld and UserContentWorld instances at WebUserContentControllerProxy who needs to treat them the same. > > > 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. Sounds good!

Created attachment 388307 [details] Patch

Comment on attachment 388307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388307&action=review > Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.h:45 > class UserContentWorld; Maybe take this out of this header.

(In reply to Darin Adler from comment #5) > Comment on attachment 388307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388307&action=review > > > Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.h:45 > > class UserContentWorld; > > Maybe take this out of this header. There's still lots of UserContentWorld use there *for now* Will remove in the followup work

Comment on attachment 388307 [details] Patch Clearing flags on attachment: 388307 Committed r254862: <https://trac.webkit.org/changeset/254862>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58764547>