Bug 118585

Summary: [WK2] Share Qt port's codes to find zoomable area with CoordinatedGraphics.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit2Assignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, cdumez, cmarcelo, commit-queue, gmak, gyuyoung.kim, lucas.de.marchi, luiz, menard, noam, rakuco, tonikitoo, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 118588    
Bug Blocks: 107631    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Eunmi Lee 2013-07-11 23:55:17 PDT
The codes to find zoomable area will be used in the coordinated graphics, so modify codes for Qt port to share with coordinated graphics.
Comment 1 Eunmi Lee 2013-07-12 00:11:08 PDT
Created attachment 206509 [details]
Patch
Comment 2 Noam Rosenthal 2013-07-12 01:07:43 PDT
Comment on attachment 206509 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2952
> +#if PLATFORM(QT) || USE(COORDINATED_GRAPHICS)

No need for #if PLATFORM(QT) here
Qt already uses coordinated graphics.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2953
> +void WebPageProxy::findZoomableAreaForPoint(const IntPoint& point, const IntSize& area)

Maybe we can put this in a new file (WebPageProxyCoordinatedGraphics)?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3221
> +    IntRect zoomableArea = node->pixelSnappedBoundingBox();

This seems like a separate fix
Comment 3 Eunmi Lee 2013-07-12 01:16:42 PDT
Comment on attachment 206509 [details]
Patch

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

Thank you for comments :)

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:2952
>> +#if PLATFORM(QT) || USE(COORDINATED_GRAPHICS)
> 
> No need for #if PLATFORM(QT) here
> Qt already uses coordinated graphics.

That's good, I will modify all "PLATFORM(QT) || USE(COORDINATED_GRAPHICS)".

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:2953
>> +void WebPageProxy::findZoomableAreaForPoint(const IntPoint& point, const IntSize& area)
> 
> Maybe we can put this in a new file (WebPageProxyCoordinatedGraphics)?

OK. I will add WebPageProxyCoordinatedGraphics.cpp.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3221
>> +    IntRect zoomableArea = node->pixelSnappedBoundingBox();
> 
> This seems like a separate fix

Right. I will make separate patch for that.
Comment 4 Eunmi Lee 2013-07-12 05:57:03 PDT
Created attachment 206529 [details]
Patch
Comment 5 Noam Rosenthal 2013-07-17 19:21:07 PDT
Comment on attachment 206529 [details]
Patch

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

I'm ok with the change in general, but the Changelog needs to be rewritten and it needs owner approval since it touches WebPage.*.

> Source/WebKit2/ChangeLog:13
> +        The codes to find zoomable area will be used in the coordinated graphics,
> +        so modify codes to be able to be used not only in the Qt port but also
> +        coordinated graphics.
> +
> +        Additionally, Add the WKViewFindZoomableAreaForRect API and didFindZoomableArea
> +        callback for coordinated graphics and they will be used in the EFL port.

Please use correct English.
Comment 6 Eunmi Lee 2013-07-18 01:47:11 PDT
Comment on attachment 206529 [details]
Patch

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

>> Source/WebKit2/ChangeLog:13
>> +        callback for coordinated graphics and they will be used in the EFL port.
> 
> Please use correct English.

OK, I will re-write the changelog.
Comment 7 Eunmi Lee 2013-07-18 02:29:36 PDT
Created attachment 206972 [details]
Patch

Update changelog.
Comment 8 Noam Rosenthal 2013-07-18 02:38:18 PDT
Comment on attachment 206972 [details]
Patch

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

I'm ok with this, but it touches PageClient.h so please ask for WK2 owner review.

> Source/WebKit2/ChangeLog:11
> +        to use that fuction, so extend codes for Qt port to be usable in the
> +        CoordinatedGraphics and add API and callback to use that.

extend codes for Qt port -> extract the code from the Qt port
Comment 9 Eunmi Lee 2013-07-18 04:52:57 PDT
OK, thanks for review!

(In reply to comment #8)
> (From update of attachment 206972 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206972&action=review
> 
> I'm ok with this, but it touches PageClient.h so please ask for WK2 owner review.
> 
> > Source/WebKit2/ChangeLog:11
> > +        to use that fuction, so extend codes for Qt port to be usable in the
> > +        CoordinatedGraphics and add API and callback to use that.
> 
> extend codes for Qt port -> extract the code from the Qt port
Comment 10 Eunmi Lee 2013-07-18 05:02:23 PDT
Created attachment 206982 [details]
Patch

Update changelog.
Comment 11 Noam Rosenthal 2013-07-18 05:08:36 PDT
Comment on attachment 206982 [details]
Patch

LGTM, requires owner approval.
Comment 12 WebKit Commit Bot 2013-07-18 19:10:54 PDT
Comment on attachment 206982 [details]
Patch

Clearing flags on attachment: 206982

Committed r152878: <http://trac.webkit.org/changeset/152878>
Comment 13 WebKit Commit Bot 2013-07-18 19:10:59 PDT
All reviewed patches have been landed.  Closing bug.