Drop background color optimization for composited layers

Drop background color optimization for composited layers

Rationale
1. GraphicsLayer deals with background-color layer as a contents layer, so a
background-color layer is in trouble in terms of the correct order with negative
z-order children.
2. CompositedLayerMapping has the big hack to decide the given layer can be
background-color layer. Unfortunately, the big hack is incomplete and invasive.
3. Hybrid Accelerated Rasterization will cover this optimization in more generic way.
4. background-color layer covers very limited cases. A layer-promoted box
object must not have children, not have decoration, only have a background-color
property.

BUG=114658, 340691
TEST=compositing/background-color/background-color-drawn-over-child.html,
compositing/*

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

[dshwang, 2014-02-25 18:18:31]

I don't update TestExpectations and >130 tests' text results yet so that
reviewers read CL more conveniently.

[dshwang, 2014-02-25 18:44:48]

~130 tests' text results should be changed as follows:
+                  (drawsContent 1)
+                  (backgroundColor #FFFF00)

~10 tests' pixel test results should be changed because of off-by-one pixel
changes, because now PictureLayer draws the given layer instead of
SolidColorLayer.

https://codereview.chromium.org/178013003/diff/1/Source/core/rendering/Compos...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/178013003/diff/1/Source/core/rendering/Compos...
Source/core/rendering/CompositedLayerMapping.cpp:1603: if
(!paintsIntoOwningLayer() || paintsIntoCompositedAncestor() ||
m_artificiallyInflatedBounds || m_owningLayer->isReflection())
I guess isSimpleContainerCompositingLayer() was implemented to check the given
layer can be background-color layer. However this method abuses
isSimpleContainerCompositingLayer() for other purpose.
In this CL, I don't change isSimpleContainerCompositingLayer() logic except for
renaming to paintsIntoOwningLayer(). I guess paintsIntoOwningLayer() needs
refactoring.

[abarth-chromium, 2014-02-26 20:58:41]

This is exciting!  We were looking at removing this optimization as well.  It's
a big part of the toggle_drawer profile.

[abarth-chromium, 2014-02-26 21:05:26]

Some context:

1) isSimpleContainerCompositingLayer is 2.5% of the main thread time on Silk's
toggle_drawer test case.

2) This code is n^2 because we walk the layer tree to update compositing, then
at each node in that walk, this code walks all the decendants in
hasVisibleNonCompositingDescendant.

It would be nice to remove isSimpleContainerCompositingLayer entirely.  It looks
like there's one caller left after this CL.

[abarth-chromium, 2014-02-26 21:07:26]

3) isSimpleContainerCompositingLayer is called during style recalc, but it looks
at the decendants, which haven't had their style calculated yet.  That means
this code is wrong when called during the chicken/egg compositing update.

[jamesr, 2014-02-26 21:09:04]

On 2014/02/26 21:05:26, abarth wrote:
> Some context:
> 
> 1) isSimpleContainerCompositingLayer is 2.5% of the main thread time on Silk's
> toggle_drawer test case.
> 
> 2) This code is n^2 because we walk the layer tree to update compositing, then
> at each node in that walk, this code walks all the decendants in
> hasVisibleNonCompositingDescendant.
> 
> It would be nice to remove isSimpleContainerCompositingLayer entirely.  It
looks
> like there's one caller left after this CL.

isSimpleContainerCompositingLayer is pretty important.  For really common cases
like:

<canvas id="usedForWebgl">

we naively would create two composited layers - one painted compositing layer to
draw the <canvas> element's border, background, etc etc and then one
texture-backed layer for the WebGL content itself.  In many many cases, the
<canvas> element itself has no border/background/whatever and so we can simply
avoid generating this layer at all.  That's what
isSimpleContainerCompositingLayer() tells you.  If we didn't do this, we'd have
an extra compositing layer to paint (with all fully transparent pixels), upload,
and composite.

I think the bigger problem is that this check is being performed at the wrong
time and really should be done later in a linear time fashion, but I haven't
studied this closely.

That aside, the solid color layer optimization isn't terribly important.  I
think it's nearly free in addition to the other checks that we have to do
anyway, but I could be wrong about that.  Absent numbers about how effective it
is we probably don't need it.

[esprehn, 2014-02-26 21:19:08]

On 2014/02/26 21:09:04, jamesr wrote:
> On 2014/02/26 21:05:26, abarth wrote:
> > Some context:
> > 
> > 1) isSimpleContainerCompositingLayer is 2.5% of the main thread time on
Silk's
> > toggle_drawer test case.
> > 
> > 2) This code is n^2 because we walk the layer tree to update compositing,
then
> > at each node in that walk, this code walks all the decendants in
> > hasVisibleNonCompositingDescendant.
> > 
> > It would be nice to remove isSimpleContainerCompositingLayer entirely.  It
> looks
> > like there's one caller left after this CL.
> 
> isSimpleContainerCompositingLayer is pretty important.  For really common
cases
> like:
> 
> <canvas id="usedForWebgl">
> 
> we naively would create two composited layers - one painted compositing layer
to
> draw the <canvas> element's border, background, etc etc and then one
> texture-backed layer for the WebGL content itself.  In many many cases, the
> <canvas> element itself has no border/background/whatever and so we can simply
> avoid generating this layer at all.  That's what
> isSimpleContainerCompositingLayer() tells you.  If we didn't do this, we'd
have
> an extra compositing layer to paint (with all fully transparent pixels),
upload,
> and composite.
> 

This is the wrong layer of the system to do this. We should notice that ::paint
did no work, or better yet just abort from paint entirely and then the
GraphicsLayer should never even get a backing.

This big brittle stack of cases needs to go. The decision should be around a
"didPaintAnything()" like operation.

[jamesr, 2014-02-26 21:21:38]

That's fine, so long as we do something somewhere.  What I'm saying is that we
can't afford to do nothing - we'll double memory bandwidth for webgl, etc.

[dshwang, 2014-02-26 21:54:46]

To clarify, I don't remove isSimpleContainerCompositingLayer(). I just rename it
to paintsIntoOwningLayer().

In detail,
isSimpleContainerCompositingLayer was used for 2 purpose.
1. as jamesr@ mentioned, check that webgl needs additional layer for border or
something.
2. background-color layer optimization.

I remove #2 optimization in this CL, so now isSimpleContainerCompositingLayer is
called by only #1 case. Only containsPaintedContent() calls it.

So now fixing chicken-egg problem is easier. I think esprehn@'s solution sounds
good.

[jamesr, 2014-02-26 22:00:18]

For the record lgtm on the concept of removing #2, although someone else should
look at the code

[alokp, 2014-02-26 22:11:16]

https://codereview.chromium.org/178013003/diff/1/Source/core/rendering/Compos...
File Source/core/rendering/CompositedLayerMapping.h (right):

https://codereview.chromium.org/178013003/diff/1/Source/core/rendering/Compos...
Source/core/rendering/CompositedLayerMapping.h:267: bool paintsIntoOwningLayer()
const;
I do not understand what this function is returning. Who paints into owning
layer? Please add a comment.

[enne (OOO), 2014-02-27 00:44:55]

lgtm % alokp's comments

We shouldn't slow down the common case for a special case.  abarth, thanks for
the perf data.

I think the real problem is that isSimpleContainerCompositingLayer asks about
paintsChildren(), which is a recursive tree walk.

A better solution would be (during computeCompositingRequirements) to keep track
of whether a subtree has a visible non-compositing descendent as part of
recursion.  If we did that, it would be easy to revisit this solid color
optimization as it'd just have to check another property or two at that time,
without the perf penalty.

https://codereview.chromium.org/178013003/diff/1/LayoutTests/compositing/back...
File
LayoutTests/compositing/background-color/background-color-drawn-over-child.html
(right):

https://codereview.chromium.org/178013003/diff/1/LayoutTests/compositing/back...
LayoutTests/compositing/background-color/background-color-drawn-over-child.html:5:
background color of #backgournd_color_container div MUST is drawn below all
children.
typo

https://codereview.chromium.org/178013003/diff/1/LayoutTests/compositing/back...
LayoutTests/compositing/background-color/background-color-drawn-over-child.html:27:
background-color: red; /* to draw something. It can be replaced to img tag or
something. */
No need for this comment or the other one.

