Bug 95680 - [EFL][WK2] CSS3 Media Queries functionality is broken
Summary: [EFL][WK2] CSS3 Media Queries functionality is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexander Shalamov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-03 04:46 PDT by Alexander Shalamov
Modified: 2012-09-04 06:30 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.77 KB, patch)
2012-09-03 05:01 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch (8.70 KB, patch)
2012-09-04 00:21 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff
Patch_3 (8.81 KB, patch)
2012-09-04 01:34 PDT, Alexander Shalamov
kenneth: review+
kenneth: commit-queue-
Details | Formatted Diff | Diff
Patch_4 (8.82 KB, patch)
2012-09-04 02:30 PDT, Alexander Shalamov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Shalamov 2012-09-03 04:46:33 PDT
WebCore/platform/PlatformScreenEfl.cpp has implementation that requires access to ewk_view (main evas object) to get evas object properties.
In WK2, WebProcess doesn't have access to that evas object, therefore, methods that get screen rect or screen depth either return incorrect value or crash.

Dependency to evas object should be removed as it is done in other ports.
Comment 1 Alexander Shalamov 2012-09-03 05:01:21 PDT
Created attachment 161900 [details]
Patch
Comment 2 Alexander Shalamov 2012-09-03 05:21:53 PDT
Comment on attachment 161900 [details]
Patch

Need to unskip media query tests
Comment 3 Chris Dumez 2012-09-03 05:30:22 PDT
Comment on attachment 161900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161900&action=review

Can you unskip any test with this fix?

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:54
> +    int error = ecore_x_init(0); \

How about calling it success?

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:55
> +    if (error > 0) { \

if (success) { \

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:63
> +    ecore_x_shutdown(); \

Should be inside the if scope.

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:118
> +    CALL_WITH_ECORE_X(ecore_x_screen_size_get(screen, &w, &h);)

Would be nicer if it would be:
CALL_WITH_ECORE_X(ecore_x_screen_size_get(screen, &w, &h));
Comment 4 Alexander Shalamov 2012-09-04 00:21:25 PDT
Created attachment 161977 [details]
Patch

- Applied fixes according to review comments
- Unskipped failing WK2 layout tests
Comment 5 Chris Dumez 2012-09-04 00:25:39 PDT
Comment on attachment 161977 [details]
Patch

LGTM. Thanks.
Comment 6 Kenneth Rohde Christiansen 2012-09-04 00:35:33 PDT
Comment on attachment 161977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161977&action=review

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:98
> +    switch (depth) {
> +    case 0:

comment about this case?

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:118
> +    CALL_WITH_ECORE_X(ecore_x_screen_size_get(screen, &w, &h));

isn't it "expensive" initializing and shutting ecore_x down? no caching?
Comment 7 Gyuyoung Kim 2012-09-04 01:02:54 PDT
Comment on attachment 161977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161977&action=review

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:117
> +    int w = 0, h = 0;

Please use "width and height" instead of abbreviation.
Comment 8 Alexander Shalamov 2012-09-04 01:18:04 PDT
(In reply to comment #6)
> (From update of attachment 161977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161977&action=review
> 
> > Source/WebCore/platform/efl/PlatformScreenEfl.cpp:98
> > +    switch (depth) {
> > +    case 0:
> 
> comment about this case?

I was thinking about case when we get 0 from ecore_x_default_depth_get.
If that case is removed, default case would be hit and 0 will be returned.

I can remove it if you think it is not required.

> 
> > Source/WebCore/platform/efl/PlatformScreenEfl.cpp:118
> > +    CALL_WITH_ECORE_X(ecore_x_screen_size_get(screen, &w, &h));
> 
> isn't it "expensive" initializing and shutting ecore_x down? no caching?

It is cached inside ecore_x (ref counted).
If ecore_x_init was called before, all consecutive init / shutdown calls will increment and decrement ref count.
In efl wk1 and wk2, we call ecore_x_init at startup of the process.

If X is not running or DISPLAY variable is unset, ecore_x init will fail immediately and we will not call any ecore_x function.
Comment 9 Alexander Shalamov 2012-09-04 01:34:42 PDT
Created attachment 161986 [details]
Patch_3

- Added comment for case 0:
- w,h => width, height
Comment 10 Kenneth Rohde Christiansen 2012-09-04 01:47:13 PDT
Comment on attachment 161986 [details]
Patch_3

View in context: https://bugs.webkit.org/attachment.cgi?id=161986&action=review

> Source/WebCore/platform/efl/PlatformScreenEfl.cpp:97
> +    // In case when 0 is returned by screenDepth, fallback to 8 bit per component

End comments with punctuation mark..

// Special tread 0 as an error, and return 8 bit per component.
Comment 11 Alexander Shalamov 2012-09-04 02:30:09 PDT
Created attachment 161994 [details]
Patch_4

Fixed comment as proposed by Kenneth.
Comment 12 WebKit Review Bot 2012-09-04 06:30:11 PDT
Comment on attachment 161994 [details]
Patch_4

Clearing flags on attachment: 161994

Committed r127463: <http://trac.webkit.org/changeset/127463>
Comment 13 WebKit Review Bot 2012-09-04 06:30:16 PDT
All reviewed patches have been landed.  Closing bug.