Bug 107631 – [EFL][WK2] Scale to focused area when double tap gesture is recognized.

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Eunmi Lee]

Implement behavior for double tap.

[Eunmi Lee]

Created attachment 184166 [details]
Patch

Initial patch

[Eunmi Lee]

Created attachment 192164 [details]
Patch

Rebased.

[Kenneth Rohde Christiansen]

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

[Eunmi Lee]

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.

[Mikhail Pozdnyakov]

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&

[Mikhail Pozdnyakov]

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.

[Eunmi Lee]

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.

[Mikhail Pozdnyakov]

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

[Mikhail Pozdnyakov]

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

[Eunmi Lee]

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

[Eunmi Lee]

Created attachment 207634 [details]
Patch

Update for comments.

[Eunmi Lee]

Created attachment 208405 [details]
Patch

Rebase.

[Chris Dumez]

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

[Eunmi Lee]

Comment on attachment 208405 [details]
Patch

Thanks, I will upload new patch with re-base and applying comments.

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 222411 [details]
Patch

Rebase and modify for Christophe Dumez's comments.

[Gyuyoung Kim]

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 ?

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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.

[Michael Catanzaro]

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.