Implement DeviceProximityController.
Created attachment 165729 [details] Initial patch. Initial patch to show how to work DeviceController.
Created attachment 177079 [details] Patch
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.
> > 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. :)
Created attachment 177978 [details] Patch
Comment on attachment 177978 [details] Patch Attachment 177978 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15151919
Comment on attachment 177978 [details] Patch Attachment 177978 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15173303
Created attachment 178149 [details] Patch
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 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 ?
(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.
(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.
(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.
(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.
(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.
(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.
(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
Created attachment 181492 [details] Patch
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.
(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.
Created attachment 181493 [details] Patch
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.
(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. ;)
Created attachment 181633 [details] Patch
Comment on attachment 181633 [details] Patch r=me considering Gyuyoung's points are addressed. @gyuyoung please let me know if you have any opinion.
(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 on attachment 181633 [details] Patch Thank you for review, Morrita and Gyuyoung :)
Comment on attachment 181633 [details] Patch Clearing flags on attachment: 181633 Committed r139050: <http://trac.webkit.org/changeset/139050>
All reviewed patches have been landed. Closing bug.