[webkit-reviews] review denied: [Bug 35758] [Chromium] Pass the WebFrameClient into WebStorageArea::setItem so we can figure out the routing ID : [Attachment 50040] patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 35758: [Chromium] Pass the WebFrameClient into WebStorageArea::setItem so
we can figure out the routing ID
https://bugs.webkit.org/show_bug.cgi?id=35758

Attachment 50040: patch
https://bugs.webkit.org/attachment.cgi?id=50040&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
>  String StorageAreaProxy::setItem(const String& key, const String& value,
ExceptionCode& ec, Frame* frame)
>  {
> -    bool quotaException = false;
> +    WebKit::WebFrameClient* webFrameClient = 0;
> +    WebKit::WebViewImpl* webViewImpl =
static_cast<WebKit::ChromeClientImpl*>(frame->page()->chrome()->client())->webV
iew();
> +    if (webViewImpl) {
> +	   WebKit::WebFrameImpl* webFrameImpl = webViewImpl->mainFrameImpl();
> +	   if (webFrameImpl)
> +	       webFrameClient = webFrameImpl->client();
> +    }  

I think you should just use WebFrameImpl::fromFrame(frame) here, and pass
along the WebFrame instead of the WebFrameClient.  You could also just pass
the WebView since we don't need the WebFrame.

Even better would be to move createLocalStorageNamespace to WebViewClient.
This is a much better solution since it avoids the layering violation of
passing a WebFrame or WebView to the WebKitClient (platform layer).

For just hacking a solution for the release branch, I'm okay with adding
the WebFrame/WebView parameter, but it would really be nice to not make
that be the long-term solution.

-Darin