Summary: | Add logging to show the flow of AppSSO | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, jiewen_tan, krollin, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jiewen Tan
2020-01-24 15:55:57 PST
Created attachment 388737 [details]
Patch
Comment on attachment 388737 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388737&action=review > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:148 > return; I'm wondering if early returns statements like these are interesting to log. Or if continuing past them is something that's interesting to log. Any time there's a change in control flow, you might want to later know what happened and why. > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:215 > + RELEASE_LOG_IF_ALLOWED("complete: (httpStatusCode: %d, hasCookies: 1, hasData: %d)", response.httpStatusCode(), !!data.length); You could consolidate this logging line with the one above, moving the consolidated line after the toCookieVector call, and passing in the result of cookies.isEmpty() to a "hasCookies: %d" format specifier. Also, the format I've been standardizing on uses equals signs with no spaces around them "... (httpStatusCode=%d, hasCookies=%d, hasData=%d)". > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:238 > + RELEASE_LOG_IF_ALLOWED("presentViewController:"); This logging statement is not at the top of this function. Is this intended? Knowing that you entered a function even if you exit early can be handy. Comment on attachment 388737 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388737&action=review Thanks Keith for reviewing my patch. >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:148 >> return; > > I'm wondering if early returns statements like these are interesting to log. Or if continuing past them is something that's interesting to log. Any time there's a change in control flow, you might want to later know what happened and why. Thanks for your tips. This check is a cautious check over pointers. The early return is probably never going to happen. Maybe I should use UNLIKELY(). >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:215 >> + RELEASE_LOG_IF_ALLOWED("complete: (httpStatusCode: %d, hasCookies: 1, hasData: %d)", response.httpStatusCode(), !!data.length); > > You could consolidate this logging line with the one above, moving the consolidated line after the toCookieVector call, and passing in the result of cookies.isEmpty() to a "hasCookies: %d" format specifier. > > Also, the format I've been standardizing on uses equals signs with no spaces around them "... (httpStatusCode=%d, hasCookies=%d, hasData=%d)". Good point! >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:238 >> + RELEASE_LOG_IF_ALLOWED("presentViewController:"); > > This logging statement is not at the top of this function. Is this intended? Knowing that you entered a function even if you exit early can be handy. I should move this above. Created attachment 388745 [details]
Patch
Comment on attachment 388745 [details]
Patch
r=me
Comment on attachment 388745 [details]
Patch
Thanks, Brent.
The commit-queue encountered the following flaky tests while processing attachment 388745 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch. The commit-queue encountered the following flaky tests while processing attachment 388745 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 388745 [details] Patch Clearing flags on attachment: 388745 Committed r255480: <https://trac.webkit.org/changeset/255480> All reviewed patches have been landed. Closing bug. |