Bug 208703

Summary: [WebAuthn] Customize a bit more on the macOS LocalAuthentication prompt
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebKit Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, darin, jiewen_tan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
none
Patch
darin: review+
Patch for landing
jiewen_tan: commit-queue-
Patch for landing
none
Part 2
bfulgham: review+
Part 2
none
Part 2
none
Part 2
bfulgham: review+
Patch for landing none

Description Jiewen Tan 2020-03-06 02:36:00 PST
Customize a bit more on the macOS LocalAuthentication prompt.
Comment 1 Radar WebKit Bug Importer 2020-03-06 02:36:30 PST
<rdar://problem/60136974>
Comment 2 Jiewen Tan 2020-03-06 02:43:14 PST
Created attachment 392696 [details]
Patch
Comment 3 Jiewen Tan 2020-03-06 14:07:08 PST
Created attachment 392771 [details]
Patch
Comment 4 Darin Adler 2020-03-08 23:14:36 PDT
Comment on attachment 392771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392771&action=review

> Source/WebCore/en.lproj/Localizable.strings:887
> +"Touch ID to allow signning into â%@â with Touch ID." = "Touch ID to allow signning into â%@â with Touch ID.";

Spelling error here in the word "signing"

> Source/WebCore/platform/LocalizedStrings.cpp:1218
> +    return formatLocalizedString(WEB_UI_CFSTRING("Touch ID to allow signning into â%@â with Touch ID.", "Allow using Touch ID to sign into the specified website on this device"), rpId.createCFString().get());

Ditto.

> Source/WebCore/platform/LocalizedStrings.h:344
> +    WEBCORE_EXPORT String makeCredentialTouchIDPromptTitle(const String& rpId);

Do we really have to call this "rpId"? Are there words we could use instead?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:52
> +#elif PLATFORM(MAC)

I think we should just use #else here.
Comment 5 Jiewen Tan 2020-03-13 14:51:08 PDT
Comment on attachment 392771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392771&action=review

Thanks Darin for the r+!

>> Source/WebCore/en.lproj/Localizable.strings:887
>> +"Touch ID to allow signning into â%@â with Touch ID." = "Touch ID to allow signning into â%@â with Touch ID.";
> 
> Spelling error here in the word "signing"

Fixed.

>> Source/WebCore/platform/LocalizedStrings.cpp:1218
>> +    return formatLocalizedString(WEB_UI_CFSTRING("Touch ID to allow signning into â%@â with Touch ID.", "Allow using Touch ID to sign into the specified website on this device"), rpId.createCFString().get());
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/LocalizedStrings.h:344
>> +    WEBCORE_EXPORT String makeCredentialTouchIDPromptTitle(const String& rpId);
> 
> Do we really have to call this "rpId"? Are there words we could use instead?

We could call it domain.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:52
>> +#elif PLATFORM(MAC)
> 
> I think we should just use #else here.

Fixed.
Comment 6 Jiewen Tan 2020-03-13 15:14:07 PDT
Created attachment 393535 [details]
Patch for landing
Comment 7 Jiewen Tan 2020-03-13 15:26:43 PDT
Comment on attachment 393535 [details]
Patch for landing

Always be tricked by the fact that webkit-patch upload will bundle any local changes into one patch.
Comment 8 Jiewen Tan 2020-03-13 15:27:57 PDT
Created attachment 393537 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2020-03-13 16:15:36 PDT
Comment on attachment 393537 [details]
Patch for landing

Clearing flags on attachment: 393537

Committed r258442: <https://trac.webkit.org/changeset/258442>
Comment 10 Jiewen Tan 2020-03-16 14:54:03 PDT
Created attachment 393687 [details]
Part 2
Comment 11 Brent Fulgham 2020-03-19 17:53:28 PDT
Comment on attachment 393687 [details]
Part 2

r=me
Comment 12 Jiewen Tan 2020-03-20 00:22:20 PDT
Created attachment 394070 [details]
Part 2
Comment 13 Jiewen Tan 2020-03-20 00:23:30 PDT
(In reply to Brent Fulgham from comment #11)
> Comment on attachment 393687 [details]
> Part 2
> 
> r=me

Thanks for the review. I just upload a newer version which addresses comments from our Tuesday meeting.
Comment 14 Darin Adler 2020-03-22 18:33:31 PDT
Comment on attachment 394070 [details]
Part 2

View in context: https://bugs.webkit.org/attachment.cgi?id=394070&action=review

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:55
> +    NSBundle *mainBundle = [NSBundle mainBundle];
> +    bundleName = appBundle.infoDictionary[(id)_kCFBundleDisplayNameKey];
> +    if (!bundleName)
> +        bundleName = appBundle.infoDictionary[(id)kCFBundleNameKey];
> +    if (!bundleName)
> +        bundleName = [mainBundle bundleIdentifier];

This code is not compiling on iOS bots
Comment 15 Jiewen Tan 2020-03-22 19:22:26 PDT
(In reply to Darin Adler from comment #14)
> Comment on attachment 394070 [details]
> Part 2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394070&action=review
> 
> > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:55
> > +    NSBundle *mainBundle = [NSBundle mainBundle];
> > +    bundleName = appBundle.infoDictionary[(id)_kCFBundleDisplayNameKey];
> > +    if (!bundleName)
> > +        bundleName = appBundle.infoDictionary[(id)kCFBundleNameKey];
> > +    if (!bundleName)
> > +        bundleName = [mainBundle bundleIdentifier];
> 
> This code is not compiling on iOS bots

I fixed that locally but was struggling to get an iOS device to run. Will upload a new patch soon once I'm done testing on my iOS device.
Comment 16 Jiewen Tan 2020-03-22 20:56:51 PDT
Created attachment 394239 [details]
Part 2
Comment 17 Jiewen Tan 2020-03-22 21:01:53 PDT
Created attachment 394240 [details]
Part 2
Comment 18 Brent Fulgham 2020-03-24 16:01:05 PDT
Comment on attachment 394240 [details]
Part 2

View in context: https://bugs.webkit.org/attachment.cgi?id=394240&action=review

R=me

> Source/WebKit/ChangeLog:12
> +        And tunes a bit on the makeCredential string.

It also polishes the text used for makeCredential.

> Source/WebKit/ChangeLog:14
> +        Besides that, it also enhances the iOS titles as well.

Besides that, it also enhances the iOS title strings.
Comment 19 Jiewen Tan 2020-03-24 16:09:01 PDT
Comment on attachment 394240 [details]
Part 2

View in context: https://bugs.webkit.org/attachment.cgi?id=394240&action=review

Thanks Brent for the r+.

>> Source/WebKit/ChangeLog:12
>> +        And tunes a bit on the makeCredential string.
> 
> It also polishes the text used for makeCredential.

Fixed.

>> Source/WebKit/ChangeLog:14
>> +        Besides that, it also enhances the iOS titles as well.
> 
> Besides that, it also enhances the iOS title strings.

Fixed.
Comment 20 Jiewen Tan 2020-03-24 16:11:39 PDT
Created attachment 394434 [details]
Patch for landing
Comment 21 EWS 2020-03-24 17:03:26 PDT
Committed r258961: <https://trac.webkit.org/changeset/258961>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394434 [details].