Bug 35758 – [Chromium] Pass the WebFrameClient into WebStorageArea::setItem so we can figure out the routing ID

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Pass the WebFrameClient into WebStorageArea::setItem so we can figure out the routing ID.

[Jeremy Orlow]

Created attachment 50040 [details]
patch

[WebKit Review Bot]

Attachment 50040 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/StorageAreaProxy.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/StorageAreaProxy.cpp:91:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/chromium/public/WebStorageArea.h:77:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50040 [details]
patch

>  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())->webView();
> +    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

[Jeremy Orlow]

(In reply to comment #3)
> (From update of attachment 50040 [details])
> >  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())->webView();
> > +    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.

Moving createLocalStorageNamespace does not help or make sense since the LocalStorageNamespace is created on first use and stored with the page group.  Thus it doesn't tell us which frame/WebView is currently using it.

Passing in a WebFrame doesn't work because only the WebFrameImpl (which is private to the WebKit API) is the only way to get at the WebFrameClient*.  As far as I can tell, we can't get a WebFrameClient from a WebView either.

I believe using WebFrameImpl::fromFrame() should work and will clean up the code though.  But besides that, I don't see how any of these suggestions are workable.  If you have a second to look at this in depth and see if there is a way to simplify it, I'd greatly appreciate it, but as far as I can tell, fromFrame() is the only thing I can change here.  I don't believe I technically have any layering violations either since WebFrameClient is in public.  Am I missing anything?

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 50040 [details] [details])
> > >  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())->webView();
> > > +    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.
> 
> Moving createLocalStorageNamespace does not help or make sense since the
> LocalStorageNamespace is created on first use and stored with the page group. 
> Thus it doesn't tell us which frame/WebView is currently using it.

I see.


> Passing in a WebFrame doesn't work because only the WebFrameImpl (which is
> private to the WebKit API) is the only way to get at the WebFrameClient*.  As
> far as I can tell, we can't get a WebFrameClient from a WebView either.

No, no... in Chromium land, you are trying to get to a RenderView.  RenderView
has a static FromWebView method that you can use to map from a WebView to a
RenderView.

Please note that it is valid to use the WebKit API with a null WebFrameClient
if you are not interested in any of the WebFrameClient methods.  Same goes for
other Client interfaces (except for WebKitClient).  This is why it is better to
pass around the WebFrame or WebView instead.


> fromFrame() is the only thing I can change here.  I don't believe I technically
> have any layering violations either since WebFrameClient is in public.  Am I
> missing anything?

WebKitClient is supposed to correspond to platform services, which should not be
parameterized by a frame.  If we need to make per-frame queries, then that is
what WebFrameClient is for.

Another way to fix this problem is to add a WebFrameClient method named
setStorageItem (or something like that), but that seems a little strange.

[Darin Fisher (:fishd, Google)]

I thought about this some more, and I'm fine with passing the WebFrame as a parameter to WebStorageArea methods.

[Jeremy Orlow]

Created attachment 50094 [details]
Addressed comments

I guess using the WebView is better, though it seems less efficient having to rely on the map, I guess it doesn't really matter.

All the other comments have been addressed.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50094 [details]
Addressed comments

> +    virtual void setItem(const WebString& key, const WebString& newValue, const WebURL& url, Result& result, WebString& oldValue, WebView*)

nit: it occurred to me that passing the WebView loses information.  passing the WebFrame
might be better, although it doesn't matter to chromium.

r=me

[Jeremy Orlow]

Landed in 55659