| Summary: | [WebAuthn] Return NotAllowedError immediately for UI cancellations | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||
| Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | alex.gaynor, bfulgham, jiewen_tan, kieun.shin, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 181943 | ||||||
| Attachments: |
|
||||||
|
Description
Jiewen Tan
2019-11-06 18:40:53 PST
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. *** |