[webkit-reviews] review denied: [Bug 102869] [Meta] Supports for Screen Orientation : [Attachment 228239] initial version
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Apr 5 13:49:46 PDT 2014
Benjamin Poulain <benjamin at webkit.org> has denied Ryuan Choi
<ryuan.choi at samsung.com>'s request for review:
Bug 102869: [Meta] Supports for Screen Orientation
https://bugs.webkit.org/show_bug.cgi?id=102869
Attachment 228239: initial version
https://bugs.webkit.org/attachment.cgi?id=228239&action=review
------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=228239&action=review
I only quickly skimmed over the patch, some comments bellow. This needs some
serious cleanup.
> Source/WebCore/ChangeLog:7
> +
Here you should give:
-An URL to the latest spec or draft (preferably the dated version).
-What is the status of support from other engines.
-Explain the architecture of your change.
> Source/WebCore/Modules/screenorientation/ScreenOrientationController.h:46
> + void setClientForTest(ScreenOrientationClient*);
> + bool hasClientForTest() const { return m_hasClientForTest; }
This is not a good way to test features like this.
You should have the mock object in the WebKit layer.
> Source/WebCore/bindings/js/JSScreenCustom.cpp:37
> +JSValue JSScreen::lockOrientation(ExecState* exec)
Why do you need a custom wrapper for this? If it is just for the promise, maybe
we should extend the code generator?
> Source/WebCore/page/Screen.cpp:130
> + static NeverDestroyed<AtomicString> portrait("portrait",
AtomicString::ConstructFromLiteral);
> + static NeverDestroyed<AtomicString> portraitPrimary("portrait-primary",
AtomicString::ConstructFromLiteral);
> + static NeverDestroyed<AtomicString>
portraitSecondary("portrait-secondary", AtomicString::ConstructFromLiteral);
> + static NeverDestroyed<AtomicString> landscape("landscape",
AtomicString::ConstructFromLiteral);
> + static NeverDestroyed<AtomicString>
landscapePrimary("landscape-primary", AtomicString::ConstructFromLiteral);
> + static NeverDestroyed<AtomicString>
landscapeSecondary("landscape-secondary", AtomicString::ConstructFromLiteral);
> + static NeverDestroyed<AtomicString> any("any",
AtomicString::ConstructFromLiteral);
Why do you use AtomicStrings here?
> Source/WebCore/page/Screen.cpp:187
> + for (unsigned i = 0; i < length; ++i) {
> + if (map[i].name == orientationString) {
This is what compile time hash map are for.
> Source/WebCore/page/Screen.h:50
> + enum ScreenOrientation {
If this is an exhaustive list, this should be a typed enum.
More information about the webkit-reviews
mailing list