[dshwang, 2014-02-27 15:36:25]

Thank you for review.

On 2014/02/26 22:11:16, Alok Priyadarshi wrote:
>
https://codereview.chromium.org/178013003/diff/1/Source/core/rendering/Compos...
> Source/core/rendering/CompositedLayerMapping.h:267: bool
paintsIntoOwningLayer()
> const;
> I do not understand what this function is returning. Who paints into owning
> layer? Please add a comment.

I added the comment for paintsIntoOwningLayer().
// Returns true if this layer has content that needs to be rendered by its own
reasons such as box decoration, non-composited visible children, and etc..


On 2014/02/27 00:44:55, enne wrote:
> A better solution would be (during computeCompositingRequirements) to keep
track
> of whether a subtree has a visible non-compositing descendent as part of
> recursion.  If we did that, it would be easy to revisit this solid color
> optimization as it'd just have to check another property or two at that time,
> without the perf penalty.

That's good idea.


>
https://codereview.chromium.org/178013003/diff/1/LayoutTests/compositing/back...
>
LayoutTests/compositing/background-color/background-color-drawn-over-child.html:5:
> background color of #backgournd_color_container div MUST is drawn below all
> children.
> typo

Done. s/is/be/

>
https://codereview.chromium.org/178013003/diff/1/LayoutTests/compositing/back...
>
LayoutTests/compositing/background-color/background-color-drawn-over-child.html:27:
> background-color: red; /* to draw something. It can be replaced to img tag or
> something. */
> No need for this comment or the other one.

