<rdar://problem/58626835>

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.