[Webkit-unassigned] [Bug 96894] Add DeviceController base-class to remove duplication of DeviceXXXControler

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96894





--- Comment #27 from Kihong Kwon <kihong.kwon at samsung.com>  2012-10-22 03:19:36 PST ---
(From update of attachment 169829)
View in context: https://bugs.webkit.org/attachment.cgi?id=169829&action=review

Thank you for your comments.

>> Source/WebCore/ChangeLog:8
>> +        Add DeviceController base-class for DeviceOrientationController.
> 
> This is better to be explained as "extracted" . See http://refactoring.com/catalog/extractSuperclass.html

It'will be changed.

>> Source/WebCore/ChangeLog:63
>> +
> 
> Let's have per-file comment where it is appropriate. Apparently this is a big change and there are some non-trivial modificaitons like Document.cpp

It'will be changed.

>> Source/WebCore/dom/DeviceOrientationController.cpp:42
>> +    static_cast<DeviceOrientationClient*>(m_client)->setController(this);
> 
> Let's define deviceOriengationClient()  to encapsulate these down cast.
> Ideally we could have some way to check the underlying client type, but that can be done in separate change.

I will add deviceOrientationClient().
And if we need type checking during implement other features(like device motion events), I will add it.

>> Source/WebCore/dom/Document.cpp:11
>> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> 
> I'd recommend not to do this unless you change the significant part of the file.

OK.

>> Source/WebCore/dom/Document.cpp:-2184
>> -    if (DeviceOrientationController* controller = DeviceOrientationController::from(page()))
> 
> Why can we remove this? That is worth mentioning in the ChangeLog.

I will do that.

>> Source/WebCore/page/DeviceController.cpp:2
>> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> 
> I'd recommend to leave original copyright holder of the original file.

OK.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.