| Summary: | [WK2][EFL] Move AC code from EwkView to WebView | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||||||
| Component: | WebKit EFL | Assignee: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | cdumez, cmarcelo, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 107657, 107662 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Mikhail Pozdnyakov
2013-02-19 06:43:05 PST
Created attachment 189149 [details]
WIP patch
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? 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? (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. > > 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).
Created attachment 189276 [details]
patch
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.
Comment on attachment 189276 [details]
patch
LGTM, maybe let cmarcelo have a look
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. (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? fine with me Created attachment 189293 [details]
patch v2
Updated due to cmarcelo comment.
Comment on attachment 189293 [details]
patch v2
Anyway to unit test this?
(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. (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. Created attachment 189310 [details]
patch v3
WKViewTranslateUserViewport -> WKViewSetUserViewportTranslation (after talking to cmarcelo on irc)
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.
Created attachment 189313 [details]
patch v4
fixed style nit.
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? (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. Created attachment 189466 [details]
patch v5
Improved code comment.
LGTM, needs someone from WK2 to look at it. 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 Created attachment 189707 [details]
to be landed
Comment on attachment 189707 [details] to be landed Clearing flags on attachment: 189707 Committed r143699: <http://trac.webkit.org/changeset/143699> All reviewed patches have been landed. Closing bug. |