Bug 210036 – Use GlobalFrameIdentifier in NavigationAction

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rob Buis]

Use GlobalFrameIdentifier in NavigationAction rather adding yet another custom data type.

[Rob Buis]

Created attachment 395523 [details]
Patch

[Rob Buis]

Created attachment 395527 [details]
Patch

[Darin Adler]

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<>.

[Rob Buis]

Created attachment 395589 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Rob Buis]

Should I go back to RefPtr? It does seem to add a lot of complexity when adding Ref here.

[Darin Adler]

(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.

[Rob Buis]

Created attachment 395608 [details]
Patch

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[Rob Buis]

Created attachment 395656 [details]
Patch

[EWS]

Committed r259629: <https://trac.webkit.org/changeset/259629>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395656 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61381656>