Cleanup: Adding more conversion helper interfaces.

Adding toChromeClientImpl & toFrameLoaderClientImpl conversion functions to replace static_cast.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159290

[r.kasibhatla, 2013-10-08 11:05:22]

PTAL. More cleanup in Source/web directory.

[tkent, 2013-10-08 22:07:03]

I still recommend to split the change to multiple pieces.  It's ok to handle
WebWidget-related classes at once. However other classes should be handled one
by one.

[tkent, 2013-10-08 22:22:39]

https://codereview.chromium.org/26450004/diff/1/Source/core/loader/FrameLoade...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/26450004/diff/1/Source/core/loader/FrameLoade...
Source/core/loader/FrameLoaderClient.h:225: virtual bool isFrameLoaderClient()
const { return false; }
The function name is very confusing. This is FrameLoaderClient class, and should
not have isFrameLoaderClient function.  Do you mean isFrameLoaderClientImpl()?

We had better wrap this function with "#if !ASSERT_DISABLED".

https://codereview.chromium.org/26450004/diff/1/Source/core/page/ChromeClient.h
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/26450004/diff/1/Source/core/page/ChromeClient...
Source/core/page/ChromeClient.h:264: virtual bool isChromeClient() const {
return false; }
Ditto. The function name is confusing. Wrap with "#if !ASSERT_DISABLED"

https://codereview.chromium.org/26450004/diff/1/Source/web/ChromeClientImpl.h
File Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/26450004/diff/1/Source/web/ChromeClientImpl.h...
Source/web/ChromeClientImpl.h:224: // No need for null check, as the client
returned by Page::client() is actually a reference.
So, we should have
  inline ChromeClientImpl* toChromeClientImpl(ChromeClient& client) or
  inline ChromeClientImpl& toChromeClientImpl(ChromeClient& client)

https://codereview.chromium.org/26450004/diff/1/Source/web/FrameLoaderClientI...
File Source/web/FrameLoaderClientImpl.h (right):

https://codereview.chromium.org/26450004/diff/1/Source/web/FrameLoaderClientI...
Source/web/FrameLoaderClientImpl.h:170: ASSERT_WITH_SECURITY_IMPLICATION(!client
|| client->isFrameLoaderClient());
Is there FrameLoaderClient implementations other than FrameLoaderClientImpl?

https://codereview.chromium.org/26450004/diff/1/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/26450004/diff/1/public/web/WebWidget.h#newcod...
public/web/WebWidget.h:224: virtual bool isPagePopup() const { return false; }
This is ok.
It's hard to wrap these functions with "#if !ASSERT_DISABLED" because this is a
public header.

[tkent, 2013-10-09 00:21:51]

On 2013/10/08 22:22:39, tkent wrote:
> We had better wrap this function with "#if !ASSERT_DISABLED".

> Ditto. The function name is confusing. Wrap with "#if !ASSERT_DISABLED"

I remembered ASSERT_WITH_SECURITY_IMPLICATION can be enabled with
ASSERT_DISABLED. So please ignore these comments.

[r.kasibhatla, 2013-10-09 04:56:32]

On 2013/10/08 22:22:39, tkent wrote:
>
https://codereview.chromium.org/26450004/diff/1/Source/core/loader/FrameLoade...
> File Source/core/loader/FrameLoaderClient.h (right):
> 
>
https://codereview.chromium.org/26450004/diff/1/Source/core/loader/FrameLoade...
> Source/core/loader/FrameLoaderClient.h:225: virtual bool isFrameLoaderClient()
> const { return false; }
> The function name is very confusing. This is FrameLoaderClient class, and
should
> not have isFrameLoaderClient function.  Do you mean isFrameLoaderClientImpl()?
> 
> We had better wrap this function with "#if !ASSERT_DISABLED".
> 
>
https://codereview.chromium.org/26450004/diff/1/Source/core/page/ChromeClient.h
> File Source/core/page/ChromeClient.h (right):
> 
>
https://codereview.chromium.org/26450004/diff/1/Source/core/page/ChromeClient...
> Source/core/page/ChromeClient.h:264: virtual bool isChromeClient() const {
> return false; }
> Ditto. The function name is confusing. Wrap with "#if !ASSERT_DISABLED"
> 
> https://codereview.chromium.org/26450004/diff/1/Source/web/ChromeClientImpl.h
> File Source/web/ChromeClientImpl.h (right):
> 
>
https://codereview.chromium.org/26450004/diff/1/Source/web/ChromeClientImpl.h...
> Source/web/ChromeClientImpl.h:224: // No need for null check, as the client
> returned by Page::client() is actually a reference.
> So, we should have
>   inline ChromeClientImpl* toChromeClientImpl(ChromeClient& client) or
>   inline ChromeClientImpl& toChromeClientImpl(ChromeClient& client)
> 
>
https://codereview.chromium.org/26450004/diff/1/Source/web/FrameLoaderClientI...
> File Source/web/FrameLoaderClientImpl.h (right):
> 
>
https://codereview.chromium.org/26450004/diff/1/Source/web/FrameLoaderClientI...
> Source/web/FrameLoaderClientImpl.h:170:
ASSERT_WITH_SECURITY_IMPLICATION(!client
> || client->isFrameLoaderClient());
> Is there FrameLoaderClient implementations other than FrameLoaderClientImpl?
Just like for ChromeClient, even FrameLoaderClient has another implementation in
EmptyFrameLoaderClient (other than FrameLoaderClientImpl). So, we do need
is<Type> checking for FrameLoaderClient also.
> 
> https://codereview.chromium.org/26450004/diff/1/public/web/WebWidget.h
> File public/web/WebWidget.h (right):
> 
>
https://codereview.chromium.org/26450004/diff/1/public/web/WebWidget.h#newcod...
> public/web/WebWidget.h:224: virtual bool isPagePopup() const { return false; }
> This is ok.
> It's hard to wrap these functions with "#if !ASSERT_DISABLED" because this is
a
> public header.

[tkent, 2013-10-10 02:00:46]

lgtm

[commit-bot: I haz the power, 2013-10-10 02:01:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/26450004/15001

[commit-bot: I haz the power, 2013-10-10 04:48:25]

Change committed as 159290