[webkit-reviews] review denied: [Bug 180819] Avoid waking plugin process up unnecessarily : [Attachment 329377] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 180819: Avoid waking plugin process up unnecessarily
https://bugs.webkit.org/show_bug.cgi?id=180819

Attachment 329377: Patch

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




--- Comment #7 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 329377
  --> https://bugs.webkit.org/attachment.cgi?id=329377
Patch

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

r- because I think the count thing is wrong.

The rest of my comments are aesthetic preferences, but they probably don't make
the code wrong.

> Source/WebKit/Shared/WebsiteData/WebsiteDataFetchOption.h:32
> +    DoNotCreateProcesses = 1 << 1,

I think this would make more sense if it were an affirmative value,
"LaunchPluginProcesses". Launching plugin processes and computing sizes are
both optional additions to the standard fetch operation, with additional cost,
which we would like to avoid if we can. Generally speaking, flags make more
sense when they indicate the presence of something. Otherwise, you end up with
double negatives like "if (!(x & DoNotCreateProcesses))", which reads "if I do
not have the do not create processes option set", which hurts my brain.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:248
> +    OptionSet<WebKit::WebsiteDataFetchOption> fetchOptions =
WebKit::WebsiteDataFetchOption::DoNotCreateProcesses;

I'm curious -- does anybody ever specify DO create processes?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:638
> +    auto activePluginCount =
WebKit::PluginProcessManager::singleton().pluginProcesses().size();
> +    if (activePluginCount != m_activePluginCount)
> +	   return true;

What happens if a new plugin launches and an old plugin exits? It seems like
you would decide that the count is the same, and do nothing. I don't think
that's right. Or do plugins never exit, and does the active count only go up?
If so, you should assert that.

> Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:122
> +PluginProcessProxy* PluginProcessManager::getOrCreatePluginProcess(uint64_t
pluginProcessToken, ProcessAccessType processesAccessType)

I think it would be clearer if you broke this into two functions: get, and
getOrCreate, with getOrCreate calling get. It's too weird for getOrCreate to
take a parameter that specifies "just kidding, don't create".

The caller can call get or getOrCreate depending on its needs.

> Source/WebKit/UIProcess/Plugins/PluginProcessManager.h:63
> +    enum class ProcessAccessType { OnlyIfLaunched, Launch };
> +    void fetchWebsiteData(const PluginModuleInfo&, ProcessAccessType
createProcessesBehavior, WTF::Function<void (Vector<String>)>&&
completionHandler);

I think you can use the WebsiteDataFetchOption enum here. This function is
about fetching website data.