Bug 35446

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.

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.

(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?

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.

Created attachment 50979 [details] Patch 2, addressing the comments and with support for 'all'

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.

Marciej, do you know who would be the right guy to review this patch?

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?

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

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

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

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

http://lists.w3.org/Archives/Public/www-style/2010Mar/0116.html Nice comments.

Created attachment 51345 [details] Patch 3

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.

Simon, could you please have a look?

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.

If OK with you, I will commit it as it is, and fix your comments in follow up patches.

> 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 on attachment 51345 [details] Patch 3 Landed in r56740

Revision r56740 cherry-picked into qtwebkit-2.0 with commit a5c57135aa32a8e883cdb5fe9898bbbdbea5ee5c