Bug 181006 – Element fullscreen interface should display the location

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Element fullscreen interface should display the locaiton.

[Jeremy Jones]

rdar://problem/36143176

[Jeremy Jones]

Created attachment 329843 [details]
Patch

[Tim Horton]

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).

[Tim Horton]

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.

[Tim Horton]

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.

[Jeremy Jones]

Created attachment 329876 [details]
Patch

[EWS Watchlist]

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.

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 329883 [details]
Patch

[EWS Watchlist]

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.

[Jeremy Jones]

Created attachment 329888 [details]
Patch

[EWS Watchlist]

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.

[Tim Horton]

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.

[Simon Fraser (smfr)]

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.

[Jeremy Jones]

(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);

[Jeremy Jones]

(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)

[Jeremy Jones]

Created attachment 330020 [details]
Patch for landing

[EWS Watchlist]

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.

[Jeremy Jones]

Created attachment 330024 [details]
Patch for landing

[EWS Watchlist]

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.

[Jeremy Jones]

Created attachment 330025 [details]
Patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 330025 [details]
Patch for landing

Clearing flags on attachment: 330025

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