I removed it.

[alokp, 2014-02-27 19:22:26]

https://codereview.chromium.org/178013003/diff/40001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.h (right):

https://codereview.chromium.org/178013003/diff/40001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.h:266: // Returns true
if this layer has content that needs to be rendered by its own reasons such as
box decoration, non-composited visible children, and etc..
The comment is still not clear to me. How is it different from
containsPaintedContent? Does this take subtree into account while
containsPaintedContent does not?

The function name sounds like !(paintsIntoCompositedAncestor).

[dshwang, 2014-02-27 19:50:51]

https://codereview.chromium.org/178013003/diff/40001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.h (right):

https://codereview.chromium.org/178013003/diff/40001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.h:266: // Returns true
if this layer has content that needs to be rendered by its own reasons such as
box decoration, non-composited visible children, and etc..
On 2014/02/27 19:22:27, Alok Priyadarshi wrote:
> The comment is still not clear to me. How is it different from
> containsPaintedContent? Does this take subtree into account while
> containsPaintedContent does not?
> 
> The function name sounds like !(paintsIntoCompositedAncestor).

How about hasVisibleContentOrNonCompositingDescendantLayers()?

Actually, paintsIntoOwningLayer() is a subset of containsPaintedContent().

[alokp, 2014-02-28 06:50:26]

https://codereview.chromium.org/178013003/diff/80001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/178013003/diff/80001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1598: bool
CompositedLayerMapping::containsPaintedContent() const
We already have too many functions with similar names. I think it will be less
confusing if the code in isSimpleContainerCompositingLayer is moved into this
function.

Re-arranging the order in which the tests are done would also help performance.
If paintsIntoOwningLayer is expensive then you should not be calling that first.
You should first try trivial checks. Is there any problem with combining the two
functions?

