Use GlobalFrameIdentifier in NavigationAction rather adding yet another custom data type.
Created attachment 395523 [details] Patch
Created attachment 395527 [details] Patch
Comment on attachment 395527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395527&action=review Some comments, not things you really must do, but all a good idea. > Source/WebCore/loader/NavigationAction.cpp:48 > - , m_pageIDAndFrameIDPair { document.frame() ? std::make_pair(document.frame()->loader().client().pageID().valueOr(PageIdentifier { }), document.frame()->loader().client().frameID().valueOr(FrameIdentifier { })) : std::make_pair<PageIdentifier, FrameIdentifier>({ }, { }) } > { > + if (document.frame()) { > + m_globalFrameIdentifier.pageID = document.frame()->loader().client().pageID().valueOr(PageIdentifier { }); > + m_globalFrameIdentifier.frameID = document.frame()->loader().client().frameID().valueOr(FrameIdentifier { }); > + } Would be nice to do this with construction, not assignment. Could make a helper function or use a lambda. Would also be nice to refactor not to repeat document.frame() and document.frame()->loader().client(). Should also investigate if we can just pass { } to valueOr without repeating the type. > Source/WebCore/loader/NavigationAction.h:67 > class Requester { For future refinement: This seems much more like a struct than a class to me. As long as we pass a const Requester around, that protects us against things being changed. And then there’s no need to have both function and data members for each thing. > Source/WebCore/loader/NavigationAction.h:74 > + PageIdentifier pageID() const { return m_globalFrameIdentifier.pageID; } > + FrameIdentifier frameID() const { return m_globalFrameIdentifier.frameID; } Consider eliminating these getters and having a GlobalFrameIdentifier getter instead. Unless it makes the code significantly uglier at the call sites. > Source/WebCore/loader/NavigationAction.h:77 > RefPtr<SecurityOrigin> m_origin; This should probably be Ref<> instead of RefPtr<>.
Created attachment 395589 [details] Patch
Comment on attachment 395589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395589&action=review > Source/WebCore/loader/NavigationAction.h:71 > + WEBCORE_EXPORT Requester(const Requester&); > + Requester& operator=(const Requester&); Would also be nice to add the move constructor and assignment operator for efficiency, otherwise we will copy even when in move contexts. Requester(Requester&&) = default; Requester& operator=(Requester&&) = default; > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:948 > + if (const auto& pageID = requester.globalFrameIdentifier().pageID) { I wouldn’t have bothered with the const here.
Comment on attachment 395589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395589&action=review > Source/WebCore/loader/NavigationAction.cpp:68 > +NavigationAction::Requester::Requester(const Requester& other) > + : m_url(other.m_url) > + , m_origin(other.m_origin.copyRef()) > + , m_globalFrameIdentifier(other.m_globalFrameIdentifier) > +{ > +} > + > +NavigationAction::Requester& NavigationAction::Requester::operator=(const Requester& other) > { > + m_url = other.m_url; > + m_origin = other.m_origin.copyRef(); > + m_globalFrameIdentifier = other.m_globalFrameIdentifier; > + return *this; > } Looks like making this use Ref ruined everything! The fact that you can’t copy a Ref is starting to be more annoying than helpful.
Should I go back to RefPtr? It does seem to add a lot of complexity when adding Ref here.
(In reply to Rob Buis from comment #7) > Should I go back to RefPtr? It does seem to add a lot of complexity when > adding Ref here. I think so.
Created attachment 395608 [details] Patch
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Created attachment 395656 [details] Patch
Committed r259629: <https://trac.webkit.org/changeset/259629> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395656 [details].
<rdar://problem/61381656>