[Webkit-unassigned] [Bug 137302] location.href is not updated on a redirect with a custom protocol

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137302

Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #239608|review?                     |review-
              Flags|                            |

--- Comment #7 from Benjamin Poulain <benjamin at webkit.org> ---
Comment on attachment 239608
  --> https://bugs.webkit.org/attachment.cgi?id=239608
Patch

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

Round1: Quick review, mostly style.

> Source/WebCore/ChangeLog:3
> +        Allow non-HTTP or invalid URLs to be passed to findDefaultProtectionSpaceForURL

You should keep the title in sync with the bug report on bugzilla.

This line of explanation should go under the "Reviewed by" line.

> Source/WebCore/ChangeLog:12
> +        No new tests, no behavior change.

You can only use this excuse for tests when you believe there is no observable difference after your change.

In this case, we would crash in Debug due to the assertion so it is observable and should have a test.

> Source/WebKit2/ChangeLog:3
> +        Add CustomProtocolManager::DidRedirect message

Keep the title "location.href is not updated on a redirect with a custom protocol" and have the explanation below.

> Source/WebKit2/ChangeLog:9
> +        The message is received after an HTTP redirect,
> +        and is forwarded to the NSURLProtocol client.

Here you should have an explanation of the bug, why it happens, and how you solved it.

> Source/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:112
> +    // not a redirect, just continue

Comments should be complete sentences in WebKit, starting with uppercase for the first letter, and ending with a period.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:60
> +    NSMutableURLRequest *request = [self.request mutableCopy];
> +    RetainPtr<NSMutableURLRequest> requestPtr = adoptNS(request);

Let's merge those two lines.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:64
> +    NSURLResponse *response = [[NSHTTPURLResponse alloc] initWithURL:targetURL
> +        statusCode:302 HTTPVersion:@"HTTP/1.0" headerFields:@{ @"Location" : targetURL.absoluteString }];

Objective-C multiline alignment is a bit weird. You should split this with one line per argument and get XCode to align it for you.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141025/a23b0f85/attachment-0002.html>