[dshwang, 2014-02-28 10:59:51]

https://codereview.chromium.org/178013003/diff/80001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/178013003/diff/80001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1598: bool
CompositedLayerMapping::containsPaintedContent() const
On 2014/02/28 06:50:28, Alok Priyadarshi wrote:
> We already have too many functions with similar names. I think it will be less
> confusing if the code in isSimpleContainerCompositingLayer is moved into this
> function.
> 
> Re-arranging the order in which the tests are done would also help
performance.
> If paintsIntoOwningLayer is expensive then you should not be calling that
first.
> You should first try trivial checks. 

That's excellent idea.


> Is there any problem with combining the two functions?

I guess no. let me test.

[dshwang, 2014-02-28 11:54:50]

Now containsPaintedContent() contains isSimpleContainerCompositingLayer() logic.
I postpone O(n^2) paintsChildren() as far as possible for optimization.

Could you review again?

[alokp, 2014-02-28 19:07:00]

https://codereview.chromium.org/178013003/diff/140001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/178013003/diff/140001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1563: do {
there is no need for do-while. We can move the test for video after the image
and then return true as soon as it is determined that it is NOT a simple
container.

[dshwang, 2014-02-28 19:54:13]

On 2014/02/28 19:07:00, Alok Priyadarshi wrote:
>
https://codereview.chromium.org/178013003/diff/140001/Source/core/rendering/c...
> File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):
> 
>
https://codereview.chromium.org/178013003/diff/140001/Source/core/rendering/c...
> Source/core/rendering/compositing/CompositedLayerMapping.cpp:1563: do {
> there is no need for do-while. We can move the test for video after the image
> and then return true as soon as it is determined that it is NOT a simple
> container.

yes, right. I did it. could you review again?

[alokp, 2014-02-28 22:57:52]

lgtm. One minor nit.

https://codereview.chromium.org/178013003/diff/160001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/178013003/diff/160001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1599: if
(paintsChildren())
nit: return paintsChildren()

[dshwang, 2014-03-01 13:09:04]

On 2014/02/28 22:57:52, Alok Priyadarshi wrote:
> lgtm. One minor nit.

Thank you for your review. code becomes better.

>
https://codereview.chromium.org/178013003/diff/160001/Source/core/rendering/c...
> File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):
> 
>
https://codereview.chromium.org/178013003/diff/160001/Source/core/rendering/c...
> Source/core/rendering/compositing/CompositedLayerMapping.cpp:1599: if
> (paintsChildren())
> nit: return paintsChildren()

Done.

[dshwang, 2014-03-01 13:09:17]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-03-01 13:09:26]

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

[dshwang, 2014-03-01 13:29:48]

abarth@, speaking toggle_drawer, jank result become a bit better.
*RESULT jank: jank= "30.0429 ms" TO "28.388 ms"

I test toggle_drawer on ivy bridge laptop using following command.
./tools/perf/run_measurement --browser=content-shell-release smoothness
tools/perf/page_sets/key_silk_cases.json --profiler=trace
--page-filter=toggle_drawer


Before
Avg frame_times: 16.358105ms
Sd  frame_times: 2.131485ms
*RESULT jank: jank= 30.0429 ms
*RESULT mean_frame_time: mean_frame_time= 16.358 ms
*RESULT mostly_smooth: mostly_smooth= 1.0 score
RESULT telemetry_page_measurement_results: num_failed= 0 count
RESULT telemetry_page_measurement_results: num_errored= 0 count

After
Avg frame_times: 16.504702ms
Sd  frame_times: 1.699468ms
*RESULT jank: jank= 28.388 ms
*RESULT mean_frame_time: mean_frame_time= 16.505 ms
*RESULT mostly_smooth: mostly_smooth= 1.0 score
RESULT telemetry_page_measurement_results: num_failed= 0 count
RESULT telemetry_page_measurement_results: num_errored= 0 count

