Bug 181006

Summary: Element fullscreen interface should display the location
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: WebKit2Assignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, fred.wang, jer.noble, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review+, simon.fraser: commit-queue-
Patch for landing
none
Patch for landing
none
Patch for landing none

Description Jeremy Jones 2017-12-19 15:41:35 PST 
Element fullscreen interface should display the locaiton.
Comment 1 Jeremy Jones 2017-12-19 15:43:21 PST 
rdar://problem/36143176
Comment 2 Jeremy Jones 2017-12-19 15:49:04 PST 
Created attachment 329843 [details]
Patch
Comment 3 Tim Horton 2017-12-19 16:01:14 PST 
Comment on attachment 329843 [details]
Patch

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

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1273
> +NSString *simplifiedUserVisibleString(NSURL *url)

I am somewhat surprised to see this code end up in WebKit! We should probably have a discussion.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1336
> +        result = [NSString stringWithFormat:@"%C%@", WBSLeftToRightMarkCharacter, [result substringFromIndex:lengthOfPrefixToSuppress]];

Surely this won’t work (the WBS thing).
Comment 4 Tim Horton 2017-12-19 16:01:58 PST 
Comment on attachment 329843 [details]
Patch

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

>> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1273
>> +NSString *simplifiedUserVisibleString(NSURL *url)
> 
> I am somewhat surprised to see this code end up in WebKit! We should probably have a discussion.

Also, you should note that other parts of WebKit (Drag and Drop) use LinkPresentation for this.
Comment 5 Tim Horton 2017-12-19 16:42:59 PST 
Comment on attachment 329843 [details]
Patch

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

>>> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1273
>>> +NSString *simplifiedUserVisibleString(NSURL *url)
>> 
>> I am somewhat surprised to see this code end up in WebKit! We should probably have a discussion.
> 
> Also, you should note that other parts of WebKit (Drag and Drop) use LinkPresentation for this.

