Bug 107820

Summary: [EFL][WK2] Use C API inside ewk_settings
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Jinwoo Song <jinwoo7.song>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, buildbot, bunhere, commit-queue, gyuyoung.kim, gyuyoung.kim, jinwoo7.song, kenneth, kling, lucas.de.marchi, mikhail.pozdnyakov, naginenis, rakuco, rniwa, sergio, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 112794    
Bug Blocks: 107657    
Attachments:
Description Flags
Patch
none
Patch
andersca: review-, andersca: commit-queue-
Patch
none
Patch
none
Patch
benjamin: review-, benjamin: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2013-01-24 06:17:18 PST
As per Bug 107657, we should start using the C API internally to avoid violating layering.
Comment 1 Chris Dumez 2013-01-24 09:57:39 PST
Created attachment 184520 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-01-24 10:36:06 PST
Comment on attachment 184520 [details]
Patch

Nice and there existed C API already for all of them!
Comment 3 Chris Dumez 2013-01-24 10:40:15 PST
(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.
Comment 4 Chris Dumez 2013-01-24 10:50:34 PST
Created attachment 184529 [details]
Patch

Rebase on master.
Comment 5 Mikhail Pozdnyakov 2013-02-01 04:49:57 PST
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.
Comment 6 Kenneth Rohde Christiansen 2013-02-05 13:16:12 PST
Ping review?
Comment 7 Anders Carlsson 2013-02-07 10:52:08 PST
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.
Comment 8 Chris Dumez 2013-02-07 11:03:16 PST
Created attachment 187138 [details]
Patch

Take andersca's feedback into consideration.
Comment 9 EFL EWS Bot 2013-02-07 11:31:46 PST
Comment on attachment 187138 [details]
Patch

Attachment 187138 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16435285
Comment 10 Chris Dumez 2013-02-07 13:05:18 PST
Created attachment 187157 [details]
Patch

Fixed build.
Comment 11 Chris Dumez 2013-02-07 23:32:44 PST
Created attachment 187245 [details]
Patch

Rebase on master due to new conflict in EwkView.
Comment 12 Benjamin Poulain 2013-02-08 00:00:49 PST
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?
Comment 13 Chris Dumez 2013-02-08 00:03:34 PST
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
Comment 14 Benjamin Poulain 2013-02-08 00:11:53 PST
(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?
Comment 15 Chris Dumez 2013-02-08 02:11:40 PST
(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.
Comment 16 Chris Dumez 2013-02-10 05:34:00 PST
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.
Comment 17 Chris Dumez 2013-02-10 05:36:05 PST
Created attachment 187477 [details]
Patch
Comment 18 Mikhail Pozdnyakov 2013-02-10 05:53:32 PST
Comment on attachment 187477 [details]
Patch

LGTM
Comment 19 Benjamin Poulain 2013-02-10 18:14:26 PST
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.
Comment 20 Build Bot 2013-02-11 09:35:25 PST
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
Comment 21 Jinwoo Song 2014-01-22 20:30:19 PST
Hi Chris! Can I continue this bug on behalf of you?
Comment 22 Gyuyoung Kim 2014-01-22 20:31:30 PST
(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.
Comment 23 Jinwoo Song 2014-01-23 21:58:48 PST
Created attachment 222078 [details]
Patch
Comment 24 Gyuyoung Kim 2014-01-23 22:03:15 PST
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.
Comment 25 Jinwoo Song 2014-01-23 22:18:03 PST
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.
Comment 26 Jinwoo Song 2014-03-24 02:30:03 PDT
Created attachment 227632 [details]
Patch

Rebased patch for landing.
Comment 27 WebKit Commit Bot 2014-03-24 03:09:31 PDT
Comment on attachment 227632 [details]
Patch

Clearing flags on attachment: 227632

Committed r166159: <http://trac.webkit.org/changeset/166159>
Comment 28 WebKit Commit Bot 2014-03-24 03:09:44 PDT
All reviewed patches have been landed.  Closing bug.