Use IntRect instead of FloatRect for window positions and sizes

Use IntRect instead of FloatRect for window positions and sizes

Effectively, a window's position and size was narrowed to an integer
rectangle (WebRect) anyway in Blink's public API (WebWidgetClient) so
the floating point processing in the middle made no sense.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189165

[Jens Widell, 2015-01-28 12:29:54]

jl@opera.com changed reviewers:
+ mkwst@chromium.org

[Jens Widell, 2015-01-28 12:29:55]

PTAL

Added some comments explaining parts of the patch.

https://codereview.chromium.org/879913003/diff/1/LayoutTests/fast/dom/Window/...
File LayoutTests/fast/dom/Window/window-resize-and-move-arguments.html (right):

https://codereview.chromium.org/879913003/diff/1/LayoutTests/fast/dom/Window/...
LayoutTests/fast/dom/Window/window-resize-and-move-arguments.html:20:
window.moveTo(screen.availLeft + 100, screen.availTop + 100);
This whole test was somewhat confusing.

In my environment (I have multiple monitors) the screen's top left corner was at
x=1200, y=240. So moveTo(0, 0) was really moveTo(1200, 240). And below, the test
asserts that moveTo() moves the window to screen.avail{Left,Top}. But it didn't
(and doesn't): moveTo() without arguments doesn't move the window at all.

Resetting the window here away move a "zero" position means the test now
actually sees a difference between "moving to 0, 0" and "not moving", which it
previously didn't.

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/Window.idl
File Source/core/frame/Window.idl (right):

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/Window.idl...
Source/core/frame/Window.idl:104: void moveBy(optional long x = 0, optional long
y = 0);
All of these were previously no-ops when called without arguments. For
moveBy/resizeBy, having zero default values results in them being no-ops, and is
a more convenient way to express it in the IDL.

For moveTo/resizeTo, we need to tell a zero argument and a missing argument
apart to not affect observable behavior. This was previously achieved (sort of)
by having [Default=Undefined] which resulted in NaN argument values. (We do
change behavior for explicit NaN arguments now; they were previously treated as
missing, now they become zero.)

It's possible the observable behavior is also wrong, but I wanted to not change
it in this patch, since I already changed quite a lot of code.

https://codereview.chromium.org/879913003/diff/1/Source/core/page/WindowFeatu...
File Source/core/page/WindowFeatures.cpp (left):

https://codereview.chromium.org/879913003/diff/1/Source/core/page/WindowFeatu...
Source/core/page/WindowFeatures.cpp:239: return static_cast<int>(parsedNumber);
Due to this cast, changing the return type from 'float' to 'int' doesn't really
change anything. (Unless the 'min', 'max' or 'defaultValue' arguments were
non-integer, but they never are.)

Also, this only affects showModalDialog(), AFAICT.

https://codereview.chromium.org/879913003/diff/1/Source/platform/PlatformScre...
File Source/platform/PlatformScreen.cpp (right):

https://codereview.chromium.org/879913003/diff/1/Source/platform/PlatformScre...
Source/platform/PlatformScreen.cpp:80: return
IntRect(hostWindow->screenInfo().rect);
WebScreenInfo::rect is WebRect, so changing the return value to IntRect doesn't
restrict the return value.

And the WebRect was of course tellingly explicitly cast to an IntRect along the
way as well.

https://codereview.chromium.org/879913003/diff/1/Source/platform/PlatformScre...
Source/platform/PlatformScreen.cpp:88: return
IntRect(hostWindow->screenInfo().availableRect);
WebScreenInfo::availableRect is WebRect too.

https://codereview.chromium.org/879913003/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/879913003/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:173: return windowRect();
This seems to be the only implementation of pageRect() that returns a non-empty
rectangle, and it returns windowRect(), which was effectively an integer
rectangle already (from a WebRect) so pageRect() was effectively an integer
rectangle already.

[Mike West, 2015-01-28 13:33:18]

Thanks for the explanation, that was very helpful.

I don't entirely understand the boolean weirdness in DOMWindow.h. Is that just
there to support the "default argument vs explicit 0" question?

Can we just break that, please? :)

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.h:154: virtual void moveTo(int x, int y, bool hasX =
true, bool hasY = true) const = 0;
This is really strange. :/

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/Window.idl
File Source/core/frame/Window.idl (right):

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/Window.idl...
Source/core/frame/Window.idl:104: void moveBy(optional long x = 0, optional long
y = 0);
On 2015/01/28 12:29:55, Jens Widell wrote:
> All of these were previously no-ops when called without arguments. For
> moveBy/resizeBy, having zero default values results in them being no-ops, and
is
> a more convenient way to express it in the IDL.
> 
> For moveTo/resizeTo, we need to tell a zero argument and a missing argument
> apart to not affect observable behavior. This was previously achieved (sort
of)
> by having [Default=Undefined] which resulted in NaN argument values. (We do
> change behavior for explicit NaN arguments now; they were previously treated
as
> missing, now they become zero.)
> 
> It's possible the observable behavior is also wrong, but I wanted to not
change
> it in this patch, since I already changed quite a lot of code.

Calling these methods without arguments doesn't make any sense, does it? Could
you check Firefox's/IE's behavior to see if we can easily justify breaking the
existing observable behavior? I'd prefer to do that, honestly, as it would also
remove the boolean weirdness from the internal implementation.

