Bug 101453 – Fix transformation between host and css for moveTo, resizeTo etc.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Kenneth Rohde Christiansen]

windowRect is the physical window rect including chrome
pageRect is NOT the rect of the page (which some ports thinks) but the rect of the viewport. Current usage only uses the size, not the x,y coords.

[Kenneth Rohde Christiansen]

Created attachment 172766 [details]
Patch

[Simon Hausmann]

Comment on attachment 172766 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:99
> -        virtual void setWindowRect(const FloatRect&) = 0;
> -        virtual FloatRect windowRect() = 0;
> -        
> -        virtual FloatRect pageRect() = 0;
> -        
> +
> +        virtual void setPhysicalWindowRect(const FloatRect&) = 0;
> +        virtual FloatRect physicalWindowRect() = 0;
> +        virtual FloatRect physicalViewportRect() = 0;

IMHO windowRect() is fine as a name, I don't see the value that "physical" adds really. It's the window you see on the screen.

I agree about pageRect() though and viewportRect() should seem a lot more suitable. I'm not sure what physical adds :)

[Kenneth Rohde Christiansen]

Well it avoid the confusing between css units and physical units which keeps appearing. I vote for making it more obvious.

[Peter Beverloo]

Comment on attachment 172766 [details]
Patch

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

I'm in favor of clarifying that these methods return the physical size rather than the logical/UI size. A few nits regarding the changelogs..

> Source/WebKit/efl/ChangeLog:8
> +        Rename the methods.

ChromeClientEfl::physicalViewportRect is now returning the physical window rect instead of what ewk_view_page_rect_get() used to return. Unless they're identical, you may want to add some background here.

> Source/WebKit/qt/ChangeLog:8
> +        Rename the methods.

The !m_webPage case in physicalViewportRect() now returns the window rect rather than an empty FloatRect.

[Kenneth Rohde Christiansen]

Created attachment 172792 [details]
Patch

[Antonio Gomes]

Comment on attachment 172792 [details]
Patch

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

I agree with Simon: not sure about 'physical', but viewportRect is much saner than pageRect. Maybe webkit-dev has opinions?

One question:

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:126
> -FloatRect WebChromeClient::pageRect()
> +FloatRect WebChromeClient::physicalViewportRect()
>  {
> +#if USE(TILED_BACKING_STORE)
> +    return FloatRect(FloatPoint(), m_page->viewportSize());
> +#else
>      return FloatRect(FloatPoint(), m_page->size());
> +#endif
>  }

here it looks there is a real difference between m_page->size x viewportSize?

[Kenneth Rohde Christiansen]

> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:126
> > -FloatRect WebChromeClient::pageRect()
> > +FloatRect WebChromeClient::physicalViewportRect()
> >  {
> > +#if USE(TILED_BACKING_STORE)
> > +    return FloatRect(FloatPoint(), m_page->viewportSize());
> > +#else
> >      return FloatRect(FloatPoint(), m_page->size());
> > +#endif
> >  }
> 
> here it looks there is a real difference between m_page->size x viewportSize?

Tiled backing store resizes the FrameView to the size of the content, that is why. viewportSize is only available when tiled backing store is in use.

[Simon Fraser (smfr)]

Comment on attachment 172792 [details]
Patch

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

It's not clear what "physical" means.

> Source/WebCore/ChangeLog:8
> +        Rename the methods and use there of.

