[webkit-reviews] review denied: [Bug 59199] [Qt][WK2] Implement geolocation provider for qt port : [Attachment 92231] patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Mahesh Kulkarni
<mahesh.kulkarni at nokia.com>'s request for review:
Bug 59199: [Qt][WK2] Implement geolocation provider for qt port
https://bugs.webkit.org/show_bug.cgi?id=59199

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92231&action=review

I don't know much about the subject but there already a few good comments so I
r-
Some comments on my side:

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:2
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

2011

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:30
> +static WebGeolocationProviderQt* toLocationProvider(const void* clientInfo)

This is a helper that do not execute any instruction, it could probably be set
explicitely inline.

>> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:32
>> +	return
reinterpret_cast<WebGeolocationProviderQt*>(const_cast<void*>(clientInfo));
> 
> Ouch. It's not very nice. How other ports are doing?

I have to agree that look strange. The geolocationManager is mutable, the
clientInfo isn't, it is strange we only use clientInfo there.

Why not remove the constness from the definition of WKGeolocationProvider. This
make sense to have it const for other port but not in this case?

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:51
> +    : QObject()

Unnecessary.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:95
> +	       // Mobility source is available.

I think this comment is not adding much.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:2
> +    Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

2011

> Source/WebKit2/UIProcess/qt/WebContextQt.cpp:56
> +    static WebGeolocationProviderQt* location =
WebGeolocationProviderQt::create(toAPI(geolocationManagerProxy()));
> +    Q_UNUSED(location);

This can be made cleaner. You should not require a comment when you do a static
initialization, the code should be clear enough by itself.