Bug 97630 – Add DeviceProximityController to support Device Proximity Events.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kihong Kwon]

Implement DeviceProximityController.

[Kihong Kwon]

Created attachment 165729 [details]
Initial patch.

Initial patch to show how to work DeviceController.

[Kihong Kwon]

Created attachment 177079 [details]
Patch

[Hajime Morrita]

Comment on attachment 177079 [details]
Patch

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

> Source/WebCore/Modules/proximity/DeviceProximityClient.h:53
> +protected:

In principle, client superclasses shouldn't have any state. It should be pure virtual class.

> Source/WebCore/Target.pri:255
> +    accessibility/AccessibilityImageMapLink.cpp \

Why change this?

> Source/WebCore/platform/mock/DeviceProximityClientMock.h:1
> +/*

Could we implement this in testing/ and get rid of new DRT API?
Having mock implementation built into WebCore and adding new DRT API are both unfortunate
and better to be avoided if possible.

[Kihong Kwon]

> > Source/WebCore/Modules/proximity/DeviceProximityClient.h:53
> > +protected:
> 
> In principle, client superclasses shouldn't have any state. It should be pure virtual class.

I understand. It'll be changed.
> 
> > Source/WebCore/Target.pri:255
> > +    accessibility/AccessibilityImageMapLink.cpp \
> 
> Why change this?
Opps, It's mistake.
> 
> > Source/WebCore/platform/mock/DeviceProximityClientMock.h:1
> > +/*
> 
> Could we implement this in testing/ and get rid of new DRT API?
> Having mock implementation built into WebCore and adding new DRT API are both unfortunate
> and better to be avoided if possible.

OK, I'll try this.

Thank you. :)

[Kihong Kwon]

Created attachment 177978 [details]
Patch

[EFL EWS Bot]

Comment on attachment 177978 [details]
Patch

Attachment 177978 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15151919

[Build Bot]

Comment on attachment 177978 [details]
Patch

Attachment 177978 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15173303

[Kihong Kwon]

Created attachment 178149 [details]
Patch

[Hajime Morrita]

Looks good to me.
I want someone else in EFL team to take another looks, mainly for EFL part before r+.
I have no idea how good it is, even though the code is relatively small.

[Gyuyoung Kim]

Comment on attachment 178149 [details]
Patch

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

Kihong, almost looks good to me in EFL WK1 side. However, as you know, we are concentrating on WK2 EFL port. I think you need to support this new feature for WK2 EFL first.

> Source/WebKit/efl/WebCoreSupport/DeviceProximityClientEfl.h:39
> +    virtual ~DeviceProximityClientEfl() { }

Why do you need to add virtual keyword here ? Don't we need to use virtual in child class ?

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 178149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178149&action=review
> 
> Kihong, almost looks good to me in EFL WK1 side. However, as you know, we are concentrating on WK2 EFL port. I think you need to support this new feature for WK2 EFL first.

Or, you may be able to add DeviceProximityProviderEfl.cpp | h to WebCore/platform/efl in order to support this functionality both EFL WK1 and WK2.

[Kihong Kwon]

(In reply to comment #10)
> > Source/WebKit/efl/WebCoreSupport/DeviceProximityClientEfl.h:39
> > +    virtual ~DeviceProximityClientEfl() { }
> 
> Why do you need to add virtual keyword here ? Don't we need to use virtual in child class ?

You are right but if someone want to make ClientEflXXX which is inherited by DeviceProximityClientEfl.h, it can make a problem. That's why I added virtual keyword here.

[Kihong Kwon]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 178149 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=178149&action=review
> > 
> > Kihong, almost looks good to me in EFL WK1 side. However, as you know, we are concentrating on WK2 EFL port. I think you need to support this new feature for WK2 EFL first.
> 
> Or, you may be able to add DeviceProximityProviderEfl.cpp | h to WebCore/platform/efl in order to support this functionality both EFL WK1 and WK2.

I think this is better. I will do this.
Thanks Gyuyoung.

[Kihong Kwon]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 178149 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=178149&action=review
> > > 
> > > Kihong, almost looks good to me in EFL WK1 side. However, as you know, we are concentrating on WK2 EFL port. I think you need to support this new feature for WK2 EFL first.
> > 
> > Or, you may be able to add DeviceProximityProviderEfl.cpp | h to WebCore/platform/efl in order to support this functionality both EFL WK1 and WK2.
> 
> I think this is better. I will do this.
> Thanks Gyuyoung.

I checked to move DeviceProximityClientEfl to WebCore/platform/efl, but I think we need to add samrt_callback_call for Device Proximity API.
Therefore, IMHO, WebCoreSupport is right position for this.

In addition, If I add codes for WK2 EFL, patch will be bigger than now.
I will add WK2 EFL First but in this patch, WK1 is more simple to test DeviceProximityController, that's why I added WK1 first.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #10)
> > > Source/WebKit/efl/WebCoreSupport/DeviceProximityClientEfl.h:39
> > > +    virtual ~DeviceProximityClientEfl() { }
> > 
> > Why do you need to add virtual keyword here ? Don't we need to use virtual in child class ?
> 
> You are right but if someone want to make ClientEflXXX which is inherited by DeviceProximityClientEfl.h, it can make a problem. That's why I added virtual keyword here.

I'm almost sure nobody won't inherit DeviceProximityClientEfl again. But, it we need to make child class in future, we can add *virtual* keyword then.

[Gyuyoung Kim]

(In reply to comment #14)

> I checked to move DeviceProximityClientEfl to WebCore/platform/efl, but I think we need to add samrt_callback_call for Device Proximity API.
> Therefore, IMHO, WebCoreSupport is right position for this.
> 
> In addition, If I add codes for WK2 EFL, patch will be bigger than now.
> I will add WK2 EFL First but in this patch, WK1 is more simple to test DeviceProximityController, that's why I added WK1 first.

As you know, EFL port community(Samsung and Intel) are focusing on WK2 port. Even we are trying to use WK2 for product. I really hope to support new functionality for WK2 port first. Is this urgent issue ? I don't think so. I eagerly wanna support this feature for EFL WK2 port first or both WK1 and WK2.

[Gyuyoung Kim]

(In reply to comment #15)
 
> I'm almost sure nobody won't inherit DeviceProximityClientEfl again. But, it we need to make child class in future, we can add *virtual* keyword then.

s/nobody won't/nobody will/g

[Kihong Kwon]

Created attachment 181492 [details]
Patch

[Zan Dobersek]

Comment on attachment 181492 [details]
Patch

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

> Source/WebCore/Target.pri:248
> +    accessibility/AccessibilityImageMapLink.cpp \

This looks unrelated.

[Kihong Kwon]

(In reply to comment #19)
> (From update of attachment 181492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181492&action=review
> 
> > Source/WebCore/Target.pri:248
> > +    accessibility/AccessibilityImageMapLink.cpp \
> 
> This looks unrelated.

You are right. Fixed.

[Kihong Kwon]

Created attachment 181493 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 181493 [details]
Patch

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

I don't wanna object to land dummy implementation for EFL port in this case. But, please file a bug for EFL port implementation soon.

> Source/WebCore/Modules/proximity/DeviceProximityController.h:54
> +    DeviceProximityController(DeviceProximityClient*);

To be honest though *explicit* keyword isn't much meaningful in private ctor, it would be good if you add it for private ctor as well. Inside class may create instance in future.

[Kihong Kwon]

(In reply to comment #22)
> (From update of attachment 181493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181493&action=review
> 
> I don't wanna object to land dummy implementation for EFL port in this case. But, please file a bug for EFL port implementation soon.

This patch is only for DeviceController class, but I will add efl port implementation soon.

> 
> > Source/WebCore/Modules/proximity/DeviceProximityController.h:54
> > +    DeviceProximityController(DeviceProximityClient*);
> 
> To be honest though *explicit* keyword isn't much meaningful in private ctor, it would be good if you add it for private ctor as well. Inside class may create instance in future.

I will do that.

Thanks gyugoung. ;)

[Kihong Kwon]

Created attachment 181633 [details]
Patch

[Hajime Morrita]

Comment on attachment 181633 [details]
Patch

r=me considering Gyuyoung's points are addressed.
@gyuyoung please let me know if you have any opinion.

[Gyuyoung Kim]

(In reply to comment #25)
> (From update of attachment 181633 [details])
> r=me considering Gyuyoung's points are addressed.
> @gyuyoung please let me know if you have any opinion.

LGTM.

[Kihong Kwon]

Comment on attachment 181633 [details]
Patch

Thank you for review, Morrita and Gyuyoung :)

[WebKit Review Bot]

Comment on attachment 181633 [details]
Patch

Clearing flags on attachment: 181633

Committed r139050: <http://trac.webkit.org/changeset/139050>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.