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.
Created attachment 161900 [details] Patch
Comment on attachment 161900 [details] Patch Need to unskip media query tests
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));
Created attachment 161977 [details] Patch - Applied fixes according to review comments - Unskipped failing WK2 layout tests
Comment on attachment 161977 [details] Patch LGTM. Thanks.
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 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.
(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.
Created attachment 161986 [details] Patch_3 - Added comment for case 0: - w,h => width, height
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.
Created attachment 161994 [details] Patch_4 Fixed comment as proposed by Kenneth.
Comment on attachment 161994 [details] Patch_4 Clearing flags on attachment: 161994 Committed r127463: <http://trac.webkit.org/changeset/127463>
All reviewed patches have been landed. Closing bug.