Bug 97427 – [EFL][DRT] Support Geolocation

• Component: WebKit EFL
• Status: RESOLVED FIXED

[seonae kim]

The EFL port should support Geolocation DRT as follows:

setMockGeolocationReset
setMockGeolocationPermission
setMockGeolocationPosition
setMockGeolocationError
numberOfPendingGeolocationPermissionRequests

[seonae kim]

Created attachment 169071 [details]
Patch

Patch.

[Gyuyoung Kim]

Comment on attachment 169071 [details]
Patch

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

As you know, WK2 is important for us. Will you support geolocation for WK2 as well ?

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:763
> +    mock->reset();

To avoid unused parameter build warning when geolocation is disabled, you need to use UNUSED_PARAM macro as below,

#else
    UNUSED_PARAM(ewkView)
#endif

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:774
> +#endif

ditto.

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:784
> +#endif

ditto.

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:794
> +#endif

ditto.

> Tools/DumpRenderTree/efl/TestRunnerEfl.cpp:372
> +        view = browser->extraViews().last();

Why do you only test last view when there are multiple views ?

[seonae kim]

(In reply to comment #2)
> (From update of attachment 169071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169071&action=review
> 
> As you know, WK2 is important for us. Will you support geolocation for WK2 as well ?
In case WK2, I need to talk with byungwoo lee.
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:763
> > +    mock->reset();
> 
> To avoid unused parameter build warning when geolocation is disabled, you need to use UNUSED_PARAM macro as below,
I'll change as you said.

> 
> #else
>     UNUSED_PARAM(ewkView)
> #endif
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:774
> > +#endif
> 
> ditto.
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:784
> > +#endif
> 
> ditto.
> 
> > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:794
> > +#endif
> 
> ditto.
> 
> > Tools/DumpRenderTree/efl/TestRunnerEfl.cpp:372
> > +        view = browser->extraViews().last();
> 
> Why do you only test last view when there are multiple views ?
Precisely, it is not last and current, because last of extraViews has been to maintain a current view. Gtk port also used a similar view that prepend for webViewList.

[seonae kim]

Created attachment 169340 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 169340 [details]
Patch

I'm not a fan of the setDumpRenderTreeModeEnabled approach; so far we have designed DRT and DRTSupport in a way that the normal code paths do not need to know what DRT is or if it is being run at all (contrary to Qt's DRT, for example). Why don't you move those calls you have added to _ewk_view_priv_new to a method in DRTSupport and call it whenever you create a view in DRT?

[seonae kim]

(In reply to comment #5)
> (From update of attachment 169340 [details])
> I'm not a fan of the setDumpRenderTreeModeEnabled approach; so far we have designed DRT and DRTSupport in a way that the normal code paths do not need to know what DRT is or if it is being run at all (contrary to Qt's DRT, for example). Why don't you move those calls you have added to _ewk_view_priv_new to a method in DRTSupport and call it whenever you create a view in DRT?

I don't understand why dumpRenderTreeModeEnabled should be moved to DRTSupport.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> > I'm not a fan of the setDumpRenderTreeModeEnabled approach; so far we have designed DRT and DRTSupport in a way that the normal code paths do not need to know what DRT is or if it is being run at all (contrary to Qt's DRT, for example). Why don't you move those calls you have added to _ewk_view_priv_new to a method in DRTSupport and call it whenever you create a view in DRT?
>
> I don't understand why dumpRenderTreeModeEnabled should be moved to DRTSupport.

Basically, for the reasons I outlined above: the code in EWK should be oblivious to whether it is running normally or within DRT. This centralizes the testing code in a single place (DRTSupport instead of different places as it has happened to the DRT code in the Qt port) and eases the maintenance.

[seonae kim]

(In reply to comment #7)
> (In reply to comment #6)
> > > I'm not a fan of the setDumpRenderTreeModeEnabled approach; so far we have designed DRT and DRTSupport in a way that the normal code paths do not need to know what DRT is or if it is being run at all (contrary to Qt's DRT, for example). Why don't you move those calls you have added to _ewk_view_priv_new to a method in DRTSupport and call it whenever you create a view in DRT?
> >
> > I don't understand why dumpRenderTreeModeEnabled should be moved to DRTSupport.
> 
> Basically, for the reasons I outlined above: the code in EWK should be oblivious to whether it is running normally or within DRT. This centralizes the testing code in a single place (DRTSupport instead of different places as it has happened to the DRT code in the Qt port) and eases the maintenance.

I agree with you that the code is used in a single place.
But dumpRenderTreeModeEnabled need to distinguish the controller in runtime whether GeolocationClientMock is used or not.
Is there any other way if this code not used in _ewk_view_priv_new?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #8)
> But dumpRenderTreeModeEnabled need to distinguish the controller in runtime whether GeolocationClientMock is used or not.
> Is there any other way if this code not used in _ewk_view_priv_new?

I suggest adding a method to DRTSupport that retrieves a WebCore::Page via EWKPriv::corePage() and calls the code you've currently added to _ewk_view_priv_new(). You then call this method from DumpRenderTreeChrome.cpp (or DumpRenderTreeView.cpp, I don't remember) whenever you are creating a new view.

[seonae kim]

(In reply to comment #9)
> (In reply to comment #8)
> > But dumpRenderTreeModeEnabled need to distinguish the controller in runtime whether GeolocationClientMock is used or not.
> > Is there any other way if this code not used in _ewk_view_priv_new?
> 
> I suggest adding a method to DRTSupport that retrieves a WebCore::Page via EWKPriv::corePage() and calls the code you've currently added to _ewk_view_priv_new(). You then call this method from DumpRenderTreeChrome.cpp (or DumpRenderTreeView.cpp, I don't remember) whenever you are creating a new view.

Okay, I understand and think that this way you suggest is clearer than previous.

[seonae kim]

Created attachment 171773 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 171773 [details]
Patch

Why not requesting r?

[seonae kim]

Created attachment 171785 [details]
Patch

[seonae kim]

(In reply to comment #12)
> (From update of attachment 171773 [details])
> Why not requesting r?
Sorry, I did.

[Gyuyoung Kim]

Comment on attachment 171785 [details]
Patch

LGTM

[Gyuyoung Kim]

Comment on attachment 171785 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:89
> +    WebCore::provideGeolocationTo(page, mock);

Sorry. I missed one. If we add this one to here, we add GeolocationClient twice. One is ewk_view and other is here.

Because, you will add a GeolocationClientEfl to WebCore's supplement in _ewk_view_priv_new, right ? When we run layout test, GeolocationClient will be added twice by this. It might to make assertion.

We have to follow other ports's implementation using a flag. (e.g. s_drt ?)

[seonae kim]

Created attachment 171814 [details]
Patch

Reverted code to use s_drtRun flag

[Gyuyoung Kim]

Comment on attachment 171814 [details]
Patch

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

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:200
> +    DumpRenderTreeSupportEfl::setDumpRenderTreeModeEnabled(true);

Add a comment why you need this setting.

For example,
// Set running in DRT mode for ewkview to create testable objects.

In addition, it would be good if you call this by alphabetical order.

[seonae kim]

Created attachment 171983 [details]
Patch

[seonae kim]

Created attachment 171984 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171984 [details]
Patch

Clearing flags on attachment: 171984

Committed r133266: <http://trac.webkit.org/changeset/133266>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.