Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411463004:
Allow local root FrameView to be throttled (Closed)
DescriptionAllow local root FrameView to be throttled
BUG=487937, 529785
Committed: https://crrev.com/291f511bd6ed7167e6b75723eebedf1fd8103a44
Cr-Commit-Position: refs/heads/master@{#367537}
Patch Set 1 #Patch Set 2 : Earlier early-out. #Patch Set 3 : Added a test. #Patch Set 4 : Less poking of internals. #Patch Set 5 : Rebased. #
Total comments: 5
Patch Set 6 : Rebased. #
Total comments: 4
Patch Set 7 : Simplify string handling. #Patch Set 8 : Fixed a memory leak in the test. #
Messages
Total messages: 26 (9 generated)
Description was changed from ========== Allow local root FrameView to be throttled BUG=487937 ========== to ========== Allow local root FrameView to be throttled BUG=487937 ==========
skyostil@chromium.org changed reviewers: + esprehn@chromium.org
I don't think we currently hit this case but I imagine it could be useful for OOPIF at some point.
On 2015/10/21 13:27:13, Sami wrote: > I don't think we currently hit this case but I imagine it could be useful for > OOPIF at some point. I've actually run into flaky failures of that assert in browser tests e.g. here: http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg_... (https://codereview.chromium.org/1393203004/#ps100001)
On 2015/10/21 13:45:51, alogvinov wrote: > On 2015/10/21 13:27:13, Sami wrote: > > I don't think we currently hit this case but I imagine it could be useful for > > OOPIF at some point. > > I've actually run into flaky failures of that assert in browser tests e.g. here: > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg_... > (https://codereview.chromium.org/1393203004/#ps100001) Interesting, looks like you're hitting the magic combo of OOPIF and frame throttling in that new test: command_line->AppendSwitch(switches::kEnableExperimentalWebPlatformFeatures); command_line->AppendSwitch(switches::kSitePerProcess); This isn't very well tested at the moment -- last I tried requestAnimationFrame was pretty broken in OOPIF and still looks like it is -- so you might see animation-related breakage. However I just checked that throttling itself seems to work fine with OOPIF in that we know which things are visible and can update the intersection observers accordingly. I've changed to patch to allow this scenario to work. This should resolve the asserts you were seeing.
skyostil@chromium.org changed reviewers: + ojan@chromium.org
Looks like this slipped through the cracks. PTAL.
Tests?
On 2015/11/17 23:17:57, esprehn wrote: > Tests? Fair point, done. This turned out to be a little more painful than I wanted because I couldn't figure out how to construct and manipulate a frame tree with a remote root without poking at some of the internals.
Description was changed from ========== Allow local root FrameView to be throttled BUG=487937 ========== to ========== Allow local root FrameView to be throttled BUG=487937,529785 ==========
Looks like this came up again in crbug.com/529785 -- any thoughts about the test?
ojan@chromium.org changed reviewers: + creis@chromium.org, dcheng@chromium.org, nasko@chromium.org, ncarter@chromium.org
oopif folks can you take a look at the test and see if there's a way to create remote frames without poking at internal APIs? https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2372: updateViewportIntersectionsForSubtree(); Do we need to update the intersections for the subtree if we're throttled? As I look more closely, do we need to update the intersections in the OnlyUpToLayoutClean early return either? It seems to me that in both cases we only need to do this if we're running the whole pipeline unthrottled (for this subtree). I'm probably missing some complication though. The early return here makes sense to me though. https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:285: frameView->updateAllLifecyclePhases(); Can this be tested via pumping a frame through the Sim framework instead of hooking into frameView directly? I have a vision of limiting the APIs the Sim framework has access to C++ bindings generated from the web IDL + some accessors for reading internal state (e.g. the document lifecycle state). So, if we can, I want to avoid cases where we modify internal state by hooking internals of the code. My goal here is to not add too much test-specific dependencies to the code and to make sure the tests keep testing things that can happen in the real world. For example, this case can happen today, but not necessarily in the same way in the future as the code that calls updateAllLifecyclePhases changes. As to creating a remote root without poking at the internals, it would definitely be ideal if we could find a way to enable the oopif flag and create the frames without poking at internals. Maybe someone on the oopif (cc'ed) team can help figure this out?
[+dcheng, our OOPIF Blink expert] https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:285: frameView->updateAllLifecyclePhases(); On 2015/12/01 01:11:05, ojan wrote: > Can this be tested via pumping a frame through the Sim framework instead of > hooking into frameView directly? I have a vision of limiting the APIs the Sim > framework has access to C++ bindings generated from the web IDL + some accessors > for reading internal state (e.g. the document lifecycle state). So, if we can, I > want to avoid cases where we modify internal state by hooking internals of the > code. My goal here is to not add too much test-specific dependencies to the code > and to make sure the tests keep testing things that can happen in the real > world. For example, this case can happen today, but not necessarily in the same > way in the future as the code that calls updateAllLifecyclePhases changes. > > As to creating a remote root without poking at the internals, it would > definitely be ideal if we could find a way to enable the oopif flag and create > the frames without poking at internals. Maybe someone on the oopif (cc'ed) team > can help figure this out? Daniel, can you check whether there's a better way to exercise RemoteFrames in Blink tests, per Ojan's question? The test looks similar to the WebFrameSwapTests (e.g., the RemoteToLocal ones) to me, so I'm not sure if there's something better to use.
https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:285: frameView->updateAllLifecyclePhases(); On 2015/12/01 at 01:11:05, ojan wrote: > Can this be tested via pumping a frame through the Sim framework instead of hooking into frameView directly? I have a vision of limiting the APIs the Sim framework has access to C++ bindings generated from the web IDL + some accessors for reading internal state (e.g. the document lifecycle state). So, if we can, I want to avoid cases where we modify internal state by hooking internals of the code. My goal here is to not add too much test-specific dependencies to the code and to make sure the tests keep testing things that can happen in the real world. For example, this case can happen today, but not necessarily in the same way in the future as the code that calls updateAllLifecyclePhases changes. > > As to creating a remote root without poking at the internals, it would definitely be ideal if we could find a way to enable the oopif flag and create the frames without poking at internals. Maybe someone on the oopif (cc'ed) team can help figure this out? oopi isn't really controlled by any flags in Blink itself: it's something controlled by the embedder. Unfortunately, that means a lot of hand-rolled code today. I'm hoping to integrate it better into FrameTestHelpers::WebViewHelper: the way I imagine it, tests would be able to supply a policy object, and FrameTestHelpers would use it to create local/remote frames as appropriate. Layout tests would probably have to interact with window.internals to get the exact behavior they want though. The main problem is we still need some abstractions in the test layer to represent this: today, the notion of a local frame and its associated remote frames is all maintained in //content, and nothing of that sort exists in Blink... yet. I'm hoping to tackle this in January.
It sounds like right now there isn't an easy way to simplify the tests without some more ground work. Do we want to keep the more complicated tests for now and replace them with something better later? I also took a brief look whether there'd be a good place to test this from content/ at a higher level, but I didn't really find anything. https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1411463004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2372: updateViewportIntersectionsForSubtree(); On 2015/12/01 01:11:05, ojan wrote: > Do we need to update the intersections for the subtree if we're throttled? > > As I look more closely, do we need to update the intersections in the > OnlyUpToLayoutClean early return either? > > It seems to me that in both cases we only need to do this if we're running the > whole pipeline unthrottled (for this subtree). I'm probably missing some > complication though. > > The early return here makes sense to me though. We need to update the subtree in both cases because otherwise we could end up in a situation where the root becomes throttled but the whole subtree isn't also marked throttled. This leads to failing lifecycle asserts because the subtree's lifecycle is in some random state (because the root was throttled) but the throttled flag isn't set. N.B. updateViewportIntersectionsForSubtree() has early-outs so in the common case it won't do much work.
This seems okay, I'll have to fix Sim to allow testing remote frames... this is pretty gross. Your bots are red too, is that unrelated? lgtm https://codereview.chromium.org/1411463004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): https://codereview.chromium.org/1411463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:275: std::string baseURL("http://internal.test/"); We need to get rid of the std::strings in FrameTestHelpers, this should all be String. https://codereview.chromium.org/1411463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:276: URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(baseURL.c_str()), WebString::fromUTF8("simple_div.html")); you don't need the fromUTF8, WebString can auto create from a literal
This seems okay, I'll have to fix Sim to allow testing remote frames... this is pretty gross. Your bots are red too, is that unrelated? lgtm
> Your bots are red too, is that unrelated? The asan bot flagged a memory leak in the test, which is now fixed. The other bots look like flakes. https://codereview.chromium.org/1411463004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right): https://codereview.chromium.org/1411463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:275: std::string baseURL("http://internal.test/"); On 2015/12/11 10:44:19, esprehn wrote: > We need to get rid of the std::strings in FrameTestHelpers, this should all be > String. Done. https://codereview.chromium.org/1411463004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:276: URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(baseURL.c_str()), WebString::fromUTF8("simple_div.html")); On 2015/12/11 10:44:20, esprehn wrote: > you don't need the fromUTF8, WebString can auto create from a literal Neat, thanks.
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1411463004/#ps140001 (title: "Fixed a memory leak in the test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411463004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411463004/140001
Message was sent while issue was closed.
Description was changed from ========== Allow local root FrameView to be throttled BUG=487937,529785 ========== to ========== Allow local root FrameView to be throttled BUG=487937,529785 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Allow local root FrameView to be throttled BUG=487937,529785 ========== to ========== Allow local root FrameView to be throttled BUG=487937,529785 Committed: https://crrev.com/291f511bd6ed7167e6b75723eebedf1fd8103a44 Cr-Commit-Position: refs/heads/master@{#367537} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/291f511bd6ed7167e6b75723eebedf1fd8103a44 Cr-Commit-Position: refs/heads/master@{#367537} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
