Bug 95680 – [EFL][WK2] CSS3 Media Queries functionality is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Alexander Shalamov]

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.

[Alexander Shalamov]

Created attachment 161900 [details]
Patch

[Alexander Shalamov]

Comment on attachment 161900 [details]
Patch

Need to unskip media query tests

[Chris Dumez]

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));

[Alexander Shalamov]

Created attachment 161977 [details]
Patch

- Applied fixes according to review comments
- Unskipped failing WK2 layout tests

[Chris Dumez]

Comment on attachment 161977 [details]
Patch

LGTM. Thanks.

[Kenneth Rohde Christiansen]

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?

[Gyuyoung Kim]

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.

[Alexander Shalamov]

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

[Alexander Shalamov]

Created attachment 161986 [details]
Patch_3

- Added comment for case 0:
- w,h => width, height

[Kenneth Rohde Christiansen]

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.

[Alexander Shalamov]

Created attachment 161994 [details]
Patch_4

Fixed comment as proposed by Kenneth.

[WebKit Review Bot]

Comment on attachment 161994 [details]
Patch_4

Clearing flags on attachment: 161994

Committed r127463: <http://trac.webkit.org/changeset/127463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.