Bug 60484 – ChromeClient::webView() is only needed for Chromium port

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Misc.

[David Kilzer (:ddkilzer)]

Created attachment 92811 [details]
Patch

Reviewed by NOBODY (OOPS!).

Source/WebCore:

* loader/EmptyClients.h:
(WebCore::EmptyChromeClient::webView): Wrap in #if
PLATFORM(CHROMIUM)/#endif.
* page/ChromeClient.h:
(WebCore::ChromeClient::webView): Ditto.
* page/brew/ChromeClientBrew.h:
(WebCore::ChromeClientBriew::webView): Removed.

Source/WebKit/efl:

* WebCoreSupport/ChromeClientEfl.h:
(WebCore::ChromeClientEfl::webView): Removed.

Source/WebKit/gtk:

NOTE: This reverts commit r85832.

* WebCoreSupport/ChromeClientGtk.h:
(WebKit::ChromeClient::webView): Restored typed, non-virtual
method.
* WebCoreSupport/EditorClientGtk.cpp:
(WebKit::imContextCommitted): Removed unnecessary static_cast.
(WebKit::imContextPreeditChanged): Ditto.
* webkit/webkitwebview.cpp:
(WebKit::kit): Ditto.

Source/WebKit/haiku:

* WebCoreSupport/ChromeClientHaiku.h:
(WebCore::ChromeClientHaiku::webView): Removed.

Source/WebKit/mac:

* WebCoreSupport/WebChromeClient.h:
(WebChromeClient::webView): Restored typed, non-virtual method.
* WebView/WebFrame.mm:
(kit): Removed unnecessary static_cast.

Source/WebKit/qt:

* WebCoreSupport/ChromeClientQt.h:
(WebCore::ChromeClientQt::webView): Removed.

Source/WebKit/win:

NOTE: This reverts commit r85831.

* WebCoreSupport/WebChromeClient.h:
(WebChromeClient::webView): Restored typed, non-virtual method.
* WebView.cpp:
(kit): Removed unnecessary static_cast.

Source/WebKit/wince:

* WebCoreSupport/ChromeClientWinCE.h:
(WebKit::ChromeClientWinCE::webView): Removed.

Source/WebKit/wx:

* WebKitSupport/ChromeClientWx.h:
(WebCore::ChromeClientWX::webView): Removed.

Source/WebKit2:

* WebProcess/WebCoreSupport/WebChromeClient.h:
(WebKit::WebChromeClient::webView): Removed.
---
 26 files changed, 123 insertions(+), 17 deletions(-)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/win/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/qt/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/gtk/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/wx/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/wince/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/haiku/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 10 in 19 files


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

[Adam Barth]

Comment on attachment 92811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92811&action=review

> Source/WebCore/page/ChromeClient.h:137
> +#if PLATFORM(CHROMIUM)
>          virtual void* webView() const = 0;
> +#endif

Why is this feature Chromium-specific?  Having a WebView is a general concept that's shared by most ports.  It would be nice to have more explanation in the changelog for why we're making this change.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:39
> -    virtual void* webView() const { return static_cast<void*>(m_webView); }
> +    WebView* webView() const { return m_webView; }

It's confusing that this client has a method that's exactly the same name as one in its superclass but that's ifdefed away on this port.

> Source/WebKit/win/WebCoreSupport/WebChromeClient.h:43
> +    WebView* webView() const { return m_webView; }

Same here

[Darin Adler]

Comment on attachment 92811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92811&action=review

>> Source/WebCore/page/ChromeClient.h:137
>> +#endif
> 
> Why is this feature Chromium-specific?  Having a WebView is a general concept that's shared by most ports.  It would be nice to have more explanation in the changelog for why we're making this change.

Having a void* pointer to a web view when it’s a higher level concept is a reverse dependency that we should not use. Nobody can make use of a void* directly, so it seems like some sort of hook that can’t be used in any clean way.

Having a pointer to this in the chrome client like this is a design mistake. I think we should make it Chromium-only but then get rid of it entirely.

[Adam Barth]

> Having a void* pointer to a web view when it’s a higher level concept is a reverse dependency that we should not use. Nobody can make use of a void* directly, so it seems like some sort of hook that can’t be used in any clean way.
> 
> Having a pointer to this in the chrome client like this is a design mistake. I think we should make it Chromium-only but then get rid of it entirely.

Ok.  That sounds like good information to include in the ChangeLog.

It's unclear to me how else to solve the problem that this method solves (at least in Chromium), short of removing SVG image, of course.

[Darin Adler]

Comment on attachment 92811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92811&action=review

>>> Source/WebCore/page/ChromeClient.h:137
>>> +#endif
>> 
>> Why is this feature Chromium-specific?  Having a WebView is a general concept that's shared by most ports.  It would be nice to have more explanation in the changelog for why we're making this change.
> 
> Having a void* pointer to a web view when it’s a higher level concept is a reverse dependency that we should not use. Nobody can make use of a void* directly, so it seems like some sort of hook that can’t be used in any clean way.
> 
> Having a pointer to this in the chrome client like this is a design mistake. I think we should make it Chromium-only but then get rid of it entirely.

WebKit2 doesn’t have a WebView.

There is no code anywhere in WebCore that calls this webView() function. We should not have to add hooks in WebCore for the WebKit level to manage its internal object relationship. It’s more flexible to do that entirely at the WebKit level.

I think that the 4 places in the Chrome port that get to the WebView this way should be changed to use the standard model; in this model, a pointer to the WebView is added, if needed, at the WebKit level.

[Adam Barth]

> I think that the 4 places in the Chrome port that get to the WebView this way should be changed to use the standard model; in this model, a pointer to the WebView is added, if needed, at the WebKit level.

I'd be happy to discuss this topic with you in more detail (and we don't need to hold up this bug for that discussion).

My understanding is that the standard approach is as follows:

Application
WebKit
WebCore
Platform
OS API

The difficulty is that because of the sandbox (and mostly it's because of the windows sandbox because Mac's sandbox is more fine-grained), some of the work that other ports do via the Platform need to be done by the Chromium embedder.  That leads to the following layering diagram:

Application
WebKit
WebCore
Platform
Application
OS API

In the SVG image case, the layering is actually more like:

Fake WebKit layer provided by SVGImage
WebCore
Platform
OS API

In this situation, WebCore has been disconnected from the real WebKit layer by the EmptyClients.  The code in Platform that previously needed to communicate with the Application needs some way of knowing that the Application is not long part of the stack.

The approach here is to signal that by having the webView pointer be null for the EmptyClient.  Is there another approach that would be better to use?  I'm certainly don't think the approach we chose is very good.  I'm couldn't figure out how to do it better.

[Blaze Burg]

webView() is still around in some subclasses of ChromeClient, but it's typed now. Closing.