[webkit-reviews] review granted: [Bug 210036] Use GlobalFrameIdentifier in NavigationAction : [Attachment 395527] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 210036: Use GlobalFrameIdentifier in NavigationAction
https://bugs.webkit.org/show_bug.cgi?id=210036

Attachment 395527: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 395527
  --> https://bugs.webkit.org/attachment.cgi?id=395527
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(PageIdentif
ier { }), 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<>.