[webkit-reviews] review requested: [Bug 124617] Fill out WebOriginDataManager, though it is still dormant : [Attachment 219344] eliminate unused include

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 18 10:46:34 PST 2013


Brady Eidson <beidson at apple.com> has asked  for review:
Bug 124617: Fill out WebOriginDataManager, though it is still dormant
https://bugs.webkit.org/show_bug.cgi?id=124617

Attachment 219344: eliminate unused include
https://bugs.webkit.org/attachment.cgi?id=219344&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219344&action=review


> Source/WebKit2/ChangeLog:11
> +	   * Shared/APIObject.h: Add StoredSecurityOrigin object.

Thought we were getting rid of this?

> Source/WebKit2/Shared/APIObject.h:76
> +	   StoredSecurityOrigin,

Thought we were getting rid of this?

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:41
> +    StoredSecurityOriginData storedSecurityOriginData =
{{securityOrigin.protocol(), securityOrigin.host(), securityOrigin.port()},
types};

StoredSecurityOriginData already stores a SecurityOriginData.  Can't we create
that SecurityOriginData directly from the securityOrigin, instead of breaking
it down into component pieces?

This way if SecurityOriginData is ever extended in the future,
StoredSecurityOriginData gets it for free, without having to revisit this call
site.

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:48
> +    StoredSecurityOriginData storedSecurityOriginData =
{{securityOrigin.protocol, securityOrigin.host, securityOrigin.port}, types};

Shouldn't the SecurityOriginData member just be constructed from the passed in
SecurityOriginData directly?

This way if SecurityOriginData is ever extended in the future,
StoredSecurityOriginData gets it for free, without having to revisit this call
site.

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:55
> +    return SecurityOrigin::create(originData.protocol, originData.host,
originData.port);

This site could create the SecurityOrigin from the stored SecurityOriginData.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:44
> +class WebOriginDataManagerProxy::GetSitesWithDataState {

What is a "GetSitesWithDataState" object?  That name is confusing to me.

Does it need to be a member class of WebOriginDataManagerProxy?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:84
> +class WebOriginDataManagerProxy::ClearSiteDataState {

What is a "ClearSiteDataState" object?	That name is confusing to me.

Does it need to be a member class of WebOriginDataManagerProxy?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:219
> +    if (types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData)) {

> +	   if (types & kWKCookieOriginData)
> +	      
context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDat
aManager::GetOrigins(kWKCookieOriginData, callbackID));
> +
> +	   if (types & ~kWKCookieOriginData) {
> +	       // FIXME (Multi-WebProcess): <rdar://problem/12239765> Make
manipulating cache information work with per-tab WebProcess.
> +	      
context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginData
Manager::GetOrigins(types & ~(kWKKeyValueStorageOriginData |
kWKPluginDataOriginData | kWKCookieOriginData), callbackID));
> +	   }
> +    }

Uggh.  All of this bitwise operator stuff doesn't read easily.

I don't know if I have any particular suggestion.  I just feel it my duty to
point out it's not easily readable.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:341
> +    if (types & kWKCookieOriginData)
> +	  
context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDat
aManager::StartObservingChanges(kWKCookieOriginData));
> +    if (types & ~kWKCookieOriginData)
> +	  
context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginData
Manager::StartObservingChanges(types & ~kWKCookieOriginData));

Why the network/other divide here?

Couldn't all processes know which flags they are capable of monitoring, or
required to monitor?

Doesn't the WebProcess still do some networking, and therefore still need to
monitor cookie changes?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:352
> +    if (types & kWKCookieOriginData)
> +	  
context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDat
aManager::StopObservingChanges(kWKCookieOriginData));
> +    if (types & ~kWKCookieOriginData)
> +	  
context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginData
Manager::StopObservingChanges(types & ~kWKCookieOriginData));

Why the network/other divide here?

Couldn't all processes know which flags they are capable of monitoring, or
required to monitor?

Doesn't the WebProcess still do some networking, and therefore still need to
monitor cookie changes?


More information about the webkit-reviews mailing list