Bug 91044 – [chromium] Introduce CCResourceProvider, replacing TextureAllocator and hiding textures from clients to allow transport

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

[chromium] Introduce CCResourceProvider, replacing TextureAllocator and hiding textures from clients to allow transport

[Antoine Labour]

Created attachment 151850 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Antoine Labour]

Comment on attachment 151850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151850&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCRenderer.h:61
> +    };

Note: I'm not sure this belongs here, but implTextureAllocator/contentsTextureAllocator used to be here. Maybe we can move to somewhere else. I don't think the CCResourceProvider should define those. Maybe CCLTHI?

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:50
> +    typedef unsigned ResourceId;

Note: I defined that type mostly to differentiate what are conceptually textures vs resources, but it means I have to include CCResourceProvider.h everywhere. I'm ok using unsigned in client classes instead.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-83
> -    // FIXME: Rename these to add "m_".

sorry - as I went through here, I wanted to fix the FIXME, but that makes this patch huge. Unfortunately it's a mix of the functional change as well as the renaming (to note: in several tests, there were a few priorityCalculator variables hiding the field - I removed them, which should be ok since CCPriorityCalculator doesn't have state).

[Eric Penner]

Comment on attachment 151850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151850&action=review

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:-83
>> -    // FIXME: Rename these to add "m_".
> 
> sorry - as I went through here, I wanted to fix the FIXME, but that makes this patch huge. Unfortunately it's a mix of the functional change as well as the renaming (to note: in several tests, there were a few priorityCalculator variables hiding the field - I removed them, which should be ok since CCPriorityCalculator doesn't have state).

My bad. I did it to keep another huge patch more readable, but obviously I should have just done it (or followed up faster).

[Eric Penner]

Closing as this is now here:
https://code.google.com/p/chromium/issues/detail?id=137094

[Eric Penner]

