Bug 206778 - Add logging to show the flow of AppSSO
Summary: Add logging to show the flow of AppSSO
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-24 15:55 PST by Jiewen Tan
Modified: 2020-01-30 18:22 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.69 KB, patch)
2020-01-24 15:58 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (6.75 KB, patch)
2020-01-24 17:06 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2020-01-24 15:55:57 PST
Add logging to show the flow of AppSSO.
Comment 1 Jiewen Tan 2020-01-24 15:56:11 PST
<rdar://problem/58626835>
Comment 2 Jiewen Tan 2020-01-24 15:58:49 PST
Created attachment 388737 [details]
Patch
Comment 3 Keith Rollin 2020-01-24 16:21:06 PST
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 4 Jiewen Tan 2020-01-24 17:01:48 PST
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.
Comment 5 Jiewen Tan 2020-01-24 17:06:14 PST
Created attachment 388745 [details]
Patch
Comment 6 Brent Fulgham 2020-01-30 16:57:50 PST
Comment on attachment 388745 [details]
Patch

r=me
Comment 7 Jiewen Tan 2020-01-30 17:08:35 PST
Comment on attachment 388745 [details]
Patch

Thanks, Brent.
Comment 8 WebKit Commit Bot 2020-01-30 18:04:19 PST
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.
Comment 9 WebKit Commit Bot 2020-01-30 18:04:40 PST
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 10 WebKit Commit Bot 2020-01-30 18:22:30 PST
Comment on attachment 388745 [details]
Patch

Clearing flags on attachment: 388745

Committed r255480: <https://trac.webkit.org/changeset/255480>
Comment 11 WebKit Commit Bot 2020-01-30 18:22:32 PST
All reviewed patches have been landed.  Closing bug.