[commit-bot: I haz the power, 2014-03-01 14:44:51]

Change committed as 168245

[cbiesinger, 2014-03-03 16:39:40]

I'm reverting this because it seems like this may have caused browser_test
failures on mac:
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/2...

[dshwang, 2014-03-03 17:07:12]

On 2014/03/03 16:39:40, cbiesinger wrote:
> I'm reverting this because it seems like this may have caused browser_test
> failures on mac:
>
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%2520Tests/builds...

It looks like not related to this CL. Unfortunately, I can not test because I
don't have mac.
Crash in browser process, not render process.

@@@STEP_LOG_LINE@TestFailsAsyncFast@0   browser_tests                      
0x37d6912a base::debug::StackTrace::StackTrace() + 26@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@1   browser_tests                      
0x3be8cfb8 content::(anonymous namespace)::DumpStackTraceSignalHandler(int) +
72@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@2   libSystem.B.dylib                  
0x976f105b _sigtramp + 43@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@3   ???                                
0xffffffff 0x0 + 4294967295@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@4   CoreFoundation                     
0x9753f2df __CFRunLoopRun + 2079@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@5   CoreFoundation                     
0x9753e3c4 CFRunLoopRunSpecific + 452@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@6   CoreFoundation                     
0x9753e1f1 CFRunLoopRunInMode + 97@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@7   HIToolbox                          
0x9a899e04 RunCurrentEventLoopInMode + 392@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@8   HIToolbox                          
0x9a899bb9 ReceiveNextEventCommon + 354@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@9   HIToolbox                          
0x9a899a3e BlockUntilNextEventMatchingListInMode + 81@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@10  AppKit                             
0x92c76595 _DPSNextEvent + 847@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@11  AppKit                             
0x92c75dd6 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] +
156@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@12  AppKit                             
0x92c381f3 -[NSApplication run] + 821@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@13  browser_tests                      
0x37d4daae base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) +
446@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@14  browser_tests                      
0x37d4d2fc base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) +
92@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@15  browser_tests                      
0x37da1152 base::MessageLoop::RunHandler() + 162@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@16  browser_tests                      
0x37dbd2ee base::RunLoop::Run() + 30@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@17  browser_tests                      
0x3bed3ee4 content::RunThisRunLoop(base::RunLoop*) + 84@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@18  browser_tests                      
0x3bed3e7a content::RunMessageLoop() + 26@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@19  browser_tests                      
0x37a3b88b WebUITestHandler::WaitForResult() + 155@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@20  browser_tests                      
0x37a3b42e
WebUITestHandler::RunJavaScriptTestWithResult(std::basic_string\u003Cunsigned
short, base::string16_char_traits, std::allocator\u003Cunsigned short> > const&)
+ 206@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@21  browser_tests                      
0x37b14bf5 WebUIBrowserTest::RunJavascriptUsingHandler(std::string const&,
ScopedVector\u003Cbase::Value const> const&, bool, bool,
content::RenderViewHost*) + 309@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@22  browser_tests                      
0x37b1f2cc
WebUIBrowserExpectFailTest::RunJavascriptAsyncTestNoReturn(std::string const&) +
364@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@23  browser_tests                      
0x37b17e81
WebUIBrowserExpectFailTest_TestFailsAsyncFast_Test::RunTestOnMainThread() +
545@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@24  browser_tests                      
0x37d33a8c InProcessBrowserTest::RunTestOnMainThreadLoop() + 204@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@25  browser_tests                      
0x3be8d331 base::internal::Invoker\u003C1,
base::internal::BindState\u003Cbase::internal::RunnableAdapter\u003Cvoid
(content::BrowserTestBase::*)()>, void ()(content::BrowserTestBase*), void
()(content::BrowserTestBase*)>, void
()(content::BrowserTestBase*)>::Run(base::internal::BindStateBase*) + 33@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@26  browser_tests                      
0x3842db1f ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 5055@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@27  browser_tests                      
0x3842c628 ChromeBrowserMainParts::PreMainMessageLoopRun() + 56@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@28  browser_tests                      
0x3b180879 content::BrowserMainLoop::PreMainMessageLoopRun() + 57@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@29  browser_tests                      
0x3b182751 base::internal::Invoker\u003C1,
base::internal::BindState\u003Cbase::internal::RunnableAdapter\u003Cint
(content::BrowserMainLoop::*)()>, int ()(content::BrowserMainLoop*), void
()(base::internal::UnretainedWrapper\u003Ccontent::BrowserMainLoop>)>, int
()(content::BrowserMainLoop*)>::Run(base::internal::BindStateBase*) + 33@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@30  browser_tests                      
0x3b41927f content::StartupTaskRunner::RunAllTasksNow() + 47@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@31  browser_tests                      
0x3b17f029 content::BrowserMainLoop::CreateStartupTasks() + 697@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@32  browser_tests                      
0x3b182e31
content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) +
337@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@33  browser_tests                      
0x3b17c55a content::BrowserMain(content::MainFunctionParams const&) + 170@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@34  browser_tests                      
0x3be8cd76 content::BrowserTestBase::SetUp() + 1046@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@35  browser_tests                      
0x37d32ae0 InProcessBrowserTest::SetUp() + 496@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@36  browser_tests                      
0x3817b6e4 testing::Test::Run() + 212@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@37  browser_tests                      
0x3817c461 testing::TestInfo::Run() + 657@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@38  browser_tests                      
0x3817c85b testing::TestCase::Run() + 347@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@39  browser_tests                      
0x3818281b testing::internal::UnitTestImpl::RunAllTests() + 1371@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@40  browser_tests                      
0x381822a7 testing::UnitTest::Run() + 103@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@41  browser_tests                      
0x37e03066 base::TestSuite::Run() + 182@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@42  browser_tests                      
0x37b14434 ChromeBrowserTestSuiteRunner::RunTestSuite(int, char**) + 68@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@43  browser_tests                      
0x37d314b2 (anonymous namespace)::ChromeTestLauncherDelegate::RunTestSuite(int,
char**) + 34@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@44  browser_tests                      
0x3becf340 content::LaunchTests(content::TestLauncherDelegate*, int, int,
char**) + 752@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@45  browser_tests                      
0x37d31462 LaunchChromeTests(int, ChromeTestSuiteRunner*, int, char**) + 66@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@46  browser_tests                      
0x37b143db main + 75@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@47  browser_tests                      
0x3713c7b5 start + 53@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@48  ???                                
0x0000000b 0x0 + 11@@@
@@@STEP_LOG_LINE@TestFailsAsyncFast@@@@

[cbiesinger, 2014-03-03 17:09:59]

No, it's a timeout, I believe it's waiting for the page to load. I don't have
insight into how this change might cause that, but that's when it seems to have
started..

[dshwang, 2014-03-03 17:20:04]

On 2014/03/03 17:09:59, cbiesinger wrote:
> No, it's a timeout, I believe it's waiting for the page to load. I don't have
> insight into how this change might cause that, but that's when it seems to
have
> started..

Aha, it's in-process tests. Thank you for that. I'll re-land after testing on
mac. I need to borrow mac book..

[cbiesinger, 2014-03-04 01:27:16]

On 2014/03/03 17:20:04, dshwang wrote:
> On 2014/03/03 17:09:59, cbiesinger wrote:
> > No, it's a timeout, I believe it's waiting for the page to load. I don't
have
> > insight into how this change might cause that, but that's when it seems to
> have
> > started..
> 
> Aha, it's in-process tests. Thank you for that. I'll re-land after testing on
> mac. I need to borrow mac book..

I have no idea whether they are in-process - but apparently this patch did cause
some pages not to finish loading, which is what the tests were waiting for.
Reverting it did fix the tests.