Bug 35446 - Add support for Widgets 1.0: View Mode Media Feature
: Add support for Widgets 1.0: View Mode Media Feature
Status: CLOSED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: PC All
: P3 Normal
Assigned To: Kenneth Rohde Christiansen
:
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-02-26 13:01 PST by Kenneth Rohde Christiansen
Modified: 2010-03-30 04:59 PDT (History)
9 users (show)

See Also:


Attachments
Patch (16.96 KB, patch)
2010-02-26 13:01 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 2, addressing the comments and with support for 'all' (16.95 KB, patch)
2010-03-17 17:32 PDT, Kenneth Rohde Christiansen
simon.fraser: review-
Details | Formatted Diff | Diff
Patch 3 (18.34 KB, patch)
2010-03-22 13:42 PDT, Kenneth Rohde Christiansen
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2010-02-26 13:01:54 PST
Created attachment 49617 [details]
Patch

The patch adds support for the view mode feature: http://www.w3.org/TR/widgets-vmmf/

It is currently only enabled for the Qt port.
Comment 1 WebKit Review Bot 2010-02-26 13:02:52 PST
Attachment 49617 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/MediaQueryEvaluator.cpp:499:  view_modeMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Ignoring "WebKit/qt/Api/qwebpage_p.h": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebpage.cpp": this file is exempt from the style guide.
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Kenneth Rohde Christiansen 2010-02-26 13:03:36 PST
The style check will complain a bit, but I'm following similar style used in
the media feature implementation, to not make it look out of place.
Comment 3 Antonio Gomes 2010-03-16 12:45:06 PDT
(In reply to comment #0)
> Created an attachment (id=49617) [details]
> Patch
> 
> The patch adds support for the view mode feature:
> http://www.w3.org/TR/widgets-vmmf/
> 
> It is currently only enabled for the Qt port.

I was walking through the patch, and a few questions raised:

1) It introduces a build guard (which I think it ok) w/ the name "ENABLE_WEBWIDGET_SUPPORT", however the spec says "Widgets" (note the S in the end). Maybe it could be changed accordingly.

2) Patch adds the term "WEBWIDGET", although the specs say "Widget" (w/o "Web"). What do you think?

3) Do we really need the -webkit prefix as in "-webkit-view-mode: mode" syntax. Is that because the spec is still a draft?

4) Patch introduces 4 possible values (mini, float, application and fullscreen). I just checked the implementation, and there is an additional "all" value, which means:

"The all value is defined to describe a widget for which it is irrelevant how it is displayed. The presentation of the widget is fully dependant on the user agent. This value does not define any actual presentation mode of a widget.

This value is to be used to override the default presentation mode set to floating that would be applied if no value is specified in the document.
"

, which btw I think should be the default value, if none is set by the webpage.

5) If the above is corrent, patch should also reset DRT to "all" after each test execution.

Kenneth, what do you think?
Comment 4 Kenneth Rohde Christiansen 2010-03-16 12:58:12 PDT
Thanks for the comments

> 1) It introduces a build guard (which I think it ok) w/ the name
> "ENABLE_WEBWIDGET_SUPPORT", however the spec says "Widgets" (note the S in the
> end). Maybe it could be changed accordingly.

I guess we could use ENABLE_WIDGETS_10_SUPPORT. I was afraid that people would confuse it with something else. That is why I added the WEB as well.
 
> 2) Patch adds the term "WEBWIDGET", although the specs say "Widget" (w/o
> "Web"). What do you think?

I'm OK with ENABLE_WIDGETS_10_SUPPORT / ENABLE(WIDGETS_10_SUPPORT)

> 
> 3) Do we really need the -webkit prefix as in "-webkit-view-mode: mode" syntax.
> Is that because the spec is still a draft?

We don't. I actually believe it is wrong. -webkit is used for CSS properties as they are ignored by other browser. This is not how it works for media features, as if the browser doesn't know a media feature it defaults to false.

I suggest just using view-mode and not -webkit-view-mode.

> 4) Patch introduces 4 possible values (mini, float, application and
> fullscreen). I just checked the implementation, and there is an additional
> "all" value, which means:

Yes, this was added in the latest spec, and it will need some special treatment.

> 5) If the above is corrent, patch should also reset DRT to "all" after each
> test execution.

No, because that means that you are running as a Widget, and it will always return true. It should only return true for "all" when we are in one of the 4 other modes. For non Widget 1.0 views, it should return false.
Comment 5 Kenneth Rohde Christiansen 2010-03-17 17:32:25 PDT
Created attachment 50979 [details]
Patch 2, addressing the comments and with support for 'all'
Comment 6 WebKit Review Bot 2010-03-17 17:36:23 PDT
Attachment 50979 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/MediaQueryEvaluator.cpp:499:  view_modeMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Kenneth Rohde Christiansen 2010-03-17 17:54:20 PDT
Marciej, do you know who would be the right guy to review this patch?
Comment 8 Simon Hausmann 2010-03-18 08:02:29 PDT
Comment on attachment 50979 [details]
Patch 2, addressing the comments and with support for 'all'

Just a thought, but perhaps the code would be simpler if you stored the view mode in the settings instead of it being queried through a virtual function?
Comment 9 Kenneth Rohde Christiansen 2010-03-18 08:05:38 PDT
(In reply to comment #8)
> (From update of attachment 50979 [details])
> Just a thought, but perhaps the code would be simpler if you stored the view
> mode in the settings instead of it being queried through a virtual function?

It is an option, though it doesn't really feel like a setting, so it might be abusing the setting system for that.
Comment 10 Antonio Gomes 2010-03-18 10:51:03 PDT
(In reply to comment #5)
> Created an attachment (id=50979) [details]
> Patch 2, addressing the comments and with support for 'all'

LGTM now, Kenneth. Thx

Personally I would not dump the render tree but a custom result via layoutTestController.dumpAsText, since it would get rid of any font size possible difference we are comonly seeing between the qt bots and normal linux machines.
Comment 11 Kenneth Rohde Christiansen 2010-03-18 10:53:28 PDT
(In reply to comment #10)
> (In reply to comment #5)
> > Created an attachment (id=50979) [details] [details]
> > Patch 2, addressing the comments and with support for 'all'
> 
> LGTM now, Kenneth. Thx
> 
> Personally I would not dump the render tree but a custom result via
> layoutTestController.dumpAsText, since it would get rid of any font size
> possible difference we are comonly seeing between the qt bots and normal linux
> machines.

That is exactly why I don't use text in the test. It already provides a cross platform result.
Comment 12 Simon Fraser (smfr) 2010-03-22 12:08:05 PDT
Comment on attachment 50979 [details]
Patch 2, addressing the comments and with support for 'all'

I think the view-mode spec is too unstable to implement now:
<http://lists.w3.org/Archives/Public/www-style/2010Mar/0119.html>
We don't want an early, unprefixed implementation in WebKit that we'll have to maintain after the spec changes later.
Comment 13 Simon Fraser (smfr) 2010-03-22 12:14:15 PDT
Comment on attachment 50979 [details]
Patch 2, addressing the comments and with support for 'all'

> diff --git a/WebCore/page/ChromeClient.h b/WebCore/page/ChromeClient.h
> index e4bc8d1..af92af3 100644
> --- a/WebCore/page/ChromeClient.h
> +++ b/WebCore/page/ChromeClient.h
> @@ -231,6 +231,16 @@ namespace WebCore {
>          virtual void needTouchEvents(bool) = 0;
>  #endif
>  
> +#if ENABLE(WIDGETS_10_SUPPORT)
> +        // As the view mode is compared with the result of the media
> +        // feature (view-mode: mode), returning a nulled string equals
> +        // disabling view mode
> +        virtual String webWidgetViewMode()
> +        {
> +            return String();
> +        };
> +#endif

I don't like the way that you're forcing the ChromeClient implement to know all about view modes. I'd prefer it returns an enum, or you have explicit isMinified(), isFullscreen() etc methods.

It's also not at all clear (even in the spec) if these modes are mutually exclusive. You may need to return a bit mask.
Comment 14 Kenneth Rohde Christiansen 2010-03-22 12:21:08 PDT
> I don't like the way that you're forcing the ChromeClient implement to know all
> about view modes. I'd prefer it returns an enum, or you have explicit
> isMinified(), isFullscreen() etc methods.
> 
> It's also not at all clear (even in the spec) if these modes are mutually
> exclusive. You may need to return a bit mask.

Judging from how the spec specifies the view modes, they are mutually exclusive: 

For instance if you are floating you will been to have transparency and no chrome, but if you are an application you should have chrome and not be transparent. I think this just needs some clarification in the spec.
Comment 15 Kenneth Rohde Christiansen 2010-03-22 13:02:48 PDT
http://lists.w3.org/Archives/Public/www-style/2010Mar/0116.html

Nice comments.
Comment 16 Kenneth Rohde Christiansen 2010-03-22 13:42:36 PDT
Created attachment 51345 [details]
Patch 3
Comment 17 WebKit Review Bot 2010-03-22 13:45:40 PDT
Attachment 51345 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/MediaQueryEvaluator.cpp:499:  view_modeMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Kenneth Rohde Christiansen 2010-03-29 13:48:54 PDT
Simon, could you please have a look?
Comment 19 Simon Fraser (smfr) 2010-03-29 14:17:32 PDT
Comment on attachment 51345 [details]
Patch 3

> 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.
Comment 20 Kenneth Rohde Christiansen 2010-03-29 14:26:16 PDT
If OK with you, I will commit it as it is, and fix your comments in follow up patches.
Comment 21 Kenneth Rohde Christiansen 2010-03-29 14:36:21 PDT
> 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.

I don't think the evaluation in order is a problem, as you could do @media (view-mode: mini) and (view-mode: floating).

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

That is a good question. I will think about that.

> Should the string compares be case-insensitive?

I'm not sure; is PX in (min-device-width: 100PX) supposed to be accepted?

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

Describes a widget docked or otherwise minimised, but with a dynamic graphical representation being available nevertheless. 

It is called docket in the Opera implementation: 
http://dev.opera.com/articles/view/opera-widgets-core-dom-reference/

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

I will make additional patches for this.

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

Sounds fair!
Comment 22 Kenneth Rohde Christiansen 2010-03-29 14:45:33 PDT
Comment on attachment 51345 [details]
Patch 3

Landed in r56740
Comment 23 Simon Hausmann 2010-03-30 04:59:08 PDT
Revision r56740 cherry-picked into qtwebkit-2.0 with commit a5c57135aa32a8e883cdb5fe9898bbbdbea5ee5c