Bug 79354 – [EFL] Add dummy GeolocationClientEfl.cpp | h

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit EFL

[Byungwoo Lee]

There is no GeolocationClient for WebKit-EFL.

[Benjamin Poulain]

Just in case, to clear any misunderstanding. I do not mind reviewing patches for that, but I am not gonna do any work here.

[Gyuyoung Kim]

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

[Byungwoo Lee]

(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

[Gyuyoung Kim]

Let's file new bug when you prepare a patch.

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

Reopening as explained in comment #5.

[Byungwoo Lee]

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.

[Byungwoo Lee]

Created attachment 132746 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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?

[Byungwoo Lee]

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?

[Gyuyoung Kim]

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.

[Raphael Kubo da Costa (:rakuco)]

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

[Byungwoo Lee]

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

[Byungwoo Lee]

(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 :)

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 132746 [details]
Patch

Clearing the r? flag for now until all the patches are sent.

[Byungwoo Lee]

Created attachment 133741 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 133741 [details]
Patch

As you discussed, I'm ok for this patch because you're going to submit concrete implementation patch.

[Benjamin Poulain]

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?

[Byungwoo Lee]

(In reply to comment #19)
Thanks for your review. I'll apply those.

[Byungwoo Lee]

(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 :)

[Byungwoo Lee]

Created attachment 133993 [details]
Patch

[Benjamin Poulain]

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?

[Byungwoo Lee]

(In reply to comment #23)
Ok~ I'll do those. Thanks~

[Byungwoo Lee]

Created attachment 134013 [details]
Patch

[Benjamin Poulain]

Comment on attachment 134013 [details]
Patch

r+ assuming you will do the follow up.

[Ryuan Choi]

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.

[Byungwoo Lee]

Created attachment 134188 [details]
Patch

[Byungwoo Lee]

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

[Ryuan Choi]

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.

[Byungwoo Lee]

Created attachment 134235 [details]
Patch

[Ryuan Choi]

(In reply to comment #31)
> Created an attachment (id=134235) [details]
> Patch

Thanks, Looks fine to me.

[Gyuyoung Kim]

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.

[Byungwoo Lee]

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.

[Gyuyoung Kim]

EFL Geolocation is supported by Geoclue and should be implemented for WK2 port