Bug 97630

Summary: Add DeviceProximityController to support Device Proximity Events.
Product: WebKit Reporter: Kihong Kwon <kihong.kwon>
Component: WebCore Misc.Assignee: Kihong Kwon <kihong.kwon>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, gyuyoung.kim, mifenton, morrita, rakuco, rwlbuis, tonikitoo, vimff0, webkit.review.bot, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 96894, 104201    
Bug Blocks: 92837    
Attachments:
Description Flags
Initial patch.
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Kihong Kwon 2012-09-25 22:11:35 PDT
Implement DeviceProximityController.
Comment 1 Kihong Kwon 2012-09-25 22:21:33 PDT
Created attachment 165729 [details]
Initial patch.

Initial patch to show how to work DeviceController.
Comment 2 Kihong Kwon 2012-11-30 21:07:26 PST
Created attachment 177079 [details]
Patch
Comment 3 Hajime Morrita 2012-12-02 23:57:38 PST
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.
Comment 4 Kihong Kwon 2012-12-03 03:24:44 PST
> > 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. :)
Comment 5 Kihong Kwon 2012-12-06 02:27:56 PST
Created attachment 177978 [details]
Patch
Comment 6 EFL EWS Bot 2012-12-06 02:43:48 PST
Comment on attachment 177978 [details]
Patch

Attachment 177978 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15151919
Comment 7 Build Bot 2012-12-06 03:09:50 PST
Comment on attachment 177978 [details]
Patch

Attachment 177978 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15173303
Comment 8 Kihong Kwon 2012-12-06 20:38:49 PST
Created attachment 178149 [details]
Patch
Comment 9 Hajime Morrita 2012-12-12 22:09:26 PST
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.
Comment 10 Gyuyoung Kim 2012-12-12 22:20:44 PST
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 ?
Comment 11 Gyuyoung Kim 2012-12-12 22:30:41 PST
(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.
Comment 12 Kihong Kwon 2012-12-12 22:55:31 PST
(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.
Comment 13 Kihong Kwon 2012-12-12 22:56:28 PST
(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.
Comment 14 Kihong Kwon 2012-12-13 17:04:54 PST
(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.
Comment 15 Gyuyoung Kim 2012-12-13 22:47:40 PST
(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.
Comment 16 Gyuyoung Kim 2012-12-13 22:53:30 PST
(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.
Comment 17 Gyuyoung Kim 2012-12-13 23:05:08 PST
(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
Comment 18 Kihong Kwon 2013-01-07 03:48:37 PST
Created attachment 181492 [details]
Patch
Comment 19 Zan Dobersek 2013-01-07 04:19:05 PST
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.
Comment 20 Kihong Kwon 2013-01-07 04:26:14 PST
(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.
Comment 21 Kihong Kwon 2013-01-07 04:29:59 PST
Created attachment 181493 [details]
Patch
Comment 22 Gyuyoung Kim 2013-01-07 04:46:19 PST
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.
Comment 23 Kihong Kwon 2013-01-07 21:21:50 PST
(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. ;)
Comment 24 Kihong Kwon 2013-01-07 21:27:30 PST
Created attachment 181633 [details]
Patch
Comment 25 Hajime Morrita 2013-01-08 00:00:18 PST
Comment on attachment 181633 [details]
Patch

r=me considering Gyuyoung's points are addressed.
@gyuyoung please let me know if you have any opinion.
Comment 26 Gyuyoung Kim 2013-01-08 00:03:01 PST
(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.
Comment 27 Kihong Kwon 2013-01-08 02:58:57 PST
Comment on attachment 181633 [details]
Patch

Thank you for review, Morrita and Gyuyoung :)
Comment 28 WebKit Review Bot 2013-01-08 03:35:44 PST
Comment on attachment 181633 [details]
Patch

Clearing flags on attachment: 181633

Committed r139050: <http://trac.webkit.org/changeset/139050>
Comment 29 WebKit Review Bot 2013-01-08 03:35:52 PST
All reviewed patches have been landed.  Closing bug.