Bug 107631

Created attachment 184166 [details] Patch Initial patch

Created attachment 192164 [details] Patch Rebased.

Comment on attachment 192164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192164&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:172 > +void WKViewFindZoomableAreaForPoint(WKViewRef viewRef, WKPoint point, WKSize size) What is size here? Maybe this should be more similar to touch points... > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1331 > > +void EwkView::didFindZoomableArea(const WKPoint& point, const WKRect& area) > +{ > + m_pageViewportControllerClient->scaleToAreaWithAnimation(toIntPoint(point), toIntRect(area), 5); It seems this could be split into two patches... API and implementation for the EwkView > Source/WebKit2/UIProcess/API/efl/TapGestureHandler.cpp:131 > void TapGestureHandler::processDoubleTap() > { > - notImplemented(); > + IntPoint position = m_ewkView->webView()->transformFromScene().mapPoint(m_ewkView->transformToScreen().inverse().mapPoint(m_secondPressedPoint)); It seems this depends on another patch > Source/WebKit2/UIProcess/PageViewportController.cpp:262 > m_contentsPosition = position; > - if (!m_pendingScaleChange) > - applyScaleAfterRenderingContents(scale); > + applyScaleAfterRenderingContents(scale); > You need bbandix to approve this change > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:94 > +{ > + m_animator = ecore_animator_add(animatorCallback, this); > + m_scaleDuration = 1 / ecore_animator_frametime_get() / 3; Does this bring us any advantage (as we are using coord. graphics) in contract to using a webcore timer? It might do, maybe you should discuss with dshuang or noamr

Comment on attachment 192164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192164&action=review Thanks for your comments :) >> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:172 >> +void WKViewFindZoomableAreaForPoint(WKViewRef viewRef, WKPoint point, WKSize size) > > What is size here? Maybe this should be more similar to touch points... The size means the area of point, that is needed because we can touch using finger. The point and size can be rectangle, so I will modify parameters. >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1331 >> + m_pageViewportControllerClient->scaleToAreaWithAnimation(toIntPoint(point), toIntRect(area), 5); > > It seems this could be split into two patches... API and implementation for the EwkView Yes, I will separate patch. >> Source/WebKit2/UIProcess/API/efl/TapGestureHandler.cpp:131 >> + IntPoint position = m_ewkView->webView()->transformFromScene().mapPoint(m_ewkView->transformToScreen().inverse().mapPoint(m_secondPressedPoint)); > > It seems this depends on another patch Yes, that's wrong code, I will modify that. >> Source/WebKit2/UIProcess/PageViewportController.cpp:262 >> > > You need bbandix to approve this change OK, thanks. >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:94 >> + m_scaleDuration = 1 / ecore_animator_frametime_get() / 3; > > Does this bring us any advantage (as we are using coord. graphics) in contract to using a webcore timer? It might do, maybe you should discuss with dshuang or noamr The ecore_animator is useful because it is running depend on settings of Ecore. That means, all ecore_animator of some applications can run with same duration and tick. So, I think ecore_animator is better than webcore timer for EFL.

Comment on attachment 192164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192164&action=review > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:43 > + , m_transition(new Transition(this)) Is it leaking? OwnPtr might help > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:93 > + m_animator = ecore_animator_add(animatorCallback, this); where is ecore_animator_del() ? >>> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:94 >>> + m_scaleDuration = 1 / ecore_animator_frametime_get() / 3; >> >> Does this bring us any advantage (as we are using coord. graphics) in contract to using a webcore timer? It might do, maybe you should discuss with dshuang or noamr > > The ecore_animator is useful because it is running depend on settings of Ecore. > That means, all ecore_animator of some applications can run with same duration and tick. > So, I think ecore_animator is better than webcore timer for EFL. maybe it is possible to make it more readable than "1 / ecore_animator_frametime_get() / 3" ? > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.h:62 > + class Transition { What is the reason for it to be nested type? > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.h:64 > + Transition(PageViewportControllerClientEfl* parent) if it is a parent there should be a child. Think it's just a viewportController. > Source/WebKit2/UIProcess/efl/ViewClientEfl.h:53 > + static void didFindZoomableArea(WKViewRef, WKPoint, WKRect, const void* clientInfo); const WKPoint& const WKRect&

Comment on attachment 192164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192164&action=review The patch is better to be split in order to make review easier. > Source/WebKit2/UIProcess/PageViewportController.cpp:261 > + applyScaleAfterRenderingContents(scale); Why? as far as I remember (I might be wrong) that would lead to scaling dead loop. Anyway must be tested carefully.

Comment on attachment 192164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192164&action=review I will separate this patch into several patches, and apply comments to the each patch. >> Source/WebKit2/UIProcess/PageViewportController.cpp:261 >> + applyScaleAfterRenderingContents(scale); > > Why? as far as I remember (I might be wrong) that would lead to scaling dead loop. Anyway must be tested carefully. Without above patch, I can not animate the scaling because m_pendingScaleChange is set to true for first iteration of animation, so I need that code. I didn't see the dead loop yet, but I will test scaling more and corefully. >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:43 >> + , m_transition(new Transition(this)) > > Is it leaking? OwnPtr might help thanks, I will modify. >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:93 >> + m_animator = ecore_animator_add(animatorCallback, this); > > where is ecore_animator_del() ? I have to add that. >>>> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:94 >>>> + m_scaleDuration = 1 / ecore_animator_frametime_get() / 3; >>> >>> Does this bring us any advantage (as we are using coord. graphics) in contract to using a webcore timer? It might do, maybe you should discuss with dshuang or noamr >> >> The ecore_animator is useful because it is running depend on settings of Ecore. >> That means, all ecore_animator of some applications can run with same duration and tick. >> So, I think ecore_animator is better than webcore timer for EFL. > > maybe it is possible to make it more readable than "1 / ecore_animator_frametime_get() / 3" ? yes, actually this patch is for testing and work in progress. So I need to make patch more completely. >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.h:62 >> + class Transition { > > What is the reason for it to be nested type? It is because it is used only here yet. >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.h:64 >> + Transition(PageViewportControllerClientEfl* parent) > > if it is a parent there should be a child. Think it's just a viewportController. Yes, right. >> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:53 >> + static void didFindZoomableArea(WKViewRef, WKPoint, WKRect, const void* clientInfo); > > const WKPoint& > const WKRect& OK.

(In reply to comment #7) > (From update of attachment 192164 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192164&action=review > > I will separate this patch into several patches, and apply comments to the each patch. > > >> Source/WebKit2/UIProcess/PageViewportController.cpp:261 > >> + applyScaleAfterRenderingContents(scale); > > > > Why? as far as I remember (I might be wrong) that would lead to scaling dead loop. Anyway must be tested carefully. > > Without above patch, I can not animate the scaling because m_pendingScaleChange is set to true for first iteration of animation, so I need that code. > I didn't see the dead loop yet, but I will test scaling more and corefully. > > >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:43 > >> + , m_transition(new Transition(this)) > > > > Is it leaking? OwnPtr might help > > thanks, I will modify. > > >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:93 > >> + m_animator = ecore_animator_add(animatorCallback, this); > > > > where is ecore_animator_del() ? > > I have to add that. > > >>>> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:94 > >>>> + m_scaleDuration = 1 / ecore_animator_frametime_get() / 3; > >>> > >>> Does this bring us any advantage (as we are using coord. graphics) in contract to using a webcore timer? It might do, maybe you should discuss with dshuang or noamr > >> > >> The ecore_animator is useful because it is running depend on settings of Ecore. > >> That means, all ecore_animator of some applications can run with same duration and tick. > >> So, I think ecore_animator is better than webcore timer for EFL. > > > > maybe it is possible to make it more readable than "1 / ecore_animator_frametime_get() / 3" ? > > yes, actually this patch is for testing and work in progress. > So I need to make patch more completely. > > >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.h:62 > >> + class Transition { > > > > What is the reason for it to be nested type? > > It is because it is used only here yet. Could you please than just define it in .cpp file and keep only forward declaration in header?

> Could you please than just define it in .cpp file and keep only forward declaration in header? I mean not make it nested.

(In reply to comment #9) > > Could you please than just define it in .cpp file and keep only forward declaration in header? > I mean not make it nested. OK, I will make it separated class, and I will define it in the header file to use OwnPtr.

Created attachment 207634 [details] Patch Update for comments.

Created attachment 208405 [details] Patch Rebase.

Comment on attachment 208405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208405&action=review > Source/WebKit2/UIProcess/PageViewportController.cpp:-260 > - if (!m_pendingScaleChange) This change looks a bit odd and is not explained in the Changelog. Do we really need this? Cannot we make sure the m_pendingScaleChange flag is correctly updated instead? > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:48 > + virtual ~Transition(); It does not look like this destructor needs to be virtual, at least for now. > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:50 > + void setRects(const WebCore::FloatRect& baseRect, const WebCore::FloatRect& targetRect); WebCore:: is not needed > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:54 > + Transition(TransitionClient*); explicit > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:61 > + WebCore::FloatRect m_targetRect; WebCore:: is not needed. > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:62 > + WebCore::FloatRect m_baseRect; Ditto. > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:89 > + m_animator = ecore_animator_add(animatorCallback, this); This looks leak prone, what if m_animator was not null? (i.e. if someone calls start() several times). We at least need an assertion. > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:99 > +bool Transition::process() This method does not seem to bring much, I would just move the code to Transition::animatorCallback(). > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:101 > + FloatRect rect; I would move this one right before m_client->setTransitionRect(rect); > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:102 > + float multiplier = easeInOutQuad(m_scaleIndex++, 0, 1.0, m_scaleDuration); Use 1 instead of 1.0, See "Floating point literals" section in WebKit coding style. > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:112 > + return false; Here I would use ECORE_CALLBACK_CANCEL for clarity > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:115 > + return true; ECORE_CALLBACK_RENEW > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:193 > + FloatRect currentContentsRect = FloatRect(contentsPosition.x, contentsPosition.y, viewportSize.width, viewportSize.height); FloatRect currentContentsRect(contentsPosition.x, contentsPosition.y, viewportSize.width, viewportSize.height);

Comment on attachment 208405 [details] Patch Thanks, I will upload new patch with re-base and applying comments.

Comment on attachment 208405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208405&action=review >> Source/WebKit2/UIProcess/PageViewportController.cpp:-260 >> - if (!m_pendingScaleChange) > > This change looks a bit odd and is not explained in the Changelog. Do we really need this? Cannot we make sure the m_pendingScaleChange flag is correctly updated instead? I will make new patch without using didChangeContentsVisibility. >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:89 >> + m_animator = ecore_animator_add(animatorCallback, this); > > This looks leak prone, what if m_animator was not null? (i.e. if someone calls start() several times). We at least need an assertion. If m_animator is not null, it should be deleted firstly.

Created attachment 222411 [details] Patch Rebase and modify for Christophe Dumez's comments.

Comment on attachment 222411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222411&action=review Looks good to me on EFL side. > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.h:71 > + void applyScaleAfterRenderingContents(float scale); Need to confirm by Coodinated graphics folks, Noam ?

(In reply to comment #17) > (From update of attachment 222411 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222411&action=review > > Looks good to me on EFL side. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.h:71 > > + void applyScaleAfterRenderingContents(float scale); > > Need to confirm by Coodinated graphics folks, Noam ? Noam, any comment ?

Comment on attachment 222411 [details] Patch Eunmi, I wonder if this patch is still valid. I would clear review? from attachment 222411 [details] so that this bug does not appear in http://webkit.org/pending-review. If this patch is still valid for EFL port, please rebase this patch, then request review again.

Closing this bug because the EFL port has been removed from trunk. If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.