https://codereview.chromium.org/879913003/diff/1/Source/platform/PlatformScre...
File Source/platform/PlatformScreen.cpp (right):

https://codereview.chromium.org/879913003/diff/1/Source/platform/PlatformScre...
Source/platform/PlatformScreen.cpp:80: return
IntRect(hostWindow->screenInfo().rect);
On 2015/01/28 12:29:55, Jens Widell wrote:
> WebScreenInfo::rect is WebRect, so changing the return value to IntRect
doesn't
> restrict the return value.
> 
> And the WebRect was of course tellingly explicitly cast to an IntRect along
the
> way as well.

Acknowledged.

https://codereview.chromium.org/879913003/diff/1/Source/platform/PlatformScre...
Source/platform/PlatformScreen.cpp:88: return
IntRect(hostWindow->screenInfo().availableRect);
On 2015/01/28 12:29:55, Jens Widell wrote:
> WebScreenInfo::availableRect is WebRect too.

Acknowledged.

[Jens Widell, 2015-01-28 14:09:23]

On 2015/01/28 13:33:18, Mike West wrote:
> I don't entirely understand the boolean weirdness in DOMWindow.h. Is that just
> there to support the "default argument vs explicit 0" question?
> 
> Can we just break that, please? :)

Based on your comments and some testing, I propose to just make the arguments
"optional long x = 0".

This changes the behavior of moveTo() from "no-op" to "move to upper-left-corner
(maybe)" and resizeTo() from "no-op" to... "resize to default size" (according
to LocalDOMWindow::adjustWindowRect()).

About default size: according to window-resize-and-move-arguments.html
resizeTo(0, 0) actually simply resizes to width=0, height=0. So if we change the
arguments to default to zero, window.resizeTo() might go from "no-op" to "make
the window go away".

Except I can't make resizeTo() (with any set of arguments) do anything at all in
Chrome. And maybe it's the UI that's expected to adjust zero sizes to some
default size?

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/DOMWindow....
Source/core/frame/DOMWindow.h:154: virtual void moveTo(int x, int y, bool hasX =
true, bool hasY = true) const = 0;
On 2015/01/28 13:33:17, Mike West wrote:
> This is really strange. :/

This is simply to differentiate between "missing" and 0. (And partly done this
way so that there's only one virtual overload for sub-classes to worry about.)

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/Window.idl
File Source/core/frame/Window.idl (right):

https://codereview.chromium.org/879913003/diff/1/Source/core/frame/Window.idl...
Source/core/frame/Window.idl:104: void moveBy(optional long x = 0, optional long
y = 0);
On 2015/01/28 13:33:17, Mike West wrote:
> On 2015/01/28 12:29:55, Jens Widell wrote:
> > All of these were previously no-ops when called without arguments. For
> > moveBy/resizeBy, having zero default values results in them being no-ops,
and
> is
> > a more convenient way to express it in the IDL.
> > 
> > For moveTo/resizeTo, we need to tell a zero argument and a missing argument
> > apart to not affect observable behavior. This was previously achieved (sort
> of)
> > by having [Default=Undefined] which resulted in NaN argument values. (We do
> > change behavior for explicit NaN arguments now; they were previously treated
> as
> > missing, now they become zero.)
> > 
> > It's possible the observable behavior is also wrong, but I wanted to not
> change
> > it in this patch, since I already changed quite a lot of code.
> 
> Calling these methods without arguments doesn't make any sense, does it? Could
> you check Firefox's/IE's behavior to see if we can easily justify breaking the
> existing observable behavior? I'd prefer to do that, honestly, as it would
also
> remove the boolean weirdness from the internal implementation.

Firefox 35: arguments are not optional.
IE 11: arguments default to zero (and a minimum window size enforced).

So I guess it should be fine to go for any of those. Non-optional breaks some
current LayoutTests (aside from the test I've already modified) so would sort of
suggest following IE would be slightly safer.

[Mike West, 2015-01-28 14:48:35]

I'd be fine with either of the solutions you propose, I think. Though I'd
personally prefer Firefox's behavior, I don't care enough to argue about it. :)

Why don't you send an "Intent to Break The Web" to blink-dev@ to see if anyone
has objections? I think the risk is pretty low, given Firefox's behavior.

[Jens Widell, 2015-01-28 14:54:42]

On 2015/01/28 14:48:35, Mike West wrote:
> I'd be fine with either of the solutions you propose, I think. Though I'd
> personally prefer Firefox's behavior, I don't care enough to argue about it.
:)
> 
> Why don't you send an "Intent to Break The Web" to blink-dev@ to see if anyone
> has objections? I think the risk is pretty low, given Firefox's behavior.

Would it be okay to land this CL as-is and then break the web in a separate CL
after a blink-dev@ notice?

[Mike West, 2015-01-28 14:55:56]

I guess so, but do break it one way or another, please. :) LGTM.

[Jens Widell, 2015-01-28 14:57:00]

On 2015/01/28 14:55:56, Mike West wrote:
> I guess so, but do break it one way or another, please. :) LGTM.

I will, promise. :-)

[Jens Widell, 2015-01-28 14:57:28]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2015-01-28 14:57:52]

The CQ bit was unchecked by jl@opera.com

[Jens Widell, 2015-01-29 06:16:17]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-01-29 06:16:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/879913003/20001

[commit-bot: I haz the power, 2015-01-29 06:19:26]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189165