Bug 217869 – Minor additions to minibrowser

• Status: NEW
• Component: New Bugs

Minor additions to minibrowser

Created attachment 411655 [details]
Patch

[Alexey Proskuryakov]

*** Bug 217868 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 411655 [details]
Patch

Need to say why.

When testing using the minibrowser certain https sites don't work, so this is an attempt to fix those issues so if and when someone does have issues with a https site while testing the engine they can try using this setting. I think someone made a point about something very similar in the dev thread on the slack channel a while back.

thanks

[Darin Adler]

Comment on attachment 411655 [details]
Patch

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

> Tools/MiniBrowser/mac/SettingsController.h:62
> +@property (nonatomic, readonly) BOOL useInvalidHTTPSWebsites;

I would call this "allow", not "use".

> Tools/MiniBrowser/mac/SettingsController.m:178
> +    [self _addItemWithTitle:@"Enable accessing sites with an invalid HTTPS configuration" action:@selector(toggleUseInvalidHTTPSWebsites:) indented:YES];

This needs a shorter title. I think "Allow Invalid HTTPS" would be a good title.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:795
> +    SettingsController *settingsController = [[NSApplication sharedApplication] browserAppDelegate].settingsController;

No need to mix styles here. Could just write NSApplication.sharedApplication.browserAppDelegate.settingsController.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:797
> +    if ([challenge protectionSpace].serverTrust
> +        && settingsController.useInvalidHTTPSWebsites) {

WebKit coding style would put this on one line, not broken like this.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:800
> +        NSURLCredential *httpsCred =
> +        [[NSURLCredential alloc] initWithTrust:[challenge protectionSpace].serverTrust];

This is not WebKit project style formatting, breaking the line like this. I suggest doing it in one line. I think credential is a fine name for this local variable, fitting more with WebKit coding style.

This object is being leaked here. This file does not get compiled with ARC, so we need to call release on httpsCred before returning.

(In reply to Darin Adler from comment #5)
> Comment on attachment 411655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411655&action=review
> 
> > Tools/MiniBrowser/mac/SettingsController.h:62
> > +@property (nonatomic, readonly) BOOL useInvalidHTTPSWebsites;
> 
> I would call this "allow", not "use".
> 
> > Tools/MiniBrowser/mac/SettingsController.m:178
> > +    [self _addItemWithTitle:@"Enable accessing sites with an invalid HTTPS configuration" action:@selector(toggleUseInvalidHTTPSWebsites:) indented:YES];
> 
> This needs a shorter title. I think "Allow Invalid HTTPS" would be a good
> title.
> 
Done.
> > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:795
> > +    SettingsController *settingsController = [[NSApplication sharedApplication] browserAppDelegate].settingsController;
> 
> No need to mix styles here. Could just write
> NSApplication.sharedApplication.browserAppDelegate.settingsController.
> 
> > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:797
> > +    if ([challenge protectionSpace].serverTrust
> > +        && settingsController.useInvalidHTTPSWebsites) {
> 
> WebKit coding style would put this on one line, not broken like this.

done.

> 
> > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:800
> > +        NSURLCredential *httpsCred =
> > +        [[NSURLCredential alloc] initWithTrust:[challenge protectionSpace].serverTrust];
> 
> This is not WebKit project style formatting, breaking the line like this. I
> suggest doing it in one line. I think credential is a fine name for this
> local variable, fitting more with WebKit coding style.
> 

Will do. I did skim through the code style guide, but it is clear that I would need to go through it in depth before my next patch.

> This object is being leaked here. This file does not get compiled with ARC,
> so we need to call release on httpsCred before returning.

My obj-c is rusty.  Will fix. Sorry about that.

Note: I didn't implement this for the Webkit1 viewer. Should I add this option to the webkit2-only settings menu, try to implement it for the Webkit1 viewer, or just leave as is?

thanks.

Created attachment 411719 [details]
new patch

Never mind on my latest comment, when I was coding it I forgot that I put it in Webkit2 only settings already.  
Sorry for the inconvenience. 

Submitted relevant patch

[Darin Adler]

Comment on attachment 411719 [details]
new patch

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

> Tools/MiniBrowser/mac/SettingsController.m:717
>  }
> -
> +- (void)toggleAllowInvalidHTTPSWebsites:(id)sender
> +{
> +    [self _toggleBooleanDefault:AllowInvalidHTTPSWebsitesKey];
> +}
> +- (BOOL)allowInvalidHTTPSWebsites
> +{
> +    return [[NSUserDefaults standardUserDefaults] boolForKey:AllowInvalidHTTPSWebsitesKey];
> +}
>  - (BOOL)nonFastScrollableRegionOverlayVisible

We’re leaving blank lines between the methods here, so I suggest the new code follow suit.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:114
> +    SettingsController *settingsController = NSApplication.sharedApplication.browserAppDelegate.settingsController;

Funny that you made this change here, but not in your own new code ;)

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:796
> +    if ([challenge protectionSpace].serverTrust && settingsController.allowInvalidHTTPSWebsites) {

Could write challenge.protectionSpace.serverTrust.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:797
> +        // and if self signed enabled

I don’t understand this comment.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:798
> +        NSURLCredential *credential = [[NSURLCredential alloc] initWithTrust:[challenge protectionSpace].serverTrust];

Could write challenge.protectionSpace.serverTrust.

Created attachment 411721 [details]
new patch (2)

I implemented your suggestions and made another cold hard look at the patch.

In addition, I made the Webkit2 comment comply with the Webkit Style Guide with using a full sentence, and removed the self signed comment as it was simply for marking the spot where I would put the code in while coding it.

[Radar WebKit Bug Importer]

<rdar://problem/70583395>