Bug 203937

<rdar://problem/56962420>

Created attachment 383003 [details] Patch

Comment on attachment 383003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383003&action=review r=me, but I would make the patch as small as possible to make it easier to integrate quickly. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202 > + clearState(); To keep the patch small, I would just keep using resetState here. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-217 > - resetState(); Then you don't need this change. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:-357 > -void AuthenticatorManager::resetState() ... or this change. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:-95 > - void resetState(); Or this change. > Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:562 > + EXPECT_TRUE(webAuthenticationPanelFailed); Nice!

Comment on attachment 383003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383003&action=review Thanks Brent for r+ this change. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202 >> + clearState(); > > To keep the patch small, I would just keep using resetState here. I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary.

Comment on attachment 383003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383003&action=review >>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202 >>> + clearState(); >> >> To keep the patch small, I would just keep using resetState here. > > I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary. I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is.

Comment on attachment 383003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383003&action=review >>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202 >>>> + clearState(); >>> >>> To keep the patch small, I would just keep using resetState here. >> >> I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary. > > I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is. The reason to have the 'resetState' method is to avoid the m_pendingCompletionHandler check in 'clearState', and is specifically crafted for this method. Now this method will invoke m_pendingCompletionHandler as well, and therefore there is no need to have the 'resetState' method anymore. That's why I remove it.

(In reply to Jiewen Tan from comment #6) > Comment on attachment 383003 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383003&action=review > > >>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202 > >>>> + clearState(); > >>> > >>> To keep the patch small, I would just keep using resetState here. > >> > >> I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary. > > > > I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is. > > The reason to have the 'resetState' method is to avoid the > m_pendingCompletionHandler check in 'clearState', and is specifically > crafted for this method. Now this method will invoke > m_pendingCompletionHandler as well, and therefore there is no need to have > the 'resetState' method anymore. That's why I remove it. Fine. Let's just get it landed so I can try to get it in.

(In reply to Brent Fulgham from comment #7) > (In reply to Jiewen Tan from comment #6) > > Comment on attachment 383003 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=383003&action=review > > > > >>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:202 > > >>>> + clearState(); > > >>> > > >>> To keep the patch small, I would just keep using resetState here. > > >> > > >> I think this patch is already small enough for quick integration. Splitting this change into two doesn't seem necessary. > > > > > > I'm not suggesting you split the change; I just think it's unnecessary to remove the 'resetState' method. Just keep it as-is. > > > > The reason to have the 'resetState' method is to avoid the > > m_pendingCompletionHandler check in 'clearState', and is specifically > > crafted for this method. Now this method will invoke > > m_pendingCompletionHandler as well, and therefore there is no need to have > > the 'resetState' method anymore. That's why I remove it. > > Fine. Let's just get it landed so I can try to get it in. Thanks, Brent.

Committed r252248: <https://trac.webkit.org/changeset/252248>

*** Bug 204186 has been marked as a duplicate of this bug. ***