Bug 110197

Summary: [EFL][WK2] Convert from device view size to UI view size only in EwkView.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: WebKit EFLAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, dglazkov, gyuyoung.kim, jturcotte, kenneth, lucas.de.marchi, mikhail.pozdnyakov, rakuco, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110298    
Bug Blocks: 110066, 110847    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
kenneth: review+, kenneth: commit-queue-
Patch for landing
none
patch for landing none

Dongseong Hwang
Reported 2013-02-19 03:02:59 PST
deviceScaleFactor is the property of device window, so only EwkView should deal with converting device view size to dip(device independent) view size and device scroll position to dip scroll position. It increases readability because we can regard view size and scroll position in other classes (e.g. WebView, PageViewportController, WebPage) except for EwkView as metrics in dip unit. Currently there is a bug. WebView::updateViewportSize() calls DrawingAreaProxy::setVisibleContentsRect() with device view size and device scroll position while PageViewportController calls setVisibleContentsRect() with the dip metrics.
Attachments
Patch (8.73 KB, patch)
2013-02-19 03:11 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (8.73 KB, patch)
2013-02-19 20:01 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (6.39 KB, patch)
2013-02-25 22:09 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (6.68 KB, patch)
2013-02-25 22:47 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (6.68 KB, patch)
2013-03-03 17:03 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (6.69 KB, patch)
2013-03-04 14:48 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (6.69 KB, patch)
2013-03-05 18:01 PST, Dongseong Hwang 		kenneth: review+
kenneth: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (18.94 KB, patch)
2013-03-06 01:01 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
patch for landing (7.41 KB, patch)
2013-03-06 01:03 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Dongseong Hwang
Comment 1 2013-02-19 03:11:13 PST
Created attachment 189040 [details] Patch
Dongseong Hwang
Comment 2 2013-02-19 03:12:19 PST
I introduced dipSize and dipScrollPosition in EwkView as following WebViewImpl::dipSize() in chromium. WDYT? :)
Kenneth Rohde Christiansen
Comment 3 2013-02-19 04:48:26 PST
Comment on attachment 189040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189040&action=review > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:486 > +IntSize EwkView::dipSize() const > +{ > + LayoutSize dipSize = m_size; > + dipSize.scale(1 / deviceScaleFactor()); > + return roundedIntSize(dipSize); > +} > + > +IntSize EwkView::size() const > +{ > + return roundedIntSize(m_size); > +} im not sure I am too fount of these names, but I guess they could work. What about size() and IntSize toDIP(const IntSize&) IntPoint toDIP(const IntPoint&)
Build Bot
Comment 4 2013-02-19 07:57:44 PST
Comment on attachment 189040 [details] Patch Attachment 189040 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16616717 New failing tests: inspector/styles/stylesheet-tracking.html
Dongseong Hwang
Comment 5 2013-02-19 18:34:03 PST
(In reply to comment #3) > (From update of attachment 189040 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189040&action=review > im not sure I am too fount of these names, but I guess they could work. > > What about size() and IntSize toDIP(const IntSize&) IntPoint toDIP(const IntPoint&) That's reasonable idea, but I don't want size() is public method. I mean that I want clients (e.g. WebView) to query only dip scale and position of EwkView. WDYT?
Dongseong Hwang
Comment 6 2013-02-19 20:01:42 PST
Created attachment 189225 [details] Patch
Kenneth Rohde Christiansen
Comment 7 2013-02-22 07:05:04 PST
(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 189040 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=189040&action=review > > im not sure I am too fount of these names, but I guess they could work. > > > > What about size() and IntSize toDIP(const IntSize&) IntPoint toDIP(const IntPoint&) > > That's reasonable idea, but I don't want size() is public method. I mean that I want clients (e.g. WebView) to query only dip scale and position of EwkView. WDYT? Well shouldn't size then not just return toDIP(m_size) or so, or dont many it query, but have us set the size instead. EwkView is very EFL specific, so it makes sense to have like a nativeSize() or similar, which works in the same domain as EFL. Then you can call ->setSize(toDIP(nativeSize())) if you need to.
Dongseong Hwang
Comment 8 2013-02-25 22:09:17 PST
Created attachment 190204 [details] Patch
Dongseong Hwang
Comment 9 2013-02-25 22:12:34 PST
(In reply to comment #7) > Well shouldn't size then not just return toDIP(m_size) or so, or dont many it query, but have us set the size instead. > > EwkView is very EFL specific, so it makes sense to have like a nativeSize() or similar, which works in the same domain as EFL. > > Then you can call ->setSize(toDIP(nativeSize())) if you need to. WebView needs to query both native size and dip size from EwkView because recent change of WebView and EwkView. So EwkView now has two api: size() and nativeSize(). How about it?
Dongseong Hwang
Comment 10 2013-02-25 22:47:00 PST
Created attachment 190210 [details] Patch
Kenneth Rohde Christiansen
Comment 11 2013-02-26 06:43:03 PST
Comment on attachment 190210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190210&action=review LGTM > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:486 > +IntSize EwkView::nativeSize() const > +{ > + return roundedIntSize(m_nativeSize); > +} m_nativeSize is already an int! at least it is set to... so rounding should do nothing
Dongseong Hwang
Comment 12 2013-03-03 17:03:25 PST
Created attachment 191154 [details] Patch
Dongseong Hwang
Comment 13 2013-03-03 17:09:47 PST
(In reply to comment #11) > (From update of attachment 190210 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190210&action=review > > LGTM Thank you for review :) > m_nativeSize is already an int! at least it is set to... so rounding should do nothing Good point. now m_nativeSize is IntSize.
Kenneth Rohde Christiansen
Comment 14 2013-03-04 01:04:03 PST
Comment on attachment 191154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191154&action=review LGTM > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:481 > + // WebPage expects a size in UI units, and not raw device units. > + FloatSize dipSize = m_nativeSize; Maybe cssSize is more understandable?
Dongseong Hwang
Comment 15 2013-03-04 02:48:26 PST
Comment on attachment 191154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191154&action=review >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:481 >> + FloatSize dipSize = m_nativeSize; > > Maybe cssSize is more understandable? Thank you for review! I think cssSize is m_nativeSize / (pageScaleFactor * deviceScaleFactor), and dipSize is m_nativeSize / pageScaleFactor. so I think that dipSize is more relevant. How do you think? :)
Dongseong Hwang
Comment 16 2013-03-04 14:48:32 PST
Created attachment 191317 [details] Patch
Dongseong Hwang
Comment 17 2013-03-04 14:50:40 PST
(In reply to comment #15) > (From update of attachment 191154 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191154&action=review > > >> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:481 > >> + FloatSize dipSize = m_nativeSize; > > > > Maybe cssSize is more understandable? > > Thank you for review! > I think cssSize is m_nativeSize / (pageScaleFactor * deviceScaleFactor), and dipSize is m_nativeSize / pageScaleFactor. so I think that dipSize is more relevant. > How do you think? :) I changed from dipSize to uiSize because UI is more general acronym. please refer to http://trac.webkit.org/wiki/ScalesAndZooms
WebKit Review Bot
Comment 18 2013-03-04 16:27:25 PST
Comment on attachment 191317 [details] Patch Attachment 191317 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16907081 New failing tests: inspector/debugger/watch-expressions-panel-switch.html inspector/debugger/set-breakpoint.html
Dongseong Hwang
Comment 19 2013-03-04 21:41:23 PST
(In reply to comment #18) > (From update of attachment 191317 [details]) > Attachment 191317 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://webkit-commit-queue.appspot.com/results/16907081 > > New failing tests: > inspector/debugger/watch-expressions-panel-switch.html > inspector/debugger/set-breakpoint.html It is flaky because this patch is not related to chromium.
Benjamin Poulain
Comment 20 2013-03-05 16:28:52 PST
Comment on attachment 191317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191317&action=review I don't mind this. Okay for WebKit2. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:484 > +IntSize EwkView::size() const > +{ > + // WebPage expects a size in UI units, and not raw device units. > + FloatSize uiSize = m_nativeSize; > + uiSize.scale(1 / deviceScaleFactor()); > + return roundedIntSize(uiSize); > +} I would compute this when changing the value. Then in EwkView::size() you just return drawingArea->size(). Or make the function private. > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:489 > +IntSize EwkView::nativeSize() const > +{ > + return m_nativeSize; > +} This name is confusing. A size is a dimension in a certain coordinate system. A "native" size does not mean anything.
Dongseong Hwang
Comment 21 2013-03-05 18:01:04 PST
Created attachment 191623 [details] Patch
Dongseong Hwang
Comment 22 2013-03-05 18:13:32 PST
(In reply to comment #20) > (From update of attachment 191317 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191317&action=review > > I don't mind this. Okay for WebKit2. Thank you for review. > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:484 > > +IntSize EwkView::size() const > > +{ > > + // WebPage expects a size in UI units, and not raw device units. > > + FloatSize uiSize = m_nativeSize; > > + uiSize.scale(1 / deviceScaleFactor()); > > + return roundedIntSize(uiSize); > > +} > > I would compute this when changing the value. Then in EwkView::size() you just return drawingArea->size(). That's good idea but we have problem if EwkView::size() returns drawingArea->size(). The initialize list of DrawingAreaProxy assigns m_size = m_size. DrawingAreaProxy::DrawingAreaProxy(DrawingAreaType type, WebPageProxy* webPageProxy) : m_type(type) , m_webPageProxy(webPageProxy) , m_size(webPageProxy->viewSize()) > Or make the function private. WebView needs to know both deviceSize and size. > > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:489 > > +IntSize EwkView::nativeSize() const > > +{ > > + return m_nativeSize; > > +} > > This name is confusing. > A size is a dimension in a certain coordinate system. A "native" size does not mean anything. I agree. I changed from native to device to match https://trac.webkit.org/wiki/ScalesAndZooms
Kenneth Rohde Christiansen
Comment 23 2013-03-06 00:36:53 PST
Comment on attachment 191623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191623&action=review r=me with the change (signed off by Benjamin) > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:466 > -void EwkView::setSize(const IntSize& size) > +void EwkView::setSize(const IntSize& deviceSize) This should be renamed to setDeviceSize then as the getter is called deviceSize()
Dongseong Hwang
Comment 24 2013-03-06 01:01:35 PST
Created attachment 191677 [details] Patch for landing
Dongseong Hwang
Comment 25 2013-03-06 01:03:05 PST
Created attachment 191678 [details] patch for landing
Kenneth Rohde Christiansen
Comment 26 2013-03-06 01:05:07 PST
Comment on attachment 191677 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=191677&action=review > Source/WebKit2/ChangeLog:34 > + > +2013-03-03 Huang Dongsung <luxtella@company100.net> > + > + [EFL][QT][WK2] Turn on ApplyPageScaleFactorInCompositor always. > + https://bugs.webkit.org/show_bug.cgi?id=111056 > + what is going on here??? multiple patches? > LayoutTests/ChangeLog:5 > + > + [EFL][QT][WK2] Turn on ApplyPageScaleFactorInCompositor always. > + https://bugs.webkit.org/show_bug.cgi?id=111056 > + what?
WebKit Review Bot
Comment 27 2013-03-06 01:37:24 PST
Comment on attachment 191678 [details] patch for landing Clearing flags on attachment: 191678 Committed r144899: <http://trac.webkit.org/changeset/144899>
WebKit Review Bot
Comment 28 2013-03-06 01:37:31 PST
All reviewed patches have been landed. Closing bug.
Dongseong Hwang
Comment 29 2013-03-06 07:25:18 PST
(In reply to comment #26) > (From update of attachment 191677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191677&action=review > > what is going on here??? multiple patches? > > what? the next to last patch is mistake. I think "./webkit-patch land-safely" has a bug. Don't worry :) r144899 is landed using the last patch.
Note You need to log in before you can comment on or make changes to this bug.