[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


Hajime Morrita <morrita at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #169829|review?                     |review-
               Flag|                            |




--- Comment #26 from Hajime Morrita <morrita at google.com>  2012-10-22 00:36:36 PST ---
(From update of attachment 169829)
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.

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