[webkit-reviews] review denied: [Bug 96894] Add DeviceController base-class to remove duplication of DeviceXXXControler : [Attachment 169829] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 96894: Add DeviceController base-class to remove duplication of
DeviceXXXControler
https://bugs.webkit.org/show_bug.cgi?id=96894

Attachment 169829: Patch
https://bugs.webkit.org/attachment.cgi?id=169829&action=review

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=169829&action=review


Almost looks good. I added some small points.

> 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

> 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

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

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

> Source/WebCore/dom/Document.cpp:-2184
> -    if (DeviceOrientationController* controller =
DeviceOrientationController::from(page()))

Why can we remove this? That is worth mentioning in the ChangeLog.

> Source/WebCore/dom/Document.cpp:-2200
> -    if (DeviceOrientationController* controller =
DeviceOrientationController::from(page()))

Ditto.

> Source/WebCore/page/DOMWindow.cpp:4
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

Ditto.

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

> Source/WebCore/page/DeviceController.h:2
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

Ditto.