(In reply to comment #5)
> Closing as this is now here:
> https://code.google.com/p/chromium/issues/detail?id=137094

OMG Sorry!

[Eric Penner]

Wrong TAB!

[Alexandre Elias]

Comment on attachment 151850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151850&action=review

Looks like a good refactoring.  I think we'll want CCResourceProvider to be a base class with virtual methods (for the non-direct-GL implementation), but I can do that in a later patch.

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:65
> +    // Producer interface.

Could you add comments and assertions specifying which methods are expected to be called from the main thread and which from the impl thread?

[Antoine Labour]

(In reply to comment #8)
> (From update of attachment 151850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151850&action=review
> 
> Looks like a good refactoring.  I think we'll want CCResourceProvider to be a base class with virtual methods (for the non-direct-GL implementation), but I can do that in a later patch.

Yeah, we can probably do that. Some of the logic (e.g. resource accounting) would make sense to share across the implementations.
You'll note that many (most of, actually) of the "FIXME: Implement this path for software compositing." moved to CCResourceProvider so that should help simplifying that work.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:65
> > +    // Producer interface.
> 
> Could you add comments and assertions specifying which methods are expected to be called from the main thread and which from the impl thread?

Will do. The expectation is that everything is called from the impl thread, or during the commit step with the impl thread locked.

[Antoine Labour]

Created attachment 152111 [details]
Patch

[Antoine Labour]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 151850 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151850&action=review
> > 
> > Looks like a good refactoring.  I think we'll want CCResourceProvider to be a base class with virtual methods (for the non-direct-GL implementation), but I can do that in a later patch.
> 
> Yeah, we can probably do that. Some of the logic (e.g. resource accounting) would make sense to share across the implementations.
> You'll note that many (most of, actually) of the "FIXME: Implement this path for software compositing." moved to CCResourceProvider so that should help simplifying that work.
> 
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:65
> > > +    // Producer interface.
> > 
> > Could you add comments and assertions specifying which methods are expected to be called from the main thread and which from the impl thread?
> 
> Will do. The expectation is that everything is called from the impl thread, or during the commit step with the impl thread locked.

Newest patch has this. Actually right now, only the impl thread calls it, so I made the asserts stricter.
Note: some tests don't bother separating main thread-side stuff vs impl thread-side stuff, so I can't use CCProxy::isImplThread without heavily modifying all those tests. Instead I assert WTF::currentThread(), which means it's a very loose check in single threaded mode.

[Nat Duca]

Comment on attachment 152111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152111&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:631
> +        copyTextureToFramebuffer(backgroundTexture->beginRenderFrom(), quad->quadRect().size(), quad->drawTransform());

a little creepy that the beginRenderFrom is in here. Move to line before so its obvious? And any where else where this pattern is done?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:721
> +        contentsTexture->endRenderFrom();

I'm really worried about early returns causing forgotten endRenderFroms/Tos.

Would a (mandatory) scoped class help ensure pairing?

Also, just bikeshedding, but why render? When I read render the first few times, I started thinking we're going to endRenderFrom when we get the swapack.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1391
> +    finishCurrentFramebuffer();

finish implies to me glFinish. Longer but more self-explanatory function name possible?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1437
> +void LayerRendererChromium::finishCurrentFramebuffer()

yeah, definitely confused what this thing is doing...

> Source/WebCore/platform/graphics/chromium/cc/CCRenderer.h:60
> +      ContentPool

Content pool?

[Nat Duca]

Comment on attachment 152111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152111&action=review

> I think we'll want CCResourceProvider to be a base class with virtual methods (for the non-direct-GL implementation), but I can do that in a later patch.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:631
> +        copyTextureToFramebuffer(backgroundTexture->beginRenderFrom(), quad->quadRect().size(), quad->drawTransform());

a little creepy that the beginRenderFrom is in here. Move to line before so its obvious? And any where else where this pattern is done?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:721
> +        contentsTexture->endRenderFrom();

I'm really worried about early returns causing forgotten endRenderFroms/Tos.

Would a (mandatory) scoped class help ensure pairing?

Also, just bikeshedding, but why render? When I read render the first few times, I started thinking we're going to endRenderFrom when we get the swapack.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1391
> +    finishCurrentFramebuffer();

finish implies to me glFinish. Longer but more self-explanatory function name possible?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1437
> +void LayerRendererChromium::finishCurrentFramebuffer()

yeah, definitely confused what this thing is doing...

> Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:83
> +            ASSERT(false);

I've heard james say not to use defaults on enum switches because that we've got our compilers set up to warn about this case? ~shrug~

> Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:218
> +    CCResourceProvider::ResourceId id = m_resourceProvider->createResource(pool, size, format, CCResourceProvider::Any);

Just noticing now, this enum value might want a better name so it better documents the positional argument when used in this form...

[Adrienne Walker]

Comment on attachment 152111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152111&action=review

This is great stuff.  :)

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:721
>>> +        contentsTexture->endRenderFrom();
>> 
>> I'm really worried about early returns causing forgotten endRenderFroms/Tos.
>> 
>> Would a (mandatory) scoped class help ensure pairing?
>> 
>> Also, just bikeshedding, but why render? When I read render the first few times, I started thinking we're going to endRenderFrom when we get the swapack.
> 
> I'm really worried about early returns causing forgotten endRenderFroms/Tos.
> 
> Would a (mandatory) scoped class help ensure pairing?
> 
> Also, just bikeshedding, but why render? When I read render the first few times, I started thinking we're going to endRenderFrom when we get the swapack.

I'm going to hop on this bikeshed too.  What about mapForRead/mapForWrite?

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1437
>>> +void LayerRendererChromium::finishCurrentFramebuffer()
>> 
>> yeah, definitely confused what this thing is doing...
> 
> yeah, definitely confused what this thing is doing...

This is the pair to a potential beginRenderTo from bindFramebufferToTexture (render to texture).

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:90
>      // These functions will aquire the texture if possible. If neither haveBackingTexture()
>      // nor canAquireBackingTexture() is true, an ID of zero will be used/returned.

Should these comments be removed too?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:39
> +CCPrioritizedTextureManager::CCPrioritizedTextureManager(size_t maxMemoryLimitBytes, int, int pool)

Can you explicitly use CCRenderer::ResourcePool in  more places? I'm fine if CCResourceProvider is agnostic and just uses an int, but higher level compositor code should be using an enum.

>> Source/WebCore/platform/graphics/chromium/cc/CCRenderer.h:60
>> +      ContentPool
> 
> Content pool?

This is an enum that differentiates the old contentsTextureAllocator (main thread content) vs. implTextureAllocator (render surfaces).

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:171
> +    ASSERT(WTF::currentThread() == m_threadIdentifier);

How many tests fail if this is CCProxy::isImplThread? Can you just flavor with DebugScopedSetImplThread to taste?

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:209
> +#ifndef NDEBUG

#if !ASSERT_DISABLED

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:236
> +    m_texSubImage = adoptPtr(new LayerTextureSubImage(useMapSub));

Having this in one place is really nice cleanup.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:254
> +    if (m_format == GraphicsContext3D::TEXTURE_2D) {
> +        ASSERT(m_externalTextureResource);
> +        resourceProvider->deleteResource(m_externalTextureResource);
> +        m_externalTextureResource = 0;
> +    }
> +
>      m_provider->putCurrentFrame(m_frame);
>      m_frame = 0;

Talking out loud: like the other didDraw FIXME, I think this will also not be true for an embedded compositor.  I think willDraw/didDraw are going to need some serious changes to handle that case.

>> Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:83
>> +            ASSERT(false);
> 
> I've heard james say not to use defaults on enum switches because that we've got our compilers set up to warn about this case? ~shrug~

That's only the case when your switch statement covers every case.

Also, ASSERT_NOT_REACHED.

[Antoine Labour]

Created attachment 152687 [details]
Patch

[Antoine Labour]

Comment on attachment 152111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152111&action=review

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:631
>>> +        copyTextureToFramebuffer(backgroundTexture->beginRenderFrom(), quad->quadRect().size(), quad->drawTransform());
>> 
>> a little creepy that the beginRenderFrom is in here. Move to line before so its obvious? And any where else where this pattern is done?
> 
> a little creepy that the beginRenderFrom is in here. Move to line before so its obvious? And any where else where this pattern is done?

I replaced by the CCScopedLockResourceForRead and stuff, it should look nicer now.

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:721
>>>> +        contentsTexture->endRenderFrom();
>>> 
>>> I'm really worried about early returns causing forgotten endRenderFroms/Tos.
>>> 
>>> Would a (mandatory) scoped class help ensure pairing?
>>> 
>>> Also, just bikeshedding, but why render? When I read render the first few times, I started thinking we're going to endRenderFrom when we get the swapack.
>> 
>> I'm really worried about early returns causing forgotten endRenderFroms/Tos.
>> 
>> Would a (mandatory) scoped class help ensure pairing?
>> 
>> Also, just bikeshedding, but why render? When I read render the first few times, I started thinking we're going to endRenderFrom when we get the swapack.
> 
> I'm going to hop on this bikeshed too.  What about mapForRead/mapForWrite?

I named lockForRead/lockForWrite. I liked map* but I'm afraid it'd conflict with a future map() that gives us mapped pixels. But I'm not attached to it, so either way.
I also added the CCScopedLockResourceFor* classes. They are effectively mandatory.

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1391
>>> +    finishCurrentFramebuffer();
>> 
>> finish implies to me glFinish. Longer but more self-explanatory function name possible?
> 
> finish implies to me glFinish. Longer but more self-explanatory function name possible?

With the CCScopedLockResource* it made sense to remove that function, so the naming problem went away.

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1437
>>>> +void LayerRendererChromium::finishCurrentFramebuffer()
>>> 
>>> yeah, definitely confused what this thing is doing...
>> 
>> yeah, definitely confused what this thing is doing...
> 
> This is the pair to a potential beginRenderTo from bindFramebufferToTexture (render to texture).

It's gone.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:90
>>      // nor canAquireBackingTexture() is true, an ID of zero will be used/returned.
> 
> Should these comments be removed too?

Updated.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:39
>> +CCPrioritizedTextureManager::CCPrioritizedTextureManager(size_t maxMemoryLimitBytes, int, int pool)
> 
> Can you explicitly use CCRenderer::ResourcePool in  more places? I'm fine if CCResourceProvider is agnostic and just uses an int, but higher level compositor code should be using an enum.

As discussed, that would invert the CCRenderer and CCPrioritizedTextureManager layering, so I didn't do it. If we change our mind, and make CCPrioritizedTextureManager aware of pools, I suggest we then hardcode ContentPool here.

>>> Source/WebCore/platform/graphics/chromium/cc/CCRenderer.h:60
>>> +      ContentPool
>> 
>> Content pool?
> 
> This is an enum that differentiates the old contentsTextureAllocator (main thread content) vs. implTextureAllocator (render surfaces).

Added comments.

>> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:171
>> +    ASSERT(WTF::currentThread() == m_threadIdentifier);
> 
> How many tests fail if this is CCProxy::isImplThread? Can you just flavor with DebugScopedSetImplThread to taste?

I'm a little sad in that it makes CCResourceProvider dependent on CCProxy, but it probably provides more safety, so be it.
I peppered DebugScopedSetImplThread in many tests. I'll let you judge.

>> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:209
>> +#ifndef NDEBUG
> 
> #if !ASSERT_DISABLED

Gone.

>> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:254
>>      m_frame = 0;
> 
> Talking out loud: like the other didDraw FIXME, I think this will also not be true for an embedded compositor.  I think willDraw/didDraw are going to need some serious changes to handle that case.

Yes, you're absolutely right, I should have mentioned it here too. I added a similar FIXME.

>>> Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:83
>>> +            ASSERT(false);
>> 
>> I've heard james say not to use defaults on enum switches because that we've got our compilers set up to warn about this case? ~shrug~
> 
> That's only the case when your switch statement covers every case.
> 
> Also, ASSERT_NOT_REACHED.

Done.

>> Source/WebKit/chromium/tests/CCResourceProviderTest.cpp:218
>> +    CCResourceProvider::ResourceId id = m_resourceProvider->createResource(pool, size, format, CCResourceProvider::Any);
> 
> Just noticing now, this enum value might want a better name so it better documents the positional argument when used in this form...

Done.

[Adrienne Walker]

Comment on attachment 152687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152687&action=review

Thanks for all the DebugScopedSetImplThread additions.  I realize that it's a little verbose, but I find it helpful when reading both code and tests to have a better sense of what context function calls are happening in.

The scoped locks are much clearer to read, too.

I'm good with this patch, but I want to give Nat a chance to chime in, since he had a bunch of feedback the first time around.

> Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:34
> +#include "cc/CCProxy.h"

Yeah, it's sad to have this dependency, but it already exists in a number of other places.  We could always split out the thread identifier stuff into a separate class that's smaller than CCProxy.h.  There's no real reason that it has to be in there.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:71
> +    // pipelinig of updates) for the client will need to exist to solve this.

typo

> Source/WebKit/chromium/tests/FakeCCGraphicsContext.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

[Antoine Labour]

Created attachment 153137 [details]
Patch

[Antoine Labour]

(In reply to comment #17)
> (From update of attachment 152687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152687&action=review
> 
> Thanks for all the DebugScopedSetImplThread additions.  I realize that it's a little verbose, but I find it helpful when reading both code and tests to have a better sense of what context function calls are happening in.
> 
> The scoped locks are much clearer to read, too.
> 
> I'm good with this patch, but I want to give Nat a chance to chime in, since he had a bunch of feedback the first time around.


Nat, did you get a chance to look at the new patch?

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:34
> > +#include "cc/CCProxy.h"
> 
> Yeah, it's sad to have this dependency, but it already exists in a number of other places.  We could always split out the thread identifier stuff into a separate class that's smaller than CCProxy.h.  There's no real reason that it has to be in there.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureLayerImpl.cpp:71
> > +    // pipelinig of updates) for the client will need to exist to solve this.
> 
> typo

Done.

> 
> > Source/WebKit/chromium/tests/FakeCCGraphicsContext.h:2
> > + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> 2012

Done.

[Adrienne Walker]

Comment on attachment 153137 [details]
Patch

R=me.  I cornered Nat and he said he was happy with the naming and excited about the scoped locks.  He said if he looked more closely and didn't like it, he'd just file bugs later.

[Nat Duca]

And to be clear, the first time I looked at this, it looked awesome anyway. :)

[WebKit Review Bot]

Comment on attachment 153137 [details]
Patch

Rejecting attachment 153137 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 316 (offset 3 lines).
Hunk #11 succeeded at 360 (offset 3 lines).
Hunk #12 succeeded at 382 (offset 3 lines).
Hunk #13 succeeded at 409 (offset 3 lines).
Hunk #14 succeeded at 433 (offset 2 lines).
patching file Source/WebKit/chromium/tests/TextureCopierTest.cpp
patching file Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13272694

[Antoine Labour]

Created attachment 153154 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 153154 [details]
Patch for landing

Clearing flags on attachment: 153154

Committed r123065: <http://trac.webkit.org/changeset/123065>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 91712

[Ryosuke Niwa]

Build fixes attempts landed in http://trac.webkit.org/changeset/123073 and http://trac.webkit.org/changeset/123074.

Using anonymous namespace is such a bad idea and please build your patch before landing patches.

[Adrienne Walker]

(In reply to comment #27)
> Build fixes attempts landed in http://trac.webkit.org/changeset/123073 and http://trac.webkit.org/changeset/123074.
> 
> Using anonymous namespace is such a bad idea and please build your patch before landing patches.

rniwa: This patch was landed with the commit queue.

The gclient errors were Win only:
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/46459/steps/gclient/logs/stdio

The build errors were Mac only: http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/40233/steps/compile-webkit/logs/stdio

The build error has absolutely nothing to do with anonymous namespaces, and instead is bug https://bugs.webkit.org/show_bug.cgi?id=74625 yet again.  Clang doesn't optimize out all transient uses of the publicly defined OwnPtr copy constructor in the same way that gcc does; in some cases this will generate link failures.  This is an ongoing failure that nobody else seems to feel is a real problem.

If you have a problem with anonymous namespaces, then please propose a style guide change rather than being frustrated at people for using them.  I find them to be a perfectly reasonable alternative to 'static' to prevent things from having external linkage.

[Ryosuke Niwa]

It doesn't change the fact this patch caused a build failure, and using commit-queue doesn't excuse you from causing a build failure.

[Adrienne Walker]

(In reply to comment #29)
> It doesn't change the fact this patch caused a build failure, and using commit-queue doesn't excuse you from causing a build failure.

It's true, but landing a patch that causes a build failure is not the same as landing a patch that doesn't build on any platform.  I'm trying to say that there's no need to accuse anybody here of being irresponsible.

The commit queue is not perfect insurance, and I don't think it's a big deal to just immediately roll out the low percentage of patches that make it through but still break the build.  It happens.

[Nat Duca]

(In reply to comment #29)
> It doesn't change the fact this patch caused a build failure, and using commit-queue doesn't excuse you from causing a build failure.

Oh come on. We're all friends here. Lets be polite.

[Antoine Labour]

Apologies for the build breakage, and thank you for fixing it.

I did indeed build and test on linux, and relied on the cq for other platforms. It seemed reasonable.

[Ryosuke Niwa]

(In reply to comment #30)
> (In reply to comment #29)
> > It doesn't change the fact this patch caused a build failure, and using commit-queue doesn't excuse you from causing a build failure.
> 
> It's true, but landing a patch that causes a build failure is not the same as landing a patch that doesn't build on any platform. I'm trying to say that there's no need to accuse anybody here of being irresponsible.

Sure, I've never accused anyone of being irresponsible. I'm only asking people to build their patches on relevant platforms before landing them especially because most of Chromium developers tend to have multiple machines at their disposal, and this is one of the reasons I develop on Mac so that I can catch Mac specific build failures on my machine and then Linux specific build failures using EWS.

> The commit queue is not perfect insurance, and I don't think it's a big deal to just immediately roll out the low percentage of patches that make it through but still break the build.  It happens.

It creates SVN churns, slows down bot cycles, and disrupts everyone's workflow. I'd argue that rolling out patches should be the last resort.


(In reply to comment #31)
> (In reply to comment #29)
> > It doesn't change the fact this patch caused a build failure, and using commit-queue doesn't excuse you from causing a build failure.
> 
> Oh come on. We're all friends here. Lets be polite.

I'm sorry I didn't mean to be impolite.

On the other hand, causing build failures, then accusing someone who fixed those failures instead of simply rolling out the patch of complaining too much, etc... is considered extremely rude in my culture.

Now, all I wanted to see was a response like the one below I would had a good night sleep but now I'm wide awake and in a really bad mood but whatever. It's not your fault.

(In reply to comment #32)
> Apologies for the build breakage, and thank you for fixing it.
> 
> I did indeed build and test on linux, and relied on the cq for other platforms. It seemed reasonable.

No problem.