[webkit-reviews] review granted: [Bug 205288] Add run-time flag for in-app browser privacy : [Attachment 385905] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 205288: Add run-time flag for in-app browser privacy
https://bugs.webkit.org/show_bug.cgi?id=205288

Attachment 385905: Patch

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




--- Comment #3 from John Wilander <wilander at apple.com> ---
Comment on attachment 385905
  --> https://bugs.webkit.org/attachment.cgi?id=385905
Patch

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

r+ with comments.

I see you've put your setting next to Ad Click Attribution. Since they aren't
really related, you don't have to. Please look at the change logs for these
files to see where people add new settings unrelated to anything existing, and
add yours accordingly. I assume it's append-style.

> Source/WebCore/page/RuntimeEnabledFeatures.h:156
> +    bool inAppBrowserPrivacyEnabled() const { return
m_inAppBrowserPrivacyEnabled; }

I know we're not perfect in this regard for these kind of settings, but we
should try to use the "is" prefix for getters of booleans.

> Source/WebKit/Shared/WebPreferences.yaml:1536
> +    humanReadableName: "In App Browser Privacy"

Needs a dash in "In-App."

> Source/WebKit/Shared/WebPreferences.yaml:1537
> +    humanReadableDescription: "Enable In App Browser Privacy"

Ditto.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3526
> +- (BOOL)inAppBrowserPrivacyEnabled

Similar for an "is" prefix.

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:615
> +- (BOOL)inAppBrowserPrivacyEnabled;

Ditto.