Bug 37505 – Enabling view modes to all platforms

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Luiz Agostini]

As implementations may not support certain view modes it is needed to check if a given viewMode is acceptable.

[Luiz Agostini]

Created attachment 53261 [details]
patch 1

[WebKit Review Bot]

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.

[Luiz Agostini]

Created attachment 53263 [details]
patch 2

[Kenneth Rohde Christiansen]

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?

[Luiz Agostini]

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

[Luiz Agostini]

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.

[Eric Seidel (no email)]

Comment on attachment 53263 [details]
patch 2

Clearing r? since this bug has been closed.

[Luiz Agostini]

Now that view mode media feature specification has evolved it may be time to enable it to all platforms.

[Luiz Agostini]

Created attachment 62962 [details]
patch 3

[Luiz Agostini]

Created attachment 62972 [details]
improving changelogs

[Antonio Gomes]

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

[Simon Fraser (smfr)]

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.

[Luiz Agostini]

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?

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

Another way to look at this is that the view mode strings should be AtomicStrings.

[Luiz Agostini]

Created attachment 63105 [details]
patch 4

[Simon Fraser (smfr)]

Comment on attachment 63105 [details]
patch 4

r=me

Please file bugs on each other platform to implement setViewMode().

[WebKit Commit Bot]

Comment on attachment 63105 [details]
patch 4

Clearing flags on attachment: 63105

Committed r64401: <http://trac.webkit.org/changeset/64401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r64401 cherry-picked into qtwebkit-2.1 with commit a4c6465389fd035ad56cc9276c3dd9f00973cdc3

[Simon Hausmann]

Revision r64401 cherry-picked into qtwebkit-2.1 with commit 881f10ac7626a4ad4585035218228d42386b5d19

[Simon Hausmann]

Revision r64401 cherry-picked into qtwebkit-2.1 with commit 4395039b9d85191e4d00e41fb4099a0b94817b33