[webkit-reviews] review denied: [Bug 102869] [Meta] Supports for Screen Orientation : [Attachment 228239] initial version

[bugzilla-daemon at webkit.org]

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.