Bug 156540 – WebKit2 should allow clients to use different plug-in load policies for private browsing.

• Component: Plug-ins
• Status: RESOLVED FIXED

<rdar://problem/25637141> WebKit2 should allow clients to use different plug-in load policies for private browsing.

Created attachment 276333 [details]
Patch 1

[WebKit Commit Bot]

Attachment 276333 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:73:  The parameter name "clientPolicy" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:119:  The parameter name "clientPolicy" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/WebProcessPool.h:187:  The parameter name "policy" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/WebProcessPool.h:188:  The parameter name "policy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 276333 [details]
Patch 1

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

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1320
> +void WebProcessPool::setPluginLoadClientPolicyForBrowsingMode(bool forPrivateBrowsing, WebCore::PluginLoadClientPolicy policy, const String& host, const String& bundleIdentifier, const String& versionString)

Instead of a boolean here, use an enum class. Something like.

enum class ForPrivateBrowsing {
    No,
    Yes
};

> Source/WebKit2/WebProcess/WebProcess.h:373
> +    void setPluginLoadClientPolicyFromCreationParameters(HashMap<String, HashMap<String, HashMap<String, uint8_t>>> *, bool);

This hash map should be a const reference. Also, I think you can just call it setPluginLoadClientPolicies.

This patch fails to build on mac because I added messages in Derived Sources/WebProcessMessages.h. I should have added it to messages.in to make it auto generate.

Created attachment 276356 [details]
Patch v2

[Anders Carlsson]

Comment on attachment 276356 [details]
Patch v2

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

> Source/WebKit2/UIProcess/WebProcessPool.h:187
> +    enum class PageBrowsingMode { BrowsingModePrivate, BrowsingModeGeneral };

Please just name this

enum class PrivateBrowsing { Yes, No };

I don't think we use the term "Browsing Mode" anywhere else.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:228
> +    auto& hostsToPluginIdentifierData =  browsingMode == PageBrowsingMode::BrowsingModePrivate ? m_hostsToPluginIdentifierDataInPrivateBrowsing : m_hostsToPluginIdentifierData;

Extra spice here.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:236
> -    if (policiesByIdentifier.contains(bundleIdentifierToSet))
> -        versionsToPolicies = policiesByIdentifier.get(bundleIdentifierToSet);
> +    auto versionsToPoliciesIterator = policiesByIdentifier.find(bundleIdentifierToSet);
> +    if (versionsToPoliciesIterator != policiesByIdentifier.end())
> +        versionsToPolicies = versionsToPoliciesIterator->value;

I don't think this change is needed - it's not avoiding a hash lookup and it makes things harder to read.

Comment on attachment 276356 [details]
Patch v2

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

>> Source/WebKit2/UIProcess/WebProcessPool.h:187
>> +    enum class PageBrowsingMode { BrowsingModePrivate, BrowsingModeGeneral };
> 
> Please just name this
> 
> enum class PrivateBrowsing { Yes, No };
> 
> I don't think we use the term "Browsing Mode" anywhere else.

Sure.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:228
>> +    auto& hostsToPluginIdentifierData =  browsingMode == PageBrowsingMode::BrowsingModePrivate ? m_hostsToPluginIdentifierDataInPrivateBrowsing : m_hostsToPluginIdentifierData;
> 
> Extra spice here.

Oops. Removed.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:236
>> +        versionsToPolicies = versionsToPoliciesIterator->value;
> 
> I don't think this change is needed - it's not avoiding a hash lookup and it makes things harder to read.

Anders clairified that we don't need to use .contains beforehand; it'll just return a constructed object if it's not in there. I'll revert back to the original and get rid of .contains().

Created attachment 276451 [details]
Patch v3

[WebKit Commit Bot]

Comment on attachment 276451 [details]
Patch v3

Rejecting attachment 276451 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Jessie Berlin]

Comment on attachment 276451 [details]
Patch v3

Anders reviewed this, marking commit-queue+

[WebKit Commit Bot]

Comment on attachment 276451 [details]
Patch v3

Clearing flags on attachment: 276451

Committed r199691: <http://trac.webkit.org/changeset/199691>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.