Bug 54334 – [QT] Implement mock client-based geolocation for layout testing

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

[Mahesh Kulkarni]

This is a follow up bug #42629

This bug is to track implementation of layout testing for client-based geolocation implementation for qtwebkit.

[Mahesh Kulkarni]

Created attachment 84479 [details]
patch

Enables layout testing for client-based geolocation. 

One case related to multiple windows escaped and will raise a different bug to fix this. This needs little more work in DRT.

Please review.

[Kenneth Rohde Christiansen]

Comment on attachment 84479 [details]
patch

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

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h:146
> +    static void mockGeolocationReset(QWebPage*);

resetGeolocationMock feels more natural to me.

> Source/WebKit/qt/Api/qwebpage.cpp:338
> +    // Incase running in DumpRenderTree mode set the controller to mock provider.

in case are two words

> Source/WebKit/qt/Api/qwebpage.cpp:340
> +    if (QWebPagePrivate::drtRun)
> +        static_cast<GeolocationClientMock*>(pageClients.geolocationClient)->setController(page->geolocationController());

You could create a 

GeolocationClientMock* toGeolocationClientMock(...)
{
     ASSERT(QWebPagePrivate::drtRun);
     return static...
}

if you ever need to use this in more places.

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:425
> +    // set running in DRT mode for qwebpage to create testable objects

In general we try to write proper sentences, ie. start with capital and end with a punctuation mark of some kind.

[Mahesh Kulkarni]

Created attachment 84609 [details]
patch

Updated patch as per above comments

[Laszlo Gombos]

Comment on attachment 84609 [details]
patch

cq+ for commit.

[WebKit Commit Bot]

Comment on attachment 84609 [details]
patch

Clearing flags on attachment: 84609

Committed r80319: <http://trac.webkit.org/changeset/80319>

[Laszlo Gombos]

Comment on attachment 84479 [details]
patch

Clearing r+ to take it out from the pending commit list as the patch has been landed.

[Steve Block]

Comment on attachment 84609 [details]
patch

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

> Source/WebCore/page/GeolocationController.h:60
> +    GeolocationClient* client() { return m_client; }

I'm surprised that you had to add this - it wasn't needed by other ports - can you explain why? It seems odd that a port would need to query the Page/GeolocationController to get a client that it has itself supplied to that Page.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.