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

[bugzilla-daemon at webkit.org]

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
const
https://bugs.webkit.org/show_bug.cgi?id=198935

Attachment 372279: Patch

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




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

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
classes.

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.