Bug 107820 – [EFL][WK2] Use C API inside ewk_settings

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

[Chris Dumez]

As per Bug 107657, we should start using the C API internally to avoid violating layering.

[Chris Dumez]

Created attachment 184520 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 184520 [details]
Patch

Nice and there existed C API already for all of them!

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 184520 [details])
> Nice and there existed C API already for all of them!

Almost all of them. We will have to decide what to do about the others later.

[Chris Dumez]

Created attachment 184529 [details]
Patch

Rebase on master.

[Mikhail Pozdnyakov]

Comment on attachment 184529 [details]
Patch

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

LGTM overall.

> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:49
> +    inline WKPreferencesRef preferences();

would prefer having method definition here (it's just 1 line), that would make it inline automatically.

[Kenneth Rohde Christiansen]

Ping review?

[Anders Carlsson]

Comment on attachment 184529 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:49
>> +    inline WKPreferencesRef preferences();
> 
> would prefer having method definition here (it's just 1 line), that would make it inline automatically.

Putting inline on a member function that is not defined in its header file (or a header file) is weird. Please either remove the inline or make the function actually be inline.

[Chris Dumez]

Created attachment 187138 [details]
Patch

Take andersca's feedback into consideration.

[EFL EWS Bot]

Comment on attachment 187138 [details]
Patch

Attachment 187138 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16435285

[Chris Dumez]

Created attachment 187157 [details]
Patch

Fixed build.

[Chris Dumez]

Created attachment 187245 [details]
Patch

Rebase on master due to new conflict in EwkView.

[Benjamin Poulain]

Comment on attachment 187245 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:241
> -    , m_settings(EwkSettings::create(this))
> +    , m_settings(EwkSettings::create(WKPageGroupGetPreferences(WKPageGetPageGroup(m_webView->pageRef()))))

I don't get this. You get the PageGroup as an argument of the constructor, yet you extract (an other?) PageGroup from m_webView?
Why not construct the attribute from your parameters?

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:99
> +
> +    return WKPreferencesGetFullScreenEnabled(const_cast<Ewk_Settings*>(settings)->preferences());

Why not make preferences() const instead of const-cast every access?

> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:49
> +    inline WKPreferencesRef preferences() {  return m_preferences.get(); }

You can remove the inline keyword, it is redundant. Make this const?

[Chris Dumez]

Comment on attachment 187245 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:241
>> +    , m_settings(EwkSettings::create(WKPageGroupGetPreferences(WKPageGetPageGroup(m_webView->pageRef()))))
> 
> I don't get this. You get the PageGroup as an argument of the constructor, yet you extract (an other?) PageGroup from m_webView?
> Why not construct the attribute from your parameters?

Because the pagegroup passed to the constructor may be NULL. Retrieving it from the WebPage / WebPageProxy makes sure that it is constructed / valid.

>> Source/WebKit2/UIProcess/API/efl/ewk_settings_private.h:49
>> +    inline WKPreferencesRef preferences() {  return m_preferences.get(); }
> 
> You can remove the inline keyword, it is redundant. Make this const?

Ok

[Benjamin Poulain]

(In reply to comment #13)
> > I don't get this. You get the PageGroup as an argument of the constructor, yet you extract (an other?) PageGroup from m_webView?
> > Why not construct the attribute from your parameters?
> 
> Because the pagegroup passed to the constructor may be NULL. Retrieving it from the WebPage / WebPageProxy makes sure that it is constructed / valid.

Having null opaque objects is something I almost only see in the EFL port.

How does that happen?

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #13)
> > > I don't get this. You get the PageGroup as an argument of the constructor, yet you extract (an other?) PageGroup from m_webView?
> > > Why not construct the attribute from your parameters?
> > 
> > Because the pagegroup passed to the constructor may be NULL. Retrieving it from the WebPage / WebPageProxy makes sure that it is constructed / valid.
> 
> Having null opaque objects is something I almost only see in the EFL port.
> 
> How does that happen?

This is called directly from:
WKViewRef WKViewCreate(Evas* canvas, WKContextRef contextRef, WKPageGroupRef pageGroupRef)

In that C API, pageGroupRef may be NULL if we wish the view to create its own new page group.

[Chris Dumez]

Created attachment 187476 [details]
Patch

Take Benjamin's comment into consideration. I opened Bug 109276 to address the issue about the WKPageGroupRef being NULL. For now, this patch still retrieves the page group from the page to make sure it is not NULL. This can be improved if Bug 109276 lands.

[Chris Dumez]

Created attachment 187477 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 187477 [details]
Patch

LGTM

[Benjamin Poulain]

Comment on attachment 187477 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:241
> -    , m_settings(EwkSettings::create(this))
> +    , m_settings(EwkSettings::create(WKPageGroupGetPreferences(WKPageGetPageGroup(wkPage()))))

LGTM, but let's fix the dependencies first in your other patch before we go ahead.
Then you can fix this line and land.

[Build Bot]

Comment on attachment 187477 [details]
Patch

Attachment 187477 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16497253

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Jinwoo Song]

Hi Chris! Can I continue this bug on behalf of you?

[Gyuyoung Kim]

(In reply to comment #21)
> Hi Chris! Can I continue this bug on behalf of you?

It seems to me Chris isn't interested in this issue nowadays.

[Jinwoo Song]

Created attachment 222078 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 222078 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:278
> +    , m_settings(std::make_unique<EwkSettings>(WKPageGroupGetPreferences(WKPageGetPageGroup(wkPage()))))

LGTM, however, Benjamin might have a concern about this.

[Jinwoo Song]

Comment on attachment 222078 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:278
>> +    , m_settings(std::make_unique<EwkSettings>(WKPageGroupGetPreferences(WKPageGetPageGroup(wkPage()))))
> 
> LGTM, however, Benjamin might have a concern about this.

In r159896, WebPageGroup is never null, so it seems to be okay but needs confirm from Benjamin.

[Jinwoo Song]

Created attachment 227632 [details]
Patch

Rebased patch for landing.

[WebKit Commit Bot]

Comment on attachment 227632 [details]
Patch

Clearing flags on attachment: 227632

Committed r166159: <http://trac.webkit.org/changeset/166159>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.