Bug 97427 - [EFL][DRT] Support Geolocation
Summary: [EFL][DRT] Support Geolocation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 82140
  Show dependency treegraph
 
Reported: 2012-09-23 23:42 PDT by seonae kim
Modified: 2012-11-01 22:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (15.85 KB, patch)
2012-10-16 19:07 PDT, seonae kim
no flags Details | Formatted Diff | Diff
Patch (16.44 KB, patch)
2012-10-17 22:04 PDT, seonae kim
no flags Details | Formatted Diff | Diff
Patch (14.69 KB, patch)
2012-10-31 23:04 PDT, seonae kim
no flags Details | Formatted Diff | Diff
Patch (14.69 KB, patch)
2012-11-01 00:13 PDT, seonae kim
gyuyoung.kim: review-
Details | Formatted Diff | Diff
Patch (15.95 KB, patch)
2012-11-01 04:47 PDT, seonae kim
gyuyoung.kim: review+
Details | Formatted Diff | Diff
Patch (16.01 KB, patch)
2012-11-01 21:25 PDT, seonae kim
no flags Details | Formatted Diff | Diff
Patch (16.00 KB, patch)
2012-11-01 21:32 PDT, seonae kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description seonae kim 2012-09-23 23:42:32 PDT
The EFL port should support Geolocation DRT as follows:

setMockGeolocationReset
setMockGeolocationPermission
setMockGeolocationPosition
setMockGeolocationError
numberOfPendingGeolocationPermissionRequests
Comment 1 seonae kim 2012-10-16 19:07:22 PDT
Created attachment 169071 [details]
Patch

Patch.
Comment 2 Gyuyoung Kim 2012-10-16 19:26:06 PDT
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 ?
Comment 3 seonae kim 2012-10-16 21:31:51 PDT
(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.
Comment 4 seonae kim 2012-10-17 22:04:22 PDT
Created attachment 169340 [details]
Patch
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-10-18 06:28:44 PDT
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?
Comment 6 seonae kim 2012-10-19 01:43:06 PDT
(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.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-10-19 02:39:58 PDT
(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.
Comment 8 seonae kim 2012-10-28 06:03:23 PDT
(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?
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-10-29 02:22:17 PDT
(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.
Comment 10 seonae kim 2012-10-30 23:48:04 PDT
(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.
Comment 11 seonae kim 2012-10-31 23:04:29 PDT
Created attachment 171773 [details]
Patch
Comment 12 Gyuyoung Kim 2012-10-31 23:52:20 PDT
Comment on attachment 171773 [details]
Patch

Why not requesting r?
Comment 13 seonae kim 2012-11-01 00:13:55 PDT
Created attachment 171785 [details]
Patch
Comment 14 seonae kim 2012-11-01 00:18:17 PDT
(In reply to comment #12)
> (From update of attachment 171773 [details])
> Why not requesting r?
Sorry, I did.
Comment 15 Gyuyoung Kim 2012-11-01 02:36:39 PDT
Comment on attachment 171785 [details]
Patch

LGTM
Comment 16 Gyuyoung Kim 2012-11-01 03:39:50 PDT
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 ?)
Comment 17 seonae kim 2012-11-01 04:47:34 PDT
Created attachment 171814 [details]
Patch

Reverted code to use s_drtRun flag
Comment 18 Gyuyoung Kim 2012-11-01 19:43:47 PDT
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.
Comment 19 seonae kim 2012-11-01 21:25:56 PDT
Created attachment 171983 [details]
Patch
Comment 20 seonae kim 2012-11-01 21:32:59 PDT
Created attachment 171984 [details]
Patch
Comment 21 WebKit Review Bot 2012-11-01 22:54:54 PDT
Comment on attachment 171984 [details]
Patch

Clearing flags on attachment: 171984

Committed r133266: <http://trac.webkit.org/changeset/133266>
Comment 22 WebKit Review Bot 2012-11-01 22:54:59 PDT
All reviewed patches have been landed.  Closing bug.