Actually, just r- for propagating this code to a third place.
Comment 6 Jeremy Jones 2017-12-19 19:23:57 PST 
Created attachment 329876 [details]
Patch
Comment 7 EWS Watchlist 2017-12-19 19:27:12 PST 
Attachment 329876 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:221:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Jeremy Jones 2017-12-19 19:49:50 PST 
(In reply to Tim Horton from comment #5)
> Comment on attachment 329843 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329843&action=review
> 
> >>> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:1273
> >>> +NSString *simplifiedUserVisibleString(NSURL *url)
> >> 
> >> I am somewhat surprised to see this code end up in WebKit! We should probably have a discussion.
> > 
> > Also, you should note that other parts of WebKit (Drag and Drop) use LinkPresentation for this.
> 
> Actually, just r- for propagating this code to a third place.

Deleted the redundant code and switched to link presentation.
Comment 9 Jeremy Jones 2017-12-19 20:00:21 PST 
Created attachment 329883 [details]
Patch
Comment 10 EWS Watchlist 2017-12-19 20:03:26 PST 
Attachment 329883 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:221:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Jeremy Jones 2017-12-19 20:34:39 PST 
Created attachment 329888 [details]
Patch
Comment 12 EWS Watchlist 2017-12-19 20:36:16 PST 
Attachment 329888 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:220:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Tim Horton 2017-12-20 10:46:27 PST 
Comment on attachment 329888 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/LinkPresentationSPI.h:40
> +#endif // (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100300)

Do we really still care about < 10.3?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:594
> +    NSString *domain = [url _lp_simplifiedDisplayString];

You need to make this more conditional, like the other place we use it, because it’s missing in the base system (working on it). For now just fall back to the non-simplified version.
Comment 14 Simon Fraser (smfr) 2017-12-20 11:11:33 PST 
Comment on attachment 329888 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:242
> +        RetainPtr<UIImage> lockImage = [UIImage imageNamed:lockImageName inBundle:bundle compatibleWithTraitCollection:nil];

The RetainPtr<UIImage> here just adds extra ref churn. The result of -imageNamed: is autoreleased, so you can just use a raw pointer.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:244
> +        [[_locationButton imageView] setTintColor:trustedSite?greenTint:whiteTint];

Spaces around the ? and :

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:268
> +    [_cancelButton setTitle:@"Done" forState:UIControlStateNormal];

Why is this not localized?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:289
> +    [bottom setPriority:UILayoutPriorityRequired-1];

Spaces around the -

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:346
> +    [UIView animateWithDuration:0.1 animations:^{

Maybe this animation duration should be in a static const.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:362
> +    [self performSelector:@selector(hideCancelButton) withObject:nil afterDelay:4.0];

Same with the delay.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:490
> +    RetainPtr<NSString> _EVOrganizationName;
> +    BOOL _EVOrganizationNameIsValid;

Is _EVOrganizationNameIsValid just an alias for having a non-null _EVOrganizationName? Or does it really mean "tried to look up _EVOrganizationName"?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:562
> +    NSDictionary *infoDictionary = CFBridgingRelease(SecTrustCopyInfo(trust));

Is CFBridgingRelease the right thing to do in a non-ARC world?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:597
> +    if ([[url absoluteString] _web_hasCaseInsensitivePrefix:@"data:"])

This should ask the url for the protocol.
Comment 15 Jeremy Jones 2017-12-21 00:26:50 PST 
(In reply to Tim Horton from comment #13)
> Comment on attachment 329888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329888&action=review
> 
> > Source/WebCore/PAL/pal/spi/cocoa/LinkPresentationSPI.h:40
> > +#endif // (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100300)
> 
> Do we really still care about < 10.3?

EWS still builds against 10.x. I'll try getting rid the version check.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:594
> > +    NSString *domain = [url _lp_simplifiedDisplayString];
> 
> You need to make this more conditional, like the other place we use it,
> because it’s missing in the base system (working on it). For now just fall
> back to the non-simplified version.

    if (LinkPresentationLibrary())
        domain = [url _lp_simplifiedDisplayString];
    else
        domain = userVisibleString(url);
Comment 16 Jeremy Jones 2017-12-21 00:53:59 PST 
(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 329888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329888&action=review
> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:242
> > +        RetainPtr<UIImage> lockImage = [UIImage imageNamed:lockImageName inBundle:bundle compatibleWithTraitCollection:nil];
> 
> The RetainPtr<UIImage> here just adds extra ref churn. The result of
> -imageNamed: is autoreleased, so you can just use a raw pointer.

Done.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:244
> > +        [[_locationButton imageView] setTintColor:trustedSite?greenTint:whiteTint];
> 
> Spaces around the ? and :

Done. 3x

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:268
> > +    [_cancelButton setTitle:@"Done" forState:UIControlStateNormal];
> 
> Why is this not localized?

Localized:
WEB_UI_STRING("Done", "Text of button that exits element fullscreen.")

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:289
> > +    [bottom setPriority:UILayoutPriorityRequired-1];
> 
> Spaces around the -

Done.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:346
> > +    [UIView animateWithDuration:0.1 animations:^{
> 
> Maybe this animation duration should be in a static const.

Done.
static const NSTimeInterval showHideAnimationDuration = 0.1;

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:362
> > +    [self performSelector:@selector(hideCancelButton) withObject:nil afterDelay:4.0];
> 
> Same with the delay.

Done.
static const NSTimeInterval autoHideDelay = 4.0;

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:490
> > +    RetainPtr<NSString> _EVOrganizationName;
> > +    BOOL _EVOrganizationNameIsValid;
> 
> Is _EVOrganizationNameIsValid just an alias for having a non-null
> _EVOrganizationName? Or does it really mean "tried to look up
> _EVOrganizationName"?

It means "tried to look it up, so don't look-up again".

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:562
> > +    NSDictionary *infoDictionary = CFBridgingRelease(SecTrustCopyInfo(trust));
> 
> Is CFBridgingRelease the right thing to do in a non-ARC world?

Fixed:
[(__bridge NSDictionary *)SecTrustCopyInfo(trust) autorelease]

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:597
> > +    if ([[url absoluteString] _web_hasCaseInsensitivePrefix:@"data:"])
> 
> This should ask the url for the protocol.

Done:
    if ([[url scheme] caseInsensitiveCompare:@"data"] == NSOrderedSame)
Comment 17 Jeremy Jones 2017-12-21 01:09:15 PST 
Created attachment 330020 [details]
Patch for landing
Comment 18 EWS Watchlist 2017-12-21 01:11:16 PST 
Attachment 330020 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:227:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Jeremy Jones 2017-12-21 01:37:24 PST 
Created attachment 330024 [details]
Patch for landing
Comment 20 EWS Watchlist 2017-12-21 01:38:57 PST 
Attachment 330024 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:227:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Jeremy Jones 2017-12-21 02:18:17 PST 
Created attachment 330025 [details]
Patch for landing
Comment 22 EWS Watchlist 2017-12-21 02:21:00 PST 
Attachment 330025 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:227:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 WebKit Commit Bot 2017-12-21 02:57:03 PST 
Comment on attachment 330025 [details]
Patch for landing

Clearing flags on attachment: 330025

Committed r226218: <https://trac.webkit.org/changeset/226218>