[webkit-reviews] review granted: [Bug 181006] Element fullscreen interface should display the location : [Attachment 329888] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 181006: Element fullscreen interface should display the location
https://bugs.webkit.org/show_bug.cgi?id=181006

Attachment 329888: Patch

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




--- Comment #14 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 329888
  --> https://bugs.webkit.org/attachment.cgi?id=329888
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.