Summary: | [EFL][WK2] Convert from device view size to UI view size only in EwkView. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||||||||
Component: | WebKit EFL | Assignee: | 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
Dongseong Hwang
2013-02-19 03:02:59 PST
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. |