Clarify the size of a document view (a.k.a layout size) that causes FrameView to emit the resize ev…

Clarify the size of a document view (a.k.a layout size) that causes FrameView to emit the resize event.

The spec DOM Level 2 Events states that the resize event occurs when a document
view is resized. Refer to http://www.w3.org/TR/DOM-Level-2-Events/events.html

After WebKit r141053, ScrollView::unscaledVisibleContentSize stands for a layout size.
When turning on fixed layout, fixed layout size stands for a layout size. There
is already ScrollView::layoutSize() method matching above explanation.

However, this issue introduced viewportSize() to include scrollbars. We don't
want that showing/hiding scrollbars is considered a layout size change. Refer to
webkit.org/b/80242

BUG=235833
TEST=fast/events/resize-events.html, fast/events/resize-events-count.html, fast/events/resize-events-fixed-layout.html

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

[dshwang, 2013-05-05 10:28:24]

On 2013/05/05 10:12:52, dshwang wrote:

There are three tests to support this issue.
1. resize-events.html
This test is changed to check that page scaling and fixed layout size changing
must not emit a resize event.

2. resize-events-fixed-layout.html
This is a copy version of resize-events.html for fixed layout. In this test,
fixed layout size changing must emit a resize event.

3. resize-events-count.html
There is no test that window.resize can emit a resize event. So this basic test
is added.

[aelias_OOO_until_Jul13, 2013-05-06 19:41:02]

Have you tested that all other browsers don't send resize events in this case?

[kenneth.r.christiansen, 2013-05-06 19:50:51]

https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/re...
File LayoutTests/fast/events/resize-events-count-expected.txt (right):

https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resize-events-count-expected.txt:1: Test how many resize
events are emitted during resizing window.
'during window resize.' sounds better IMHO

https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/re...
File LayoutTests/fast/events/resize-events-count.html (right):

https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resize-events-count.html:30: setTimeout(finish, 20);
Any way to avoid setTimeout. It would be sad if this ends up being a flaky test

https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/re...
File LayoutTests/fast/events/resize-events-fixed-layout-expected.txt (right):

https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resize-events-fixed-layout-expected.txt:1: Following
actions must not emit resize event. - page load - dynamic content generation -
page scaling When fixed layout is enabled, changing fixed layout size must emit
resize event. Do not resize the page. It invalidates the test.
That is a weird way of writing it, why not:

The following action much not emit resize events: page load, dynamic...

https://codereview.chromium.org/14766010/diff/3001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14766010/diff/3001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1024: m_lastViewportSize =
layoutSizeIncludingScrollbars();
The layout size doesn't include scrollbars. That sounds weird.

It is actually the visible rect including scrollbars right?

OK, it doesn't seem like you introduced this method, so ignore my comment

https://codereview.chromium.org/14766010/diff/3001/Source/core/platform/Scrol...
File Source/core/platform/ScrollView.h (right):

https://codereview.chromium.org/14766010/diff/3001/Source/core/platform/Scrol...
Source/core/platform/ScrollView.h:146: // However, we don't want scrollbar to
emit a resize event because other browsers don't have this behavior, and actual
site is badly affected. Refer to webkit.org/b/80242
I don't think this explains it very well. You want to say that showing/hiding
scrollbars shouldn't be considered a layout size change?

[dshwang, 2013-05-10 11:43:38]

On 2013/05/06 19:41:02, aelias wrote:
> Have you tested that all other browsers don't send resize events in this case?

I checked FF and Opera don't emit resize events using
ManualTests/resize-events.html
I did not check IE yet (because dom api is different :/)

On 2013/05/06 19:50:51, kenneth.r.christiansen wrote:
> 'during window resize.' sounds better IMHO
Thx!

>
https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/re...
> LayoutTests/fast/events/resize-events-count.html:30: setTimeout(finish, 20);
> Any way to avoid setTimeout. It would be sad if this ends up being a flaky
test
Yes, I agree. But writing test for events must need setTimeout. I do my best to
reduce the number of calling setTimeout.

In addition, I filed the bug related to this.
https://code.google.com/p/chromium/issues/detail?id=239759

This rev is blocked by https://codereview.chromium.org/15023009/ because of
setTimeout issue.

> The following action much not emit resize events: page load, dynamic...

>
https://codereview.chromium.org/14766010/diff/3001/Source/core/platform/Scrol...
> Source/core/platform/ScrollView.h:146: // However, we don't want scrollbar to
> emit a resize event because other browsers don't have this behavior, and
actual
> site is badly affected. Refer to webkit.org/b/80242
> I don't think this explains it very well. You want to say that showing/hiding
> scrollbars shouldn't be considered a layout size change?

Thx!

[aelias_OOO_until_Jul13, 2013-05-29 18:47:28]

It looks like there is no layout test explicitly turning scrollbars on and off
(unless I'm missing something), could you add one?

https://codereview.chromium.org/14766010/diff/18001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14766010/diff/18001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:137: static inline IntSize viewportSize(const
FrameView* view)
I suggest not having this static method and just calling view->layoutSize(). 
Please move the comment into your layout test.

https://codereview.chromium.org/14766010/diff/18001/Source/core/platform/Scro...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/14766010/diff/18001/Source/core/platform/Scro...
Source/core/platform/ScrollView.cpp:218: ASSERT(!m_fixedLayoutSize.isEmpty());
This subtly changes the behavior in a way that might cause a regression (size
initialization is quite timing-sensitive) and I don't think it's needed to solve
the bug  so I suggest not touching this?

[dshwang, 2013-06-19 14:25:03]

Hi, thx for reivew! I apologize long interval.

On 2013/05/29 18:47:28, aelias wrote:
> It looks like there is no layout test explicitly turning scrollbars on and off
> (unless I'm missing something), could you add one?

That's good question. addBlock() in resize-events.html already turns on
scrollbars. addBlock() adds many div blocks to show scrollbars by increasing
document height.
I changed from addBlock() to showScrollbar() to clarify.


>
https://codereview.chromium.org/14766010/diff/18001/Source/core/page/FrameVie...
> File Source/core/page/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/14766010/diff/18001/Source/core/page/FrameVie...
> Source/core/page/FrameView.cpp:137: static inline IntSize viewportSize(const
> FrameView* view)
> I suggest not having this static method and just calling view->layoutSize(). 
> Please move the comment into your layout test.
Done.

>
https://codereview.chromium.org/14766010/diff/18001/Source/core/platform/Scro...
> File Source/core/platform/ScrollView.cpp (right):
> 
>
https://codereview.chromium.org/14766010/diff/18001/Source/core/platform/Scro...
> Source/core/platform/ScrollView.cpp:218: ASSERT(!m_fixedLayoutSize.isEmpty());
> This subtly changes the behavior in a way that might cause a regression (size
> initialization is quite timing-sensitive) and I don't think it's needed to
solve
> the bug  so I suggest not touching this?
You're right. I removed this unnecessary change.

[aelias_OOO_until_Jul13, 2013-06-19 21:15:01]

lgtm (although I'm not a Blink owner, abarth@ or jamesr@ need to approve)

[abarth-chromium, 2013-06-20 01:41:29]

I'm probably not the best reviewer for this CL.  I'd run git blame on
FrameView.cpp to see who has reviewed interesting changes to it.

[aelias_OOO_until_Jul13, 2013-06-20 02:29:40]

Adding leviw@ as reviewer.

[dshwang, 2013-06-24 05:38:55]

On 2013/06/20 02:29:40, aelias wrote:
> Adding leviw@ as reviewer.

thx for lgtm :)

hi Levi, could you have chance to review?

[leviw_travelin_and_unemployed, 2013-06-26 16:14:18]

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
File LayoutTests/fast/events/resize-events-count.html (right):

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resize-events-count.html:34: // No resize events are
acceptable.
This comment is no longer correct.

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resize-events-count.html:35: if (resizecount == 1)
How about making these a script tests (js-test-pre.js)? They could also probably
all be rolled into one.

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
File LayoutTests/fast/events/resize-events-fixed-layout.html (right):

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resize-events-fixed-layout.html:61: // No resize events
are acceptable.
Ditto comment.

[dshwang, 2013-06-26 17:01:35]

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
File LayoutTests/fast/events/resize-events-count.html (right):

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resize-events-count.html:35: if (resizecount == 1)
On 2013/06/26 16:14:19, Levi wrote:
> How about making these a script tests (js-test-pre.js)? They could also
probably
> all be rolled into one.

Thx for review.

In my understanding, you want to minimize duplicated code of resize-events,
resize-events-count, and resize-evnets-fixed-layout.
finish() function is most duplicated code, but three finish() is slightly
different.

imho, following steps don't make more succinct code.
1. add resize-event-pre-test.js
2. define finish(var condition, var containerElement) function.
3. 3 files use the finish(..) function.

If I don't understand correctly, could you explain again?

[leviw_travelin_and_unemployed, 2013-06-26 17:22:53]

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
File LayoutTests/fast/events/resize-events-count.html (right):

https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resize-events-count.html:35: if (resizecount == 1)
On 2013/06/26 17:01:36, dshwang wrote:
> On 2013/06/26 16:14:19, Levi wrote:
> > How about making these a script tests (js-test-pre.js)? They could also
> probably
> > all be rolled into one.
> 
> Thx for review.
> 
> In my understanding, you want to minimize duplicated code of resize-events,
> resize-events-count, and resize-evnets-fixed-layout.
> finish() function is most duplicated code, but three finish() is slightly
> different.
> 
> imho, following steps don't make more succinct code.
> 1. add resize-event-pre-test.js
Not a new js file. There's already a js-test-pre.js that gives you useful
functions such as shouldBe and shouldBeEqual. Using it also has the effect of
making our tests more consistent, and therefor easier to maintain and
understand.

> 2. define finish(var condition, var containerElement) function.
> 3. 3 files use the finish(..) function.
I'm not suggesting you do it specifically this way. I'm suggesting that you can
have 3 tests that you run with a cleanup step in between that are all in one
file instead of 3 separate files. I consider this a "nice to have" so if you
switch to using our js test harness I'm happy to let this go in.

> 
> If I don't understand correctly, could you explain again?

[dshwang, 2013-06-26 18:13:04]

On 2013/06/26 17:22:53, Levi wrote:
> Not a new js file. There's already a js-test-pre.js that gives you useful
> functions such as shouldBe and shouldBeEqual. Using it also has the effect of
> making our tests more consistent, and therefor easier to maintain and
> understand.

> I'm not suggesting you do it specifically this way. I'm suggesting that you
can
> have 3 tests that you run with a cleanup step in between that are all in one
> file instead of 3 separate files. I consider this a "nice to have" so if you
> switch to using our js test harness I'm happy to let this go in.

I understand what you said. Thank for kind explanation!

[leviw_travelin_and_unemployed, 2013-06-27 17:45:40]

Thanks! lgtm.

[commit-bot: I haz the power, 2013-06-27 17:55:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14766010/40001

[commit-bot: I haz the power, 2013-06-27 19:38:00]

Change committed as 153167