[webkit-reviews] review denied: [Bug 198935] Crash at com.apple.WebKit: WebKit::WebsiteDataStore::processPools const : [Attachment 372279] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 18 10:21:05 PDT 2019

Geoffrey Garen <ggaren at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 198935: Crash at com.apple.WebKit: WebKit::WebsiteDataStore::processPools

Attachment 372279: Patch


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

Separation of concerns <https://en.wikipedia.org/wiki/Separation_of_concerns>
is one of our most powerful abstractions for writing maintainable code. It's
why we break up code into functions, and why we break up code and data into

This patch eliminates some of the separation of concerns between
WebsiteDataStore and WebProcessProxy because it requires WebsiteDataStore to
know all of the conditions under which WebProcessProxy's processPool() might be
null. If someone changes those conditions in WebProcessProxy in the future,
they need to know that they should also update the code in WebsiteDataStore --
but they're very unlikely to know that since, by appearance, WebsiteDataStore
and WebProcessProxy are separate classes.

I'd suggest instead is that you add a function named
WebProcessProxy::processPoolIfExists(). (That's not my favorite name, but it's
a naming scheme we've agreed upon for WebKit.)
WebProcessProxy::processPoolIfExists() should return m_processPool if it is not
null, or null otherwise. If you do that, then WebsiteDataStore can call
processPoolIfExists() without knowing all the conditions that might make the
return value null, retaining most of the separation of concerns.

More information about the webkit-reviews mailing list