Bug 97427

Created attachment 169071 [details] Patch Patch.

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 ?

(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.

Created attachment 169340 [details] Patch

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?

(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.

(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.

(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?

(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.

(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.

Created attachment 171773 [details] Patch

Comment on attachment 171773 [details] Patch Why not requesting r?

Created attachment 171785 [details] Patch

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

Comment on attachment 171785 [details] Patch LGTM

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 ?)

Created attachment 171814 [details] Patch Reverted code to use s_drtRun flag

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.

Created attachment 171983 [details] Patch

Created attachment 171984 [details] Patch

Comment on attachment 171984 [details] Patch Clearing flags on attachment: 171984 Committed r133266: <http://trac.webkit.org/changeset/133266>

All reviewed patches have been landed. Closing bug.