[webkit-reviews] review granted: [Bug 35446] Add support for Widgets 1.0: View Mode Media Feature : [Attachment 51345] Patch 3

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Kenneth Rohde
Christiansen <kenneth at webkit.org>'s request for review:
Bug 35446: Add support for Widgets 1.0: View Mode Media Feature
https://bugs.webkit.org/show_bug.cgi?id=35446

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/css/MediaQueryEvaluator.cpp
b/WebCore/css/MediaQueryEvaluator.cpp

> +#if ENABLE(WIDGETS_10_SUPPORT)
> +static bool view_modeMediaFeatureEval(CSSValue* value, RenderStyle*, Frame*
frame, MediaFeaturePrefix op)
> +{
> +    if (value) {
> +	   String mode =
static_cast<CSSPrimitiveValue*>(value)->getStringValue();
> +	   if (ChromeClient* client = frame->page()->chrome()->client()) {
> +	       if (mode == "all")
> +		   return true;
> +	       if (mode == "mini" && client->isDocked())
> +		   return true;
> +	       if (mode == "floating" && client->isFloating())
> +		   return true;
> +	       if (mode == "application" && client->isApplication())
> +		   return true;
> +	       if (mode == "fullscreen" && client->isFullscreen())
> +		   return true;
> +	       return false;
> +	   }
> +    }
> +    return false;
> +}

I don't see any words in the spec that say that the modes are evaluated in this
order, and it's not clear that they are mutually exclusive.

Should the "application" mode be assumed it none of the other modes are in
force?

Should the string compares be case-insensitive?

> diff --git a/WebCore/page/ChromeClient.h b/WebCore/page/ChromeClient.h

> +#if ENABLE(WIDGETS_10_SUPPORT)
> +	   virtual bool isDocked() { return false; }

I don't like this name; "docked" has platform-implications. Maybe
"isMimimized()".

> +	   virtual bool isFloating() { return false; }
> +	   virtual bool isApplication() { return false; }
> +	   virtual bool isFullscreen() { return false; }


> diff --git a/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h
b/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h
> index 5cf5104..a562fd3 100644
> --- a/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h
> +++ b/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h
> @@ -138,6 +138,7 @@ public slots:
>      void setMainFrameIsFirstResponder(bool isFirst);
>      void setXSSAuditorEnabled(bool enable);
>      void setCaretBrowsingEnabled(bool enable);
> +    void setViewModeMediaFeature(const QString& mode);
>  
>      bool pauseAnimationAtTimeOnElementWithId(const QString& animationName,
double time, const QString& elementId);
>      bool pauseTransitionAtTimeOnElementWithId(const QString& propertyName,
double time, const QString& elementId);

I'd prefer to see LayoutTestController fixed in the primary platforms as well,
or at least stubbed.

r=me but we reserve the right to modify the -webkit-view-mode behavior as the
spec evolves. If you need a frozen implementation, you'll have to maintain it
in your own source tree.