Bug 37505 - Enabling view modes to all platforms
Summary: Enabling view modes to all platforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Luiz Agostini
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-13 10:06 PDT by Luiz Agostini
Modified: 2010-08-24 06:36 PDT (History)
7 users (show)

See Also:


Attachments
patch 1 (27.23 KB, patch)
2010-04-13 10:45 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 2 (27.23 KB, patch)
2010-04-13 11:01 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 3 (20.41 KB, patch)
2010-07-29 09:38 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
improving changelogs (21.07 KB, patch)
2010-07-29 11:21 PDT, Luiz Agostini
simon.fraser: review-
Details | Formatted Diff | Diff
patch 4 (21.55 KB, patch)
2010-07-30 14:55 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luiz Agostini 2010-04-13 10:06:36 PDT
As implementations may not support certain view modes it is needed to check if a given viewMode is acceptable.
Comment 1 Luiz Agostini 2010-04-13 10:45:49 PDT
Created attachment 53261 [details]
patch 1
Comment 2 WebKit Review Bot 2010-04-13 10:49:12 PDT
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.
Comment 3 Luiz Agostini 2010-04-13 11:01:38 PDT
Created attachment 53263 [details]
patch 2
Comment 4 Kenneth Rohde Christiansen 2010-04-14 13:13:08 PDT
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?
Comment 5 Luiz Agostini 2010-04-14 14:43:39 PDT
(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.
Comment 6 Luiz Agostini 2010-05-07 14:42:59 PDT
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 7 Eric Seidel (no email) 2010-05-09 15:28:18 PDT
Comment on attachment 53263 [details]
patch 2

Clearing r? since this bug has been closed.
Comment 8 Luiz Agostini 2010-07-23 11:39:29 PDT
Now that view mode media feature specification has evolved it may be time to enable it to all platforms.
Comment 9 Luiz Agostini 2010-07-29 09:38:08 PDT
Created attachment 62962 [details]
patch 3
Comment 10 Luiz Agostini 2010-07-29 11:21:25 PDT
Created attachment 62972 [details]
improving changelogs
Comment 11 Antonio Gomes 2010-07-29 12:30:53 PDT
(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 12 Simon Fraser (smfr) 2010-07-29 13:22:25 PDT
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.
Comment 13 Luiz Agostini 2010-07-29 13:42:29 PDT
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?
Comment 14 Simon Fraser (smfr) 2010-07-29 13:49:42 PDT
(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?
Comment 15 Simon Fraser (smfr) 2010-07-29 13:50:27 PDT
Another way to look at this is that the view mode strings should be AtomicStrings.
Comment 16 Luiz Agostini 2010-07-30 14:55:00 PDT
Created attachment 63105 [details]
patch 4
Comment 17 Simon Fraser (smfr) 2010-07-30 15:06:41 PDT
Comment on attachment 63105 [details]
patch 4

r=me

Please file bugs on each other platform to implement setViewMode().
Comment 18 WebKit Commit Bot 2010-07-30 19:46:28 PDT
Comment on attachment 63105 [details]
patch 4

Clearing flags on attachment: 63105

Committed r64401: <http://trac.webkit.org/changeset/64401>
Comment 19 WebKit Commit Bot 2010-07-30 19:46:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Simon Hausmann 2010-08-10 08:06:09 PDT
Revision r64401 cherry-picked into qtwebkit-2.1 with commit a4c6465389fd035ad56cc9276c3dd9f00973cdc3
Comment 21 Simon Hausmann 2010-08-11 00:36:11 PDT
Revision r64401 cherry-picked into qtwebkit-2.1 with commit 881f10ac7626a4ad4585035218228d42386b5d19
Comment 22 Simon Hausmann 2010-08-12 03:34:05 PDT
Revision r64401 cherry-picked into qtwebkit-2.1 with commit 4395039b9d85191e4d00e41fb4099a0b94817b33