Bug 107820

Created attachment 184520 [details] Patch

Comment on attachment 184520 [details] Patch Nice and there existed C API already for all of them!

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

Created attachment 184529 [details] Patch Rebase on master.

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.

Ping review?

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.

Created attachment 187138 [details] Patch Take andersca's feedback into consideration.

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

Created attachment 187157 [details] Patch Fixed build.

Created attachment 187245 [details] Patch Rebase on master due to new conflict in EwkView.

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

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

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

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.

Created attachment 187477 [details] Patch

Comment on attachment 187477 [details] Patch LGTM

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

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

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

Created attachment 222078 [details] Patch

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

Created attachment 227632 [details] Patch Rebased patch for landing.

Comment on attachment 227632 [details] Patch Clearing flags on attachment: 227632 Committed r166159: <http://trac.webkit.org/changeset/166159>

All reviewed patches have been landed. Closing bug.