As implementations may not support certain view modes it is needed to check if a given viewMode is acceptable.
Created attachment 53261 [details] patch 1
Attachment 53261 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp" WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:170: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h" Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 53263 [details] patch 2
I heard that Simon Fraser disliked that you moved the methods to Page. Is it possible to modify the patch so that it does not do that?
(In reply to comment #4) > I heard that Simon Fraser disliked that you moved the methods to Page. Is it > possible to modify the patch so that it does not do that? Yes, I had very quick a talk to him in IRC about it. I think that with no event and keeping viewMode in QWebPage as before there is almost nothing to do. If QWebPage keeps the current viewMode value, is the only one that will possibly change it and no event need to be dispatched we just should keep things as they are now. Validation can be done by the user before calling qt_wrt_setViewMode.
If QWebPage keeps the current viewMode value, is the only one that will possibly change it and no event need to be dispatched we just should keep things as they are now. Validation can be done by the user before changing _q_viewMode property.
Comment on attachment 53263 [details] patch 2 Clearing r? since this bug has been closed.
Now that view mode media feature specification has evolved it may be time to enable it to all platforms.
Created attachment 62962 [details] patch 3
Created attachment 62972 [details] improving changelogs
(In reply to comment #10) > Created an attachment (id=62972) [details] > improving changelogs Looks good to me: it cleans up Qt and WebCore related code, as well as removes unneeded DEFINEs and build guards. @smfr, do you have objections?
Comment on attachment 62972 [details] improving changelogs I don't like how the view mode strings used in createViewModesSet(), and passed into setViewMode() from outside have to match. That makes it too easy for platforms to unintentionally diverge. I think I'd prefer: enum ViewMode { ... }; void Page::viewModeChanged(ViewMode) which the platform calls when something changes that affects the view mode. Do the enum -> string mapping in one place.
thanks for the comments and review. :) (In reply to comment #12) > (From update of attachment 62972 [details]) > I don't like how the view mode strings used in createViewModesSet(), and passed into setViewMode() from outside have to match. That makes it too easy for platforms to unintentionally diverge. I think I'd prefer: How would them diverge? Just the specified values are accepted by Page. > > enum ViewMode { > ... > }; > void Page::viewModeChanged(ViewMode) which the platform calls when something changes that affects the view mode. The problem with this approach is the string -> enum mapping that must be used in: 1) in qwebpage because we expect our users to provide view mode strings. 2) in MediaQueryEvaluator because its parameter is a string. 3) in layout tests because a string is received from javascript. Considering this I decided that String would be the most natural format to keep the current view mode value. > Do the enum -> string mapping in one place. Where would be that place?
(In reply to comment #13) > thanks for the comments and review. :) > > (In reply to comment #12) > > (From update of attachment 62972 [details] [details]) > > I don't like how the view mode strings used in createViewModesSet(), and passed into setViewMode() from outside have to match. That makes it too easy for platforms to unintentionally diverge. I think I'd prefer: > > How would them diverge? Just the specified values are accepted by Page. It would be too easy for someone to change the strings in WebCore without knowing, or even being able to change the strings in all clients who pass them in. You might not even have access to the code where the strings originate. > > enum ViewMode { > > ... > > }; > > void Page::viewModeChanged(ViewMode) which the platform calls when something changes that affects the view mode. > > The problem with this approach is the string -> enum mapping that must be used in: > > 1) in qwebpage because we expect our users to provide view mode strings. That's a Qt problem. > 2) in MediaQueryEvaluator because its parameter is a string. You'd call the same string->enum mapping (in Page, perhaps) > 3) in layout tests because a string is received from javascript. At least the layout tests are in open source. > > Do the enum -> string mapping in one place. > > Where would be that place? Page?
Another way to look at this is that the view mode strings should be AtomicStrings.
Created attachment 63105 [details] patch 4
Comment on attachment 63105 [details] patch 4 r=me Please file bugs on each other platform to implement setViewMode().
Comment on attachment 63105 [details] patch 4 Clearing flags on attachment: 63105 Committed r64401: <http://trac.webkit.org/changeset/64401>
All reviewed patches have been landed. Closing bug.
Revision r64401 cherry-picked into qtwebkit-2.1 with commit a4c6465389fd035ad56cc9276c3dd9f00973cdc3
Revision r64401 cherry-picked into qtwebkit-2.1 with commit 881f10ac7626a4ad4585035218228d42386b5d19
Revision r64401 cherry-picked into qtwebkit-2.1 with commit 4395039b9d85191e4d00e41fb4099a0b94817b33