There is no GeolocationClient for WebKit-EFL.
Just in case, to clear any misunderstanding. I do not mind reviewing patches for that, but I am not gonna do any work here.
(In reply to comment #1) > Just in case, to clear any misunderstanding. I do not mind reviewing patches for that, but I am not gonna do any work here. I think it is better to push this after finishing Bug 40373 - Support only client-based Geolocation. Byungwoo, if you don't implement GeolocationClientEfl.cpp yet, why don't you submit a patch after finishing it ?
(In reply to comment #2) > (In reply to comment #1) > > Just in case, to clear any misunderstanding. I do not mind reviewing patches for that, but I am not gonna do any work here. Sorry for my misunderstanding~ Thanks for your clarification :) > > I think it is better to push this after finishing Bug 40373 - Support only client-based Geolocation. > > Byungwoo, if you don't implement GeolocationClientEfl.cpp yet, why don't you submit a patch after finishing it ? Sure~ I'll
Let's file new bug when you prepare a patch.
Huh? Closing this bug to file a new one later doesn't make much sense. You can just leave it open until someone steps up and implements these features.
Reopening as explained in comment #5.
Thanks for your suggestions, and sorry for not sharing my progress. I have a plan to implement GeolocationClientEfl.cpp/h with Geoclue DBus APIs and E_Dbus(Efl DBus) library. And according to the Gyuyoung's suggestion, I'll add initial patch for the GeolocationClientEfl when it is ready. (So, I have no plan to add patch for adding dummy files now) I'm agree with Kubo's opinion about leaving this bug for some other one. I'll check this bug and close if there is no patch until I'm ready.
Created attachment 132746 [details] Patch
I wonder if it really makes sense to add these dummy implementations of things: they do not implement any new feature and may become dead weight, just like the old GeolocationServiceEfl implementation did. Why not only commit these files when they have an actual implementation?
I made this patch for reducing patch size. I thought that dividing large patch to smaller one will be better for reviewers. This bug item still exists, so I think starting the patch with this will be no proplem. How about your opinion?
Comment on attachment 132746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132746&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:618 > + pageClients.geolocationClient = new WebCore::GeolocationClientEfl(smartData->self); Though I think you need to add GeolocationClientMock to test Geolocation's test cases by EFL DRT, looks fine for now.
(In reply to comment #10) > I made this patch for reducing patch size. I thought that dividing large patch to smaller one will be better for reviewers. > This bug item still exists, so I think starting the patch with this will be no proplem. How about your opinion? I think reviewing this kind of skeleton code is quite simple in general, and it doesn't really contribute much to the complexity of the final, whole patch. But the real problem I have is with committing this kind of dummy implementation and then not sending the actual code for a long time (or not implementing it at all), just like it happened to the previous geolocation code. There's also the risk that it takes a while between the two (or more) patches are committed. At least I would really appreciate it if you could send the rest of the implementation code in a separate patch and create a dependency between the bug reports so that we can see the bigger picture. If such code doesn't exist yet, I'd refrain from asking for this patch to be reviewed.
> Though I think you need to add GeolocationClientMock to test Geolocation's test cases by EFL DRT, looks fine for now. I also have a plan to check EFL geolocation DRT test, and will create new bug and patch for that. Thanks.
(In reply to comment #12) > At least I would really appreciate it if you could send the rest of the implementation code in a separate patch and create a dependency between the bug reports so that we can see the bigger picture. Ok~ I got the point. Thanks for your clarification. I almost finished to implement the body of this class and other related codes (including wrapper class for the e-dbus and geoclue dbus api). Now, I'm preparing a patch for internal review. I'll create a patch after the internal review is finished. ps. I need to check the EFL geolocation DRT codes for that patch, as Gyuyoung said above. And this can be another blocking issue for this patch :)
If you don't mind, I'd rather remove the r? flag from this patch until all the necessary bits are uploaded for review then.
Comment on attachment 132746 [details] Patch Clearing the r? flag for now until all the patches are sent.
Created attachment 133741 [details] Patch
Comment on attachment 133741 [details] Patch As you discussed, I'm ok for this patch because you're going to submit concrete implementation patch.
Comment on attachment 133741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133741&action=review > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.cpp:32 > + ASSERT(m_view.get()); ASSERT(m_view) will do. > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.cpp:74 > + > + Extra spaces > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.h:45 > + void geolocationDestroyed(); > + > + void startUpdating(); > + void stopUpdating(); > + void setEnableHighAccuracy(bool); > + GeolocationPosition* lastPosition(); > + > + void requestPermission(Geolocation*); > + void cancelPermissionRequest(Geolocation*); I suggest you to explicitly mark the client methods with virtual, and to use OVERRIDE. > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.h:48 > + ~GeolocationClientEfl() { }; Why did you set the destructor as protected for a a concrete class?
(In reply to comment #19) Thanks for your review. I'll apply those.
(In reply to comment #19) > Why did you set the destructor as protected for a a concrete class? It was my mistake on creating codes from GeolocationClient virtual class. Thanks for checking :)
Created attachment 133993 [details] Patch
Comment on attachment 133993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133993&action=review > Source/WebKit/PlatformEfl.cmake:138 > +IF (ENABLE_GEOLOCATION) > + LIST(APPEND WebKit_INCLUDE_DIRECTORIES > + ${WEBCORE_DIR}/Modules/geolocation > + ) > + LIST(APPEND WebKit_SOURCES > + efl/WebCoreSupport/GeolocationClientEfl.cpp > + ) > +ENDIF () I see you did not follow the indentation of this file. Please fix the indentation. > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.h:27 > +#include "GeolocationPosition.h" You can skip this header and forward declare this class. > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.h:46 > + virtual void geolocationDestroyed(); > + > + virtual void startUpdating(); > + virtual void stopUpdating(); > + virtual void setEnableHighAccuracy(bool); > + virtual GeolocationPosition* lastPosition(); > + > + virtual void requestPermission(Geolocation*); > + virtual void cancelPermissionRequest(Geolocation*); Why not also having OVERRIDE to avoid future mistake?
(In reply to comment #23) Ok~ I'll do those. Thanks~
Created attachment 134013 [details] Patch
Comment on attachment 134013 [details] Patch r+ assuming you will do the follow up.
Comment on attachment 134013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134013&action=review > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.h:25 > +#include "EWebKit.h" nitpick, this file doesn't need EWebKit.h. If you want to use Evas_Object, Evas.h is what you want.
Created attachment 134188 [details] Patch
(In reply to comment #27) > If you want to use Evas_Object, Evas.h is what you want. Thanks for the comment~. I applied it.
Comment on attachment 134188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134188&action=review > Source/WebKit/efl/WebCoreSupport/GeolocationClientEfl.h:50 > +private: > + RefPtr<Evas_Object> m_view; > +}; As discussed in private channel, m_view will be deleted when GelocationClientEfl is terminated. So, I preferred this as raw pointer like other *ClientEfl. Or, you need to refactor ewk_view to control reference count well.
Created attachment 134235 [details] Patch
(In reply to comment #31) > Created an attachment (id=134235) [details] > Patch Thanks, Looks fine to me.
As you discussed on this bug thread, I also wanna to land this patch after preparing concrete patch. So, if you don't have concrete patch it now, could you clear r? flag ? Then, please request again when you have full patch.
I removed the review flag. During internal review process, there were some other opinion about implementing the geolocation feature (using geoclue looks not good). So I dropped my previous implementation, and maybe some other one will be on this feature.
EFL Geolocation is supported by Geoclue and should be implemented for WK2 port