[webkit-reviews] review granted: [Bug 200150] NavigationSOAuthorizationSession should check the active URL of the responding page after waking up from waiting : [Attachment 374933] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 200150: NavigationSOAuthorizationSession should check the active URL of the
responding page after waking up from waiting
https://bugs.webkit.org/show_bug.cgi?id=200150

Attachment 374933: Patch

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




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 374933
  --> https://bugs.webkit.org/attachment.cgi?id=374933
Patch

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

>
Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.
mm:70
> +    // When the current main frame URL is different from the before one,

I think it might be simpler to say:

"// When the current main frame URL has changed from the start of
authentication, the SOAuthorization is no longer valid."

Even better would be to make a method:

bool
NavigationSOAuthorizationSession::mainFrameURLDidChangeDuringAuthentication()
{
    auto* page = this->page();
    return !page || page->pageLoadState().activeURL() !=
m_waitingPageActiveURL;
}

>
Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.
mm:72
> +    if (page->pageLoadState().activeURL() != m_waitingPageActiveURL) {

Then this just becomes:

if (mainFrameURLDidChangeDuringAuthentication()) {
    abort();
...

And you don't need a comment. :-)

>
Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm
:-79
> -    ASSERT_NOT_REACHED();

Why is it okay to remove this assertion? Is it because we now expect to abort
if the URL changed during authentication?