Bug 110216 – [WK2][EFL] Move AC code from EwkView to WebView

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

[Mikhail Pozdnyakov]

SSIA. The goal is to stop direct accessing to coord graphics scene from EwkView.

[Mikhail Pozdnyakov]

Created attachment 189149 [details]
WIP patch

[Kenneth Rohde Christiansen]

Comment on attachment 189149 [details]
WIP patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:61
> +void WKViewSetUserViewportTransformation(WKViewRef viewRef, const WKViewMatrix* userViewportTransformation)

I think "transformation" would be fine here...

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:63
> +    WebCore::TransformationMatrix transform(userViewportTransformation->xx, userViewportTransformation->yx,

I could just call it matrix

matrix(transformation->xx,...

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:78
> +WK_EXPORT void WKViewSetUserViewportTransformation(WKViewRef, const WKViewMatrix* userViewportTransformation);

no need to add userView...

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:269
> -    if (behavior == DefaultBehavior) {
> -        coordinatedGraphicsScene()->setActive(true);
> -        WKPageSetUseFixedLayout(wkPage(), true);
> -    }
> +    WKPageSetUseFixedLayout(wkPage(), behavior == DefaultBehavior);

Are you sure it should not be active?

[Kenneth Rohde Christiansen]

Comment on attachment 189149 [details]
WIP patch

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

> Source/WebKit2/UIProcess/efl/WebView.cpp:109
> +    RefPtr<cairo_t> graphicsContext = adoptRef(cairo_create(surface));
> +
> +    const FloatPoint& pagePosition = m_ewkView->pagePosition();
> +    cairo_translate(graphicsContext.get(), - pagePosition.x(), - pagePosition.y());
> +    cairo_scale(graphicsContext.get(), m_ewkView->pageScaleFactor(), m_ewkView->pageScaleFactor());
> +    cairo_scale(graphicsContext.get(), m_page->deviceScaleFactor(), m_page->deviceScaleFactor());

can we do this smarter with a matrix instead? like one operation?

> Source/WebKit2/UIProcess/efl/WebView.cpp:218
> +    WebKit::CoordinatedLayerTreeHostProxy* coordinatedLayerTreeHostProxy = drawingArea->coordinatedLayerTreeHostProxy();
> +    if (!coordinatedLayerTreeHostProxy)

wouldnt calling it laterTreeHostProxy not be sufficient

> Source/WebKit2/UIProcess/efl/WebView.h:93
> +    // FIXME: Should be private.
> +    WebCore::AffineTransform transformFromScene() const;
> +    WebCore::AffineTransform transformToScene() const;
> +

why are they not?

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 189149 [details])
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:269
> > -    if (behavior == DefaultBehavior) {
> > -        coordinatedGraphicsScene()->setActive(true);
> > -        WKPageSetUseFixedLayout(wkPage(), true);
> > -    }
> > +    WKPageSetUseFixedLayout(wkPage(), behavior == DefaultBehavior);
> 
> Are you sure it should not be active?

It is set in WebView::initialize() now.

[Mikhail Pozdnyakov]

> > Source/WebKit2/UIProcess/efl/WebView.h:93
> > +    // FIXME: Should be private.
> > +    WebCore::AffineTransform transformFromScene() const;
> > +    WebCore::AffineTransform transformToScene() const;
> > +
> 
> why are they not?

Unfortunately they are used in EwkView so far :/ (until we move event handling to web view where it should be).

[Mikhail Pozdnyakov]

Created attachment 189276 [details]
patch

[WebKit Review Bot]

Attachment 189276 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/C/efl/WKView.cpp', u'Source/WebKit2/UIProcess/API/C/efl/WKView.h', u'Source/WebKit2/UIProcess/API/efl/EwkView.cpp', u'Source/WebKit2/UIProcess/API/efl/EwkView.h', u'Source/WebKit2/UIProcess/efl/WebView.cpp', u'Source/WebKit2/UIProcess/efl/WebView.h']" exit_code: 1
Source/WebKit2/UIProcess/API/C/efl/WKView.h:42:  More than one command on the same line  [whitespace/newline] [4]
Source/WebKit2/UIProcess/API/C/efl/WKView.h:43:  More than one command on the same line  [whitespace/newline] [4]
Source/WebKit2/UIProcess/API/C/efl/WKView.h:44:  More than one command on the same line  [whitespace/newline] [4]
Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 189276 [details]
patch

LGTM, maybe let cmarcelo have a look

[Caio Marcelo de Oliveira Filho]

Comment on attachment 189276 [details]
patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:73
> +WK_EXPORT void WKViewSetUserViewportTransformation(WKViewRef, const WKViewMatrix* transformation);
> +WK_EXPORT WKPoint WKViewUserViewportToContents(WKViewRef, WKPoint);

Question: does arbitrary matrices work correctly here? What about start smaller and support vertical and horizontal displacement and then we change to use matrices when we have means to test this? That would spare us having this WKViewMatrix type for now.

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 189276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189276&action=review
> 
> > Source/WebKit2/UIProcess/API/C/efl/WKView.h:73
> > +WK_EXPORT void WKViewSetUserViewportTransformation(WKViewRef, const WKViewMatrix* transformation);
> > +WK_EXPORT WKPoint WKViewUserViewportToContents(WKViewRef, WKPoint);
> 
> Question: does arbitrary matrices work correctly here? What about start smaller and support vertical and horizontal displacement and then we change to use matrices when we have means to test this? That would spare us having this WKViewMatrix type for now.

makes sense to me, having just viewport translation api looks sufficient at the moment. Kenneth, what do you think?

[Kenneth Rohde Christiansen]

fine with me

[Mikhail Pozdnyakov]

Created attachment 189293 [details]
patch v2

Updated due to cmarcelo comment.

[Kenneth Rohde Christiansen]

Comment on attachment 189293 [details]
patch v2

Anyway to unit test this?

[Mikhail Pozdnyakov]

(In reply to comment #13)
> (From update of attachment 189293 [details])
> Anyway to unit test this?

WKView is now used as middle layer for ewk_view, so WKView's functionality is covered by ewk tests.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 189293 [details] [details])
> > Anyway to unit test this?
> 
> WKView is now used as middle layer for ewk_view, so WKView's functionality is covered by ewk tests.

In the future we should import the API tests we created for NIXView, but renaming to WKView. They are in our github branch inside Tools/TestWebKitAPI/Tests/nix.

[Mikhail Pozdnyakov]

Created attachment 189310 [details]
patch v3

WKViewTranslateUserViewport -> WKViewSetUserViewportTranslation (after talking to cmarcelo on irc)

[WebKit Review Bot]

Attachment 189310 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/C/efl/WKView.cpp', u'Source/WebKit2/UIProcess/API/C/efl/WKView.h', u'Source/WebKit2/UIProcess/API/efl/EwkView.cpp', u'Source/WebKit2/UIProcess/API/efl/EwkView.h', u'Source/WebKit2/UIProcess/efl/WebView.cpp', u'Source/WebKit2/UIProcess/efl/WebView.h']" exit_code: 1
Source/WebKit2/UIProcess/efl/WebView.cpp:227:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikhail Pozdnyakov]

Created attachment 189313 [details]
patch v4

fixed style nit.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 189313 [details]
patch v4

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

LGTM with comments.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:269
> -    if (behavior == DefaultBehavior) {
> -        coordinatedGraphicsScene()->setActive(true);
> -        WKPageSetUseFixedLayout(wkPage(), true);
> -    }
> +    WKPageSetUseFixedLayout(wkPage(), behavior == DefaultBehavior);

Do we need to set use fixed layout when behavior != DefaultBehavior?

> Source/WebKit2/UIProcess/efl/WebView.h:97
> +    // FIXME: Should be private.

Explain here what is preventing to become private, and how we fix. It seems to me they are not private yet for the sake of creating Events, which will be moved later on to WKView too. Is that correct?

[Mikhail Pozdnyakov]

(In reply to comment #19)
> (From update of attachment 189313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189313&action=review
> 
> LGTM with comments.
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:269
> > -    if (behavior == DefaultBehavior) {
> > -        coordinatedGraphicsScene()->setActive(true);
> > -        WKPageSetUseFixedLayout(wkPage(), true);
> > -    }
> > +    WKPageSetUseFixedLayout(wkPage(), behavior == DefaultBehavior);
> 
> Do we need to set use fixed layout when behavior != DefaultBehavior?
In principal we should not, as web page does not use fixed layout by default.

However WKPageSetUseFixedLayout(wkPage(), behavior == DefaultBehavior); does not hurt because WebPageProxy::setUseFixedLayout checks whether the current value is equal to the new one and returns if it is.

I prefer WKPageSetUseFixedLayout(wkPage(), behavior == DefaultBehavior); to 

if (behavior == DefaultBehavior)
    WKPageSetUseFixedLayout(wkPage(), true);

for the sake of code compactness.

> 
> > Source/WebKit2/UIProcess/efl/WebView.h:97
> > +    // FIXME: Should be private.
> 
> Explain here what is preventing to become private, and how we fix. It seems to me they are not private yet for the sake of creating Events, which will be moved later on to WKView too. Is that correct?

Absolutely correct. I'll improve the comment.

[Mikhail Pozdnyakov]

Created attachment 189466 [details]
patch v5

Improved code comment.

[Caio Marcelo de Oliveira Filho]

LGTM, needs someone from WK2 to look at it.

[WebKit Review Bot]

Comment on attachment 189466 [details]
patch v5

Rejecting attachment 189466 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 189466, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
nk #5 succeeded at 413 (offset -3 lines).
patching file Source/WebKit2/UIProcess/efl/WebView.h
Hunk #1 succeeded at 31 with fuzz 1.
Hunk #2 succeeded at 55 (offset -3 lines).
Hunk #3 succeeded at 91 (offset -3 lines).
Hunk #4 FAILED at 181.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/efl/WebView.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Anders Carlsson']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16696169

[Mikhail Pozdnyakov]

Created attachment 189707 [details]
to be landed

[WebKit Review Bot]

Comment on attachment 189707 [details]
to be landed

Clearing flags on attachment: 189707

Committed r143699: <http://trac.webkit.org/changeset/143699>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.