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.
Created attachment 189040 [details] Patch
I introduced dipSize and dipScrollPosition in EwkView as following WebViewImpl::dipSize() in chromium. WDYT? :)
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&)
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
(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?
Created attachment 189225 [details] Patch
(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.
Created attachment 190204 [details] Patch
(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?
Created attachment 190210 [details] Patch
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
Created attachment 191154 [details] Patch
(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.
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?
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? :)
Created attachment 191317 [details] Patch
(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
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
(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.
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.
Created attachment 191623 [details] Patch
(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
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()
Created attachment 191677 [details] Patch for landing
Created attachment 191678 [details] patch for landing
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?
Comment on attachment 191678 [details] patch for landing Clearing flags on attachment: 191678 Committed r144899: <http://trac.webkit.org/changeset/144899>
All reviewed patches have been landed. Closing bug.
(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.