Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

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

Created:
7 years, 7 months ago by dshwang
Modified:
7 years, 5 months ago
CC:
blink-reviews, jeez, kenneth.r.christiansen, abarth-chromium, jamesr
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Add new test: fast/events/resize-events-count.html #

Total comments: 5

Patch Set 3 : WIP: waiting for landing crbug.com/15023009 #

Patch Set 4 : patch for review #

Total comments: 2

Patch Set 5 : Apply aellas's review #

Total comments: 5

Patch Set 6 : Fix comment nits #

Patch Set 7 : use js-test-pre.js #

Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -30 lines) Patch
M LayoutTests/fast/events/resize-events.html View 1 2 3 4 5 6 1 chunk +27 lines, -22 lines 0 comments Download
A LayoutTests/fast/events/resize-events-count.html View 1 2 3 4 5 6 1 chunk +39 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/resize-events-count-expected.txt View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M LayoutTests/fast/events/resize-events-expected.txt View 1 2 3 4 5 6 1 chunk +4 lines, -5 lines 0 comments Download
A LayoutTests/fast/events/resize-events-fixed-layout.html View 1 2 3 4 5 6 1 chunk +64 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/resize-events-fixed-layout-expected.txt View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/page/FrameView.cpp View 1 2 3 4 5 2 chunks +2 lines, -3 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
dshwang
7 years, 7 months ago (2013-05-05 10:12:52 UTC) #1
dshwang
On 2013/05/05 10:12:52, dshwang wrote: There are three tests to support this issue. 1. resize-events.html ...
7 years, 7 months ago (2013-05-05 10:28:24 UTC) #2
jamesr
7 years, 7 months ago (2013-05-06 19:33:14 UTC) #3
aelias_OOO_until_Jul13
Have you tested that all other browsers don't send resize events in this case?
7 years, 7 months ago (2013-05-06 19:41:02 UTC) #4
kenneth.r.christiansen
https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/resize-events-count-expected.txt File LayoutTests/fast/events/resize-events-count-expected.txt (right): https://codereview.chromium.org/14766010/diff/3001/LayoutTests/fast/events/resize-events-count-expected.txt#newcode1 LayoutTests/fast/events/resize-events-count-expected.txt:1: Test how many resize events are emitted during resizing ...
7 years, 7 months ago (2013-05-06 19:50:51 UTC) #5
dshwang
On 2013/05/06 19:41:02, aelias wrote: > Have you tested that all other browsers don't send ...
7 years, 7 months ago (2013-05-10 11:43:38 UTC) #6
dshwang
7 years, 7 months ago (2013-05-10 11:44:23 UTC) #7
dshwang
7 years, 6 months ago (2013-05-29 14:25:03 UTC) #8
aelias_OOO_until_Jul13
It looks like there is no layout test explicitly turning scrollbars on and off (unless ...
7 years, 6 months ago (2013-05-29 18:47:28 UTC) #9
dshwang
Hi, thx for reivew! I apologize long interval. On 2013/05/29 18:47:28, aelias wrote: > It ...
7 years, 6 months ago (2013-06-19 14:25:03 UTC) #10
aelias_OOO_until_Jul13
lgtm (although I'm not a Blink owner, abarth@ or jamesr@ need to approve)
7 years, 6 months ago (2013-06-19 21:15:01 UTC) #11
abarth-chromium
I'm probably not the best reviewer for this CL. I'd run git blame on FrameView.cpp ...
7 years, 6 months ago (2013-06-20 01:41:29 UTC) #12
aelias_OOO_until_Jul13
Adding leviw@ as reviewer.
7 years, 6 months ago (2013-06-20 02:29:40 UTC) #13
dshwang
On 2013/06/20 02:29:40, aelias wrote: > Adding leviw@ as reviewer. thx for lgtm :) hi ...
7 years, 6 months ago (2013-06-24 05:38:55 UTC) #14
leviw_travelin_and_unemployed
https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/resize-events-count.html File LayoutTests/fast/events/resize-events-count.html (right): https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/resize-events-count.html#newcode34 LayoutTests/fast/events/resize-events-count.html:34: // No resize events are acceptable. This comment is ...
7 years, 6 months ago (2013-06-26 16:14:18 UTC) #15
dshwang
https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/resize-events-count.html File LayoutTests/fast/events/resize-events-count.html (right): https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/resize-events-count.html#newcode35 LayoutTests/fast/events/resize-events-count.html:35: if (resizecount == 1) On 2013/06/26 16:14:19, Levi wrote: ...
7 years, 6 months ago (2013-06-26 17:01:35 UTC) #16
leviw_travelin_and_unemployed
https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/resize-events-count.html File LayoutTests/fast/events/resize-events-count.html (right): https://codereview.chromium.org/14766010/diff/24001/LayoutTests/fast/events/resize-events-count.html#newcode35 LayoutTests/fast/events/resize-events-count.html:35: if (resizecount == 1) On 2013/06/26 17:01:36, dshwang wrote: ...
7 years, 6 months ago (2013-06-26 17:22:53 UTC) #17
dshwang
On 2013/06/26 17:22:53, Levi wrote: > Not a new js file. There's already a js-test-pre.js ...
7 years, 6 months ago (2013-06-26 18:13:04 UTC) #18
leviw_travelin_and_unemployed
Thanks! lgtm.
7 years, 5 months ago (2013-06-27 17:45:40 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14766010/40001
7 years, 5 months ago (2013-06-27 17:55:27 UTC) #20
commit-bot: I haz the power
7 years, 5 months ago (2013-06-27 19:38:00 UTC) #21
Message was sent while issue was closed.
Change committed as 153167

Powered by Google App Engine
This is Rietveld 408576698