Bug 91044 - [chromium] Introduce CCResourceProvider, replacing TextureAllocator and hiding textures from clients to allow transport
Summary: [chromium] Introduce CCResourceProvider, replacing TextureAllocator and hidin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Labour
URL:
Keywords:
Depends on: 91712
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-11 20:21 PDT by Antoine Labour
Modified: 2013-04-15 08:01 PDT (History)
16 users (show)

See Also:


Attachments
Patch (340.31 KB, patch)
2012-07-11 20:40 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (341.24 KB, patch)
2012-07-12 17:42 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (342.39 KB, patch)
2012-07-16 20:19 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (342.46 KB, patch)
2012-07-18 16:30 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch for landing (342.43 KB, patch)
2012-07-18 18:31 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Labour 2012-07-11 20:21:14 PDT
[chromium] Introduce CCResourceProvider, replacing TextureAllocator and hiding textures from clients to allow transport
Comment 1 Antoine Labour 2012-07-11 20:40:01 PDT
Created attachment 151850 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-11 20:43:12 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Antoine Labour 2012-07-11 20:48:28 PDT
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).
Comment 4 Eric Penner 2012-07-12 12:25:57 PDT
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).
Comment 5 Eric Penner 2012-07-12 12:27:26 PDT
Closing as this is now here:
https://code.google.com/p/chromium/issues/detail?id=137094
Comment 6 Eric Penner 2012-07-12 12:27:44 PDT
(In reply to comment #5)
> Closing as this is now here:
> https://code.google.com/p/chromium/issues/detail?id=137094

OMG Sorry!
Comment 7 Eric Penner 2012-07-12 12:28:10 PDT
Wrong TAB!
Comment 8 Alexandre Elias 2012-07-12 13:15:40 PDT
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?
Comment 9 Antoine Labour 2012-07-12 13:23:23 PDT
(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.
Comment 10 Antoine Labour 2012-07-12 17:42:23 PDT
Created attachment 152111 [details]
Patch
Comment 11 Antoine Labour 2012-07-12 17:45:23 PDT
(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.
Comment 12 Nat Duca 2012-07-13 00:02:55 PDT
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?
Comment 13 Nat Duca 2012-07-13 00:03:10 PDT
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...
Comment 14 Adrienne Walker 2012-07-13 15:21:38 PDT
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.
Comment 15 Antoine Labour 2012-07-16 20:19:47 PDT
Created attachment 152687 [details]
Patch
Comment 16 Antoine Labour 2012-07-16 20:29:37 PDT
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.
Comment 17 Adrienne Walker 2012-07-16 21:09:18 PDT
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
Comment 18 Antoine Labour 2012-07-18 16:30:17 PDT
Created attachment 153137 [details]
Patch
Comment 19 Antoine Labour 2012-07-18 16:59:03 PDT
(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.
Comment 20 Adrienne Walker 2012-07-18 17:03:30 PDT
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.
Comment 21 Nat Duca 2012-07-18 17:24:33 PDT
And to be clear, the first time I looked at this, it looked awesome anyway. :)
Comment 22 WebKit Review Bot 2012-07-18 18:10:15 PDT
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
Comment 23 Antoine Labour 2012-07-18 18:31:39 PDT
Created attachment 153154 [details]
Patch for landing
Comment 24 WebKit Review Bot 2012-07-18 19:46:42 PDT
Comment on attachment 153154 [details]
Patch for landing

Clearing flags on attachment: 153154

Committed r123065: <http://trac.webkit.org/changeset/123065>
Comment 25 WebKit Review Bot 2012-07-18 19:46:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2012-07-18 22:40:12 PDT
Re-opened since this is blocked by 91712
Comment 27 Ryosuke Niwa 2012-07-18 22:56:36 PDT
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.
Comment 28 Adrienne Walker 2012-07-18 23:42:18 PDT
(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.
Comment 29 Ryosuke Niwa 2012-07-18 23:48:30 PDT
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.
Comment 30 Adrienne Walker 2012-07-18 23:58:16 PDT
(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.
Comment 31 Nat Duca 2012-07-19 00:01:45 PDT
(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.
Comment 32 Antoine Labour 2012-07-19 01:07:07 PDT
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.
Comment 33 Ryosuke Niwa 2012-07-19 01:17:24 PDT
(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.