You need to explain your new terminology here. I don't really know what "physical" means. Should I get out a ruler and measure my screen?

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> (From update of attachment 172792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172792&action=review
> 
> It's not clear what "physical" means.
> 
> > Source/WebCore/ChangeLog:8
> > +        Rename the methods and use there of.
> 
> You need to explain your new terminology here. I don't really know what "physical" means. Should I get out a ruler and measure my screen?

It is "native" thus device pixels. Beveloo suggested that physical was better.

What do you suggest?

[Kenneth Rohde Christiansen]

I discussed this with smfr on irc. I will post the log for people to understand the background.

<kenneth_> smfr, you have a better suggestion that physical? it is in native device pixels. Physical was suggested to me, but maybe native? is better
<smfr> kenneth_: "device" is pretty descriptive
<kenneth_> ok then I will use that
<smfr> kenneth_: but will it be device on all platforms?
<smfr> kenneth_: i think not; it's not device on Mac or iOS retina screens, right?
<smfr> i think it's more like "UI" pixels
<kenneth_> yes, adjustment should be done elsewhere, or it will stay a mess
<kenneth_> people use this as well for actually resizing their native windows, so forcing an adjustment there seems confusing
<kenneth_> and webkit knows all about the deviceScaleFactor 
<smfr> ChromeWindowRect?
<smfr> kenneth_: since these are methods on ChromeClient I'm not sure a rename is actually necessary
<kenneth_> yeah im wondering as well
<kenneth_> Maybe I should just remove the prefix, people seems to think that is the best solution
<smfr> kenneth_: also, how does this interact with window.moveBy etc?
<smfr> are web authors expecting those to be "chorme" pixels?
<smfr> "chrome"
<kenneth_> that is actually something I wondered today
<kenneth_> because currently they are chrome pixels
<smfr> does the spec say?
<smfr> i'd epxect them to be like CSS pixels, but I'm not sure
<kenneth_> I couldn't find this in any spec, mozilla docs says it is not specced
<kenneth_> I would like them to be css pixels as well
<kenneth_> if people want to align things with web content that makes sense
<kenneth_> like the google chrome music experience that created many pop up windows
<smfr> kenneth_: would be good to figure that out. maybe post on webkit-dev
<smfr> kenneth_: also, another approach here would be to use types
<kenneth_> in what way?
<smfr> typedef FloatRect  ChromeRect;
<smfr> then the type tells you want units its in
<kenneth_> yeah I wanted to tackle the moveBy in another patch
<kenneth_> oh yeah, I should consider that
<kenneth_> you prefer ChromeRect to DeviceRect ?
<kenneth_> so you would do this typedef in ChromeClient.h ?
<smfr> kenneth_: i don't think it's device on all platforms
<smfr> i think it's the units that the UI is described in
<kenneth_> yes, make sense
<kenneth_> so how does this fit with deviceScaleFactor then? say we wanted to use these for some web exposed values?
<kenneth_> say if the ChromeClient exposed the screenRect (which is currently in PlatformScreen)
<smfr> i don't think you can know how it relates to deviceScaleFactor across different OSes
<kenneth_> so really we should have some mapping methods for that
<smfr> sure

We then discussed the naming of the ChromeRect. UIRect was dismissed as being too generic, together with ScreenRect.

Any other reviewer has better suggestion?

[Kenneth Rohde Christiansen]

Created attachment 173007 [details]
Patch

[Early Warning System Bot]

Comment on attachment 173007 [details]
Patch

Attachment 173007 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14775054

[Early Warning System Bot]

Comment on attachment 173007 [details]
Patch

Attachment 173007 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14761660

[Kenneth Rohde Christiansen]

Created attachment 173010 [details]
Patch

[Build Bot]

Comment on attachment 173010 [details]
Patch

Attachment 173010 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14777010

[Kenneth Rohde Christiansen]

Fixed mac locally, but im waiting for the EWS to finish before uploading.

[kov's GTK+ EWS bot]

Comment on attachment 173010 [details]
Patch

Attachment 173010 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14760854

[Simon Fraser (smfr)]

Comment on attachment 173010 [details]
Patch

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

I don't think we can make these changes until we know how to convert between host pixels and whatever moveBy() etc. use.

> Source/WebCore/page/DOMWindow.cpp:1449
> +    FloatRect windowRect = page->chrome()->windowRect();

Here you're implicitly converting between "host" coordinates and whatever coordinates moveBy uses (CSS pixels?), which I think is wrong.

> Source/WebCore/page/DOMWindow.cpp:1453
> +    adjustWindowRect(screenAvailableRect(page->mainFrame()->view()), windowRect, update);

Does screenAvailableRect() work in host pixels?

> Source/WebCore/page/DOMWindow.cpp:1469
> +    FloatRect windowRect = page->chrome()->windowRect();

Same here.

> Source/WebCore/page/DOMWindow.cpp:1470
> +    FloatRect screenRect = screenAvailableRect(page->mainFrame()->view());

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1495
> +    FloatRect windowRect = page->chrome()->windowRect();
> +    FloatSize destination = windowRect.size() + FloatSize(x, y);
> +    FloatRect update(windowRect.location(), destination);
> +    adjustWindowRect(screenAvailableRect(page->mainFrame()->view()), windowRect, update);
> +    page->chrome()->setWindowRect(windowRect);

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1514
> +    FloatRect windowRect = page->chrome()->windowRect();
> +    FloatSize destination = FloatSize(width, height);
> +    FloatRect update(windowRect.location(), destination);
> +    adjustWindowRect(screenAvailableRect(page->mainFrame()->view()), windowRect, update);
> +    page->chrome()->setWindowRect(windowRect);

Ditto.

[Kenneth Rohde Christiansen]

OK, I wanted to take that in another round. What do you think about reusing the screenToRootView methods in HostWindow and fixing them up?

[Kenneth Rohde Christiansen]

Created attachment 173062 [details]
Test using the moveTo, etc methods

[Simon Fraser (smfr)]

(In reply to comment #19)
> OK, I wanted to take that in another round. What do you think about reusing the screenToRootView methods in HostWindow and fixing them up?

If you do, you should at least add stub conversion methods in this patch.

I'm not sure about using screenToRootView(). I have no idea whether those window rects are in "screen" coordinates.

[Kenneth Rohde Christiansen]

(In reply to comment #21)
> (In reply to comment #19)
> > OK, I wanted to take that in another round. What do you think about reusing the screenToRootView methods in HostWindow and fixing them up?
> 
> If you do, you should at least add stub conversion methods in this patch.
> 
> I'm not sure about using screenToRootView(). I have no idea whether those window rects are in "screen" coordinates.

I was wondering since those methods seems barely used, for instance the following gives me nothing.

git grep  " screenToRootView(" Source/ | grep -v virtual

We could also rename them, say to hostToRootView or convertToPage like discussed yesterday. I am open to suggestions.

[Kenneth Rohde Christiansen]

Following FrameView::mapFromLayoutToCSSUnits(LayoutUnit);

we could call them

FloatRect HostWindow::mapFromHostToCSSRect(HostRect rect)
HostRect  HostWindow::mapFromCSSToHostRect(FloatRect rect)

Comments?

[Simon Fraser (smfr)]

(In reply to comment #23)
> Following FrameView::mapFromLayoutToCSSUnits(LayoutUnit);
> 
> we could call them
> 
> FloatRect HostWindow::mapFromHostToCSSRect(HostRect rect)
> HostRect  HostWindow::mapFromCSSToHostRect(FloatRect rect)

I can see how these would scale, but what's the origin for the mapping? Do they do coordinate flipping?

[Kenneth Rohde Christiansen]

(In reply to comment #24)
> (In reply to comment #23)
> > Following FrameView::mapFromLayoutToCSSUnits(LayoutUnit);
> > 
> > we could call them
> > 
> > FloatRect HostWindow::mapFromHostToCSSRect(HostRect rect)
> > HostRect  HostWindow::mapFromCSSToHostRect(FloatRect rect)
> 
> I can see how these would scale, but what's the origin for the mapping? Do they do coordinate flipping?

I think they should do that if that is needed by the platform. Do you agree?

On mac everything needs coordinate flipping? like are windows positioned after origin in lower left?

[Kenneth Rohde Christiansen]

Created attachment 173320 [details]
WIP

[Kenneth Rohde Christiansen]

Created attachment 173607 [details]
EWS run

[Kenneth Rohde Christiansen]

Created attachment 173627 [details]
With manual test

[Build Bot]

Comment on attachment 173607 [details]
EWS run

Attachment 173607 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14796871

[Peter Beverloo (cr-android ews)]

Comment on attachment 173607 [details]
EWS run

Attachment 173607 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14821092

[WebKit Review Bot]

Comment on attachment 173607 [details]
EWS run

Attachment 173607 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14816182

[Build Bot]

Comment on attachment 173627 [details]
With manual test

Attachment 173627 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14821403

[WebKit Review Bot]

Comment on attachment 173627 [details]
With manual test

Attachment 173627 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14821440

[Peter Beverloo (cr-android ews)]

Comment on attachment 173627 [details]
With manual test

Attachment 173627 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14813654

[Kenneth Rohde Christiansen]

I am dropping this for now