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
: WebKit
New Bugs
: 528+ (Nightly build)
: PC All
: P3 Normal
Assigned To:
:
:
:
: 35784
  Show dependency treegraph
 
Reported: 2010-02-26 13:01 PST by
Modified: 2010-03-30 04:59 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-02-26 13:01:54 PST
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.
------- Comment #1 From 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 From 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 From 2010-03-16 12:45:06 PST -------
(In reply to comment #0)
> Created an attachment (id=49617) [details] [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 From 2010-03-16 12:58:12 PST -------
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 From 2010-03-17 17:32:25 PST -------
Created an attachment (id=50979) [details]
Patch 2, addressing the comments and with support for 'all'
------- Comment #6 From 2010-03-17 17:36:23 PST -------
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 From 2010-03-17 17:54:20 PST -------
Marciej, do you know who would be the right guy to review this patch?
------- Comment #8 From 2010-03-18 08:02:29 PST -------
(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?
------- Comment #9 From 2010-03-18 08:05:38 PST -------
(In reply to comment #8)
> (From update of attachment 50979 [details] [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 From 2010-03-18 10:51:03 PST -------
(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.
------- Comment #11 From 2010-03-18 10:53:28 PST -------
(In reply to comment #10)
> (In reply to comment #5)
> > Created an attachment (id=50979) [details] [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 From 2010-03-22 12:08:05 PST -------
(From update of attachment 50979 [details])
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 From 2010-03-22 12:14:15 PST -------
(From update of attachment 50979 [details])
> 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 From 2010-03-22 12:21:08 PST -------
> 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 From 2010-03-22 13:02:48 PST -------
http://lists.w3.org/Archives/Public/www-style/2010Mar/0116.html

Nice comments.
------- Comment #16 From 2010-03-22 13:42:36 PST -------
Created an attachment (id=51345) [details]
Patch 3
------- Comment #17 From 2010-03-22 13:45:40 PST -------
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 From 2010-03-29 13:48:54 PST -------
Simon, could you please have a look?
------- Comment #19 From 2010-03-29 14:17:32 PST -------
(From update of attachment 51345 [details])
> 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 From 2010-03-29 14:26:16 PST -------
If OK with you, I will commit it as it is, and fix your comments in follow up patches.
------- Comment #21 From 2010-03-29 14:36:21 PST -------
> 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 From 2010-03-29 14:45:33 PST -------
(From update of attachment 51345 [details])
Landed in r56740
------- Comment #23 From 2010-03-30 04:59:08 PST -------
Revision r56740 cherry-picked into qtwebkit-2.0 with commit a5c57135aa32a8e883cdb5fe9898bbbdbea5ee5c