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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 16140024: Fix resource scheduling on documents with <frameset>. (Closed)

Created:
7 years, 6 months ago by James Simonsen
Modified:
7 years, 6 months ago
Reviewers:
abarth-chromium
CC:
blink-reviews, eae+blinkwatch, adamk+blink_chromium.org
Visibility:
Public.

Description

Fix resource scheduling on documents with <frameset>. The ResourceScheduler relies on a signal that the parser has seen enough content before it starts loading images. Normally the signal is sent when the parser reaches the <body>. However, documents that use <frameset> typically don't have a <body>, so they don't issue the signal to the scheduler. In extreme cases, such as "forever frames," that means images are never loaded, because the scheduler never receives any signals from the renderer that it's ok to do so. Now, the same signal is sent to the scheduler when the parser encounters either <body> or <frameset>. BUG=244043 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151718

Patch Set 1 #

Total comments: 1

Patch Set 2 : Move to constructor #

Total comments: 1

Patch Set 3 : Check frame presence #

Total comments: 1

Patch Set 4 : Move to insertedInto #

Total comments: 1

Patch Set 5 : Check inDocument #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -7 lines) Patch
A LayoutTests/http/tests/misc/frameset-disables-resource-scheduling.html View 1 chunk +13 lines, -0 lines 0 comments Download
A + LayoutTests/http/tests/misc/frameset-disables-resource-scheduling-expected.txt View 1 chunk +6 lines, -5 lines 0 comments Download
A LayoutTests/http/tests/misc/resources/slow-frame-with-image.php View 1 2 3 1 chunk +49 lines, -0 lines 0 comments Download
M Source/core/html/HTMLFrameSetElement.h View 1 2 3 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/html/HTMLFrameSetElement.cpp View 1 2 3 4 2 chunks +11 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
James Simonsen
7 years, 6 months ago (2013-06-03 22:11:20 UTC) #1
abarth-chromium
https://codereview.chromium.org/16140024/diff/1/Source/core/html/parser/HTMLConstructionSite.cpp File Source/core/html/parser/HTMLConstructionSite.cpp (right): https://codereview.chromium.org/16140024/diff/1/Source/core/html/parser/HTMLConstructionSite.cpp#newcode490 Source/core/html/parser/HTMLConstructionSite.cpp:490: frame->loader()->client()->dispatchWillInsertBody(); This is a bad dependency. We want it ...
7 years, 6 months ago (2013-06-03 22:12:45 UTC) #2
James Simonsen
On 2013/06/03 22:12:45, abarth wrote: > https://codereview.chromium.org/16140024/diff/1/Source/core/html/parser/HTMLConstructionSite.cpp > File Source/core/html/parser/HTMLConstructionSite.cpp (right): > > https://codereview.chromium.org/16140024/diff/1/Source/core/html/parser/HTMLConstructionSite.cpp#newcode490 > ...
7 years, 6 months ago (2013-06-03 22:24:39 UTC) #3
abarth-chromium
https://codereview.chromium.org/16140024/diff/1007/Source/core/html/HTMLFrameSetElement.cpp File Source/core/html/HTMLFrameSetElement.cpp (right): https://codereview.chromium.org/16140024/diff/1007/Source/core/html/HTMLFrameSetElement.cpp#newcode60 Source/core/html/HTMLFrameSetElement.cpp:60: document->frame()->loader()->client()->dispatchWillInsertBody(); Should we wait for the HTMLFrameSetElement to be ...
7 years, 6 months ago (2013-06-03 22:46:48 UTC) #4
James Simonsen
On 2013/06/03 22:46:48, abarth wrote: > https://codereview.chromium.org/16140024/diff/1007/Source/core/html/HTMLFrameSetElement.cpp > File Source/core/html/HTMLFrameSetElement.cpp (right): > > https://codereview.chromium.org/16140024/diff/1007/Source/core/html/HTMLFrameSetElement.cpp#newcode60 > ...
7 years, 6 months ago (2013-06-03 23:16:25 UTC) #5
abarth-chromium
https://codereview.chromium.org/16140024/diff/10001/Source/core/html/HTMLFrameSetElement.cpp File Source/core/html/HTMLFrameSetElement.cpp (right): https://codereview.chromium.org/16140024/diff/10001/Source/core/html/HTMLFrameSetElement.cpp#newcode61 Source/core/html/HTMLFrameSetElement.cpp:61: document->frame()->loader()->client()->dispatchWillInsertBody(); You want to hook HTMLFrameSetElement::insertedInto rather than the ...
7 years, 6 months ago (2013-06-03 23:29:50 UTC) #6
James Simonsen
On 2013/06/03 23:29:50, abarth wrote: > https://codereview.chromium.org/16140024/diff/10001/Source/core/html/HTMLFrameSetElement.cpp > File Source/core/html/HTMLFrameSetElement.cpp (right): > > https://codereview.chromium.org/16140024/diff/10001/Source/core/html/HTMLFrameSetElement.cpp#newcode61 > ...
7 years, 6 months ago (2013-06-04 00:31:51 UTC) #7
abarth-chromium
https://codereview.chromium.org/16140024/diff/13001/Source/core/html/HTMLFrameSetElement.cpp File Source/core/html/HTMLFrameSetElement.cpp (right): https://codereview.chromium.org/16140024/diff/13001/Source/core/html/HTMLFrameSetElement.cpp#newcode206 Source/core/html/HTMLFrameSetElement.cpp:206: document()->frame()->loader()->client()->dispatchWillInsertBody(); Ok, in the second iteration of this CL, ...
7 years, 6 months ago (2013-06-04 00:48:17 UTC) #8
abarth-chromium
LGTM
7 years, 6 months ago (2013-06-04 00:48:21 UTC) #9
James Simonsen
On 2013/06/04 00:48:17, abarth wrote: > https://codereview.chromium.org/16140024/diff/13001/Source/core/html/HTMLFrameSetElement.cpp > File Source/core/html/HTMLFrameSetElement.cpp (right): > > https://codereview.chromium.org/16140024/diff/13001/Source/core/html/HTMLFrameSetElement.cpp#newcode206 > ...
7 years, 6 months ago (2013-06-04 00:58:24 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/16140024/19001
7 years, 6 months ago (2013-06-04 01:06:16 UTC) #11
commit-bot: I haz the power
Retried try job too often on mac_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout_rel&number=8467
7 years, 6 months ago (2013-06-04 02:22:10 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/16140024/19001
7 years, 6 months ago (2013-06-04 02:32:44 UTC) #13
commit-bot: I haz the power
7 years, 6 months ago (2013-06-04 03:22:25 UTC) #14
Message was sent while issue was closed. 
Change committed as 151718

Powered by Google App Engine
This is Rietveld 408576698