Decoupling backbuffer allocation suggestion from frontbuffer allocation suggestion.

Decoupling backbuffer allocation suggestion from frontbuffer allocation suggestion.

Frontbuffer drop was hinged onto the discardBackbuffer message.  Since only a single discard mess is sent now, and since discarding backbuffer comes first, a message frontbuffer drop was waiting for would never come.

This cl just refactors to decouple the two allocations and should be functionally equivalent to current behaviour, except it does not make assumptions about when discard messages should come through.

BUG=126542
TEST=Manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137432

[mmocny, 2012-05-08 14:17:13]

Jonathan, may I get a review?

Al tried this out last night and confirmed it fixed the issue.

The failing try jobs seem to be flake.

[jonathan.backer, 2012-05-09 18:13:01]

There's a lot of minor code in common between the different transport surfaces.
I don't see how to fix that unfortunately.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_linux.cc:366:
OnResize(front_surface_->size());
AFAICT, we're only managing the backbuffer here. So why are we calling it when
the frontbuffer allocation changes?

The only reason we need a front_surface_ to create a back_surface_ is to get a
size. Maybe we should keep this state separately to simplify the code?

If not, maybe some of these early exits should become DCHECKS so that it's
easier to reason about.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_linux.cc:371: return;
Why do we exit early here? Perhaps we should never be in this state (in which
case a DCHECK is appropriate). But I don't understand early exit.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_mac.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_mac.cc:182: CreateIOSurface();
Should we DCHECK that we have a front buffer? Not sure, but it's in the spirit
of the original code.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_win.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_win.cc:145: Resize(visible_size_);
DCHECK that we should have a front buffer?

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_win.cc:194: if
(backbuffer_suggested_allocation_ && frontbuffer_suggested_allocation_)
I think that it may make more sense to DCHECK that we have
frontbuffer_suggested_allocation_ here. I don't that what we want to drop this.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/texture_ima...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/texture_ima...
content/common/gpu/texture_image_transport_surface.cc:171:
AdjustBufferAllocations();
Why are we doing this when AdjustBufferAllocations ignores it?

[mmocny, 2012-05-09 18:59:14]

Regarding your comment on lots of duplicated code:
I can add implementation of "suggested_allocations" to the base class, and
require AdjustMemoryAllocation to be implemented by derived classes.  (I see
something similar was done with GetRegionsToCopy).

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_linux.cc:366:
OnResize(front_surface_->size());
Yes, we need front only to read size, and we should just be caching it.  I was
attempting to limit drive by fixme's and just replicate existing functionality,
but this is probably worth the fix, especially given the next comment..

As for why this gets called on frontbuffer allocation change:  Just for
consistency with the other paths.  It is a noop for now, but that may change.

On 2012/05/09 18:13:01, jonathan.backer wrote:
> AFAICT, we're only managing the backbuffer here. So why are we calling it when
> the frontbuffer allocation changes?
> 
> The only reason we need a front_surface_ to create a back_surface_ is to get a
> size. Maybe we should keep this state separately to simplify the code?
> 
> If not, maybe some of these early exits should become DCHECKS so that it's
> easier to reason about.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_linux.cc:371: return;
Its possible that this is testing for a case that isn't expected and a DCHECK
would suffice.  This is only needed because we expect a front when we recreate,
and if we just cache the size that wont be needed.

On 2012/05/09 18:13:01, jonathan.backer wrote:
> Why do we exit early here? Perhaps we should never be in this state (in which
> case a DCHECK is appropriate). But I don't understand early exit.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_mac.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_mac.cc:182: CreateIOSurface();
I don't think so.  Since this is the minimum we can do to create a backbuffer,
even if we are suggested to not have a frontbuffer, we still have to do this. 
The renderer doesn't care what you frontbuffer suggestion is, but when it calls
ensureBackbuffer, it better be there.

On 2012/05/09 18:13:01, jonathan.backer wrote:
> Should we DCHECK that we have a front buffer? Not sure, but it's in the spirit
> of the original code.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_win.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_win.cc:145: Resize(visible_size_);
Again, I don't think so.
Previously we would set BUFFER_ALLOCATION_FRONT_AND_BACK whenever suggest
backbuffer == true, regardless of frontbuffer suggestion.  Now that the two are
decoupled, we shouldn't assume/necessitate front == true when back == true.
On 2012/05/09 18:13:01, jonathan.backer wrote:
> DCHECK that we should have a front buffer?

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_win.cc:194: if
(backbuffer_suggested_allocation_ && frontbuffer_suggested_allocation_)
Fair enough.  Shouldn't we DCHECK for both?
Will we ever send resize message when not visible?

On 2012/05/09 18:13:01, jonathan.backer wrote:
> I think that it may make more sense to DCHECK that we have
> frontbuffer_suggested_allocation_ here. I don't that what we want to drop
this.

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/texture_ima...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/texture_ima...
content/common/gpu/texture_image_transport_surface.cc:171:
AdjustBufferAllocations();
Just for consistency.  I can remove it from this patch and add it once it does
something.

On 2012/05/09 18:13:01, jonathan.backer wrote:
> Why are we doing this when AdjustBufferAllocations ignores it?

[mmocny, 2012-05-10 17:32:10]

Patch is up.

I tested on windows yesterday, and John Bates tested on Mac for me.

I did not refactor to move common code in the base class.  Some of the
implementations are not 100% the same (like not calling AdjustBufferAllocations
during fronbuffer suggestion changes on platforms that don't discard it).

[jonathan.backer, 2012-05-10 21:37:48]

I think that the code will be simpler still (easier to reason about), if we
adjust the front and back buffers separately.

- on GLX, OSX, and Win7 the front buffers are managed in the browser: allocated
on Swap and dropped on Release or Suspend
- similarly the backbuffers are managed in the GPU process (via Resize or
CreateIOSurface)

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
File content/common/gpu/image_transport_surface_win.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/image_trans...
content/common/gpu/image_transport_surface_win.cc:194: if
(backbuffer_suggested_allocation_ && frontbuffer_suggested_allocation_)
On 2012/05/09 18:59:15, mmocny wrote:
> Fair enough.  Shouldn't we DCHECK for both?
> Will we ever send resize message when not visible?

AFAIK, we shouldn't resize when not visible. I think that resize is meant for
the backbuffer. So it would be strange to do so. 
> On 2012/05/09 18:13:01, jonathan.backer wrote:
> > I think that it may make more sense to DCHECK that we have
> > frontbuffer_suggested_allocation_ here. I don't that what we want to drop
> this.
>

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/texture_ima...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10388010/diff/1/content/common/gpu/texture_ima...
content/common/gpu/texture_image_transport_surface.cc:171:
AdjustBufferAllocations();
On 2012/05/09 18:59:15, mmocny wrote:
> Just for consistency.  I can remove it from this patch and add it once it does
> something.

I'm concerned that order of allocating buffers matters. For example, when a tab
is made visible, we may allocate a front and back buffer when previously we had
none. If we allocate the back and then the front, we will call CreateBackTexture
twice in AdjustBufferAllocations.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_linux.cc:628:
AdjustBufferAllocations();
I think that we can just Resize here (the window is out backbuffer).

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_linux.cc:634:
frontbuffer_suggested_allocation_ = allocated;
I think that we can just ReleaseSurface here when allocated == false and bound_
== true. The frontbuffer is created (in reponse to SwapBuffers) and destroyed in
the browser.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_linux.cc:639: if
(backbuffer_suggested_allocation_) {
We wouldn't need this.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_mac.cc (right):

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_mac.cc:182: CreateIOSurface();
Should only be called for BB allocation. AFAIK, the front buffer will be created
in the browser when we display (on Swap).

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_mac.cc:184: UnrefIOSurface();
I think that we can unref for the BB dealloc.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_mac.cc:185: helper_->Suspend();
AFAIK, this causes the browser to drop the front buffer. Only do it on FB
dealloc.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_mac.cc:185: helper_->Suspend();
I Think that this causes FB dealloc in the browser.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_win.cc (right):

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_win.cc:132:
backbuffer_suggested_allocation_ = allocation;
Resize here accordingly.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_win.cc:140:
AdjustBufferAllocations();
Suspend here accordingly.

http://codereview.chromium.org/10388010/diff/7003/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_win.cc:143: void
PbufferImageTransportSurface::AdjustBufferAllocations() {
nix this.

[mmocny, 2012-05-11 17:10:49]

Updated as per review comments.

I agree, this is cleaner to completely separate out the two allocations. 
However, we can now have certain orderings of messages that we did not before.

Specifically, when >8 windows are open at once, and you switch tabs, you will
now drop FB before dropping BB, and this was previously not possible.

This is almost certainly safe, but I am going to run tests on all the platforms
to confirm.

[jonathan.backer, 2012-05-11 18:43:15]

https://chromiumcodereview.appspot.com/10388010/diff/4005/content/common/gpu/...
File content/common/gpu/image_transport_surface_linux.cc (right):

https://chromiumcodereview.appspot.com/10388010/diff/4005/content/common/gpu/...
content/common/gpu/image_transport_surface_linux.cc:352: if
(!!back_surface_.get() != backbuffer_suggested_allocation_) {
This check seems redundant (just testing my understanding). Don't we always act
immediately?

https://chromiumcodereview.appspot.com/10388010/diff/4005/content/common/gpu/...
content/common/gpu/image_transport_surface_linux.cc:363:
frontbuffer_suggested_allocation_ = allocated;
Can we release the frontbuffer here?

[mmocny, 2012-05-14 17:10:36]

Alright, most recent patch is tested on all platforms except windows (I am
getting some odd build errors, working on it).

Pending confirmation that windows build is ok, does this patch look good?

https://chromiumcodereview.appspot.com/10388010/diff/4005/content/common/gpu/...
File content/common/gpu/image_transport_surface_linux.cc (right):

https://chromiumcodereview.appspot.com/10388010/diff/4005/content/common/gpu/...
content/common/gpu/image_transport_surface_linux.cc:352: if
(!!back_surface_.get() != backbuffer_suggested_allocation_) {
Yes, you are right.
On 2012/05/11 18:43:15, jonathan.backer wrote:
> This check seems redundant (just testing my understanding). Don't we always
act
> immediately?

https://chromiumcodereview.appspot.com/10388010/diff/4005/content/common/gpu/...
content/common/gpu/image_transport_surface_linux.cc:363:
frontbuffer_suggested_allocation_ = allocated;
We can -- but that should be a subsequent patch, right?
On 2012/05/11 18:43:15, jonathan.backer wrote:
> Can we release the frontbuffer here?

[jonathan.backer, 2012-05-15 15:53:30]

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
File content/common/gpu/image_transport_surface_linux.cc (right):

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
content/common/gpu/image_transport_surface_linux.cc:402: if
(!frontbuffer_suggested_allocation_)
Need to set |previous_damage_rect_| here. AFAIK, we don't alternate b/w full
swaps and partial swaps (always just one or the other), but I'd like our code to
support that use case.

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
content/common/gpu/image_transport_surface_linux.cc:434: if
(!frontbuffer_suggested_allocation_)
I think that we need to set |previous_damage_rect_| here. Plus we should
probably be asserting that update region is the full backbuffer --- we rely on
being able to read from the frontbuffer for partial updates and we can't in this
case.

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:213:
DCHECK(backbuffer_suggested_allocation_);
Why not exit early if no frontbuffer (I realize it would NOOP because we aren't
dropping front buffers yet, but you've done this for EGL as well, even though we
aren't dropping those front buffers). If you do add it (your call), the comments
for the EGL path hold.

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
content/common/gpu/texture_image_transport_surface.cc:234:
DCHECK(backbuffer_suggested_allocation_);
Same as above.

[mmocny, 2012-05-15 20:02:27]

Alright, this patch fixes the issues raised.

I've tested again on EGL, and still working on a windows build to varify (should
be done in an hour).

I'm being pestered to land this fix asap since it apparently needs to be in by
tomorrow for beta cutoff, as this fixes a beta blocker.

http://codereview.chromium.org/10388010/diff/13002/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/10388010/diff/13002/content/common/gpu/image_t...
content/common/gpu/image_transport_surface_linux.cc:402: if
(!frontbuffer_suggested_allocation_)
Actually, I think I need to set previous damage to full frame when the
frontbuffer suggestion changes.  Otherwise, dropping and recreating frontbuffer
without a swap will lead to corruption.
On 2012/05/15 15:53:31, jonathan.backer wrote:
> Need to set |previous_damage_rect_| here. AFAIK, we don't alternate b/w full
> swaps and partial swaps (always just one or the other), but I'd like our code
to
> support that use case.

http://codereview.chromium.org/10388010/diff/13002/content/common/gpu/image_t...
content/common/gpu/image_transport_surface_linux.cc:434: if
(!frontbuffer_suggested_allocation_)
I dont think we do read from the frontbuffer (right?), nor should we receive
full frame updates when we dont have one.

We copy from the backbuffer to the frontbuffer all the regions which changed,
for the purpose of ensure that once the front swaps to back it will have the
same contents as the previous backbuffer did.

Since we are not actually swapping buffers when !front, the current backbuffer
will always have the right contents.

The only time we need a full frame update, is right after we re-create the
backbuffer.  To test that, we would probably need a
"backbuffer_was_just_recreated_" flag and then test for that case here.  Is that
worth adding?

On 2012/05/15 15:53:31, jonathan.backer wrote:
> I think that we need to set |previous_damage_rect_| here. Plus we should
> probably be asserting that update region is the full backbuffer --- we rely on
> being able to read from the frontbuffer for partial updates and we can't in
this
> case.

http://codereview.chromium.org/10388010/diff/13002/content/common/gpu/texture...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10388010/diff/13002/content/common/gpu/texture...
content/common/gpu/texture_image_transport_surface.cc:213:
DCHECK(backbuffer_suggested_allocation_);
Fair point, I prefer to add it here than remove it there.

On 2012/05/15 15:53:31, jonathan.backer wrote:
> Why not exit early if no frontbuffer (I realize it would NOOP because we
aren't
> dropping front buffers yet, but you've done this for EGL as well, even though
we
> aren't dropping those front buffers). If you do add it (your call), the
comments
> for the EGL path hold.

[mmocny, 2012-05-15 20:40:53]

Finally got a build on windows.  Verified that this patch is not causing any
issues.

[jonathan.backer, 2012-05-16 02:39:00]

LGTM

I have a slight preference from the preview_damage_rect to be set at the time of
Swap or PostSub because this is consistent with the code flow. Your fix to drop
it at front buffer time is equivalent, but slightly inconsistent with how it is
written.

You mind DCHECKing that the damage rect passed in to PostSub is for the full
frame when we don't have a front buffer to copy from (as per earlier comment)?

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
File content/common/gpu/image_transport_surface_linux.cc (right):

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
content/common/gpu/image_transport_surface_linux.cc:402: if
(!frontbuffer_suggested_allocation_)
We use |previous_damage_rect_| for two frame damage tracking --- it is the
damage on the current front buffer and we get the damage for the backbuffer
passed in to PostSubBuffer.

The region outside of the |previous_damage_rect_| on the frontbuffer is assumed
to be common to the front and backbuffer. In order to make the backbuffer
complete, we copy over the difference between |preview_damage_rect_| and the
|new_damage_rect| passed in to PostSubBuffer from the front to the backbuffer.

Hope this clarifies the role.

And yes, it is probably sufficient to do this reset when the front buffer is
dropped (although it is somewhat inconsistent with how the code flows... at the
end of SwapBuffers and PostSubBuffer we assume that we have created a new front
buffer and adjusted the previous_damage_rect_ accordingly).

IIRC, |previous_damage_rect_| refers to On 2012/05/15 20:02:28, mmocny wrote:
> Actually, I think I need to set previous damage to full frame when the
> frontbuffer suggestion changes.  Otherwise, dropping and recreating
frontbuffer
> without a swap will lead to corruption.
> On 2012/05/15 15:53:31, jonathan.backer wrote:
> > Need to set |previous_damage_rect_| here. AFAIK, we don't alternate b/w full
> > swaps and partial swaps (always just one or the other), but I'd like our
code
> to
> > support that use case.
>

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
content/common/gpu/image_transport_surface_linux.cc:434: if
(!frontbuffer_suggested_allocation_)
On 2012/05/15 20:02:28, mmocny wrote:
> I dont think we do read from the frontbuffer (right?), nor should we receive
> full frame updates when we dont have one.

We often do a copy from the front buffer to the back buffer in this function
(see comment above). However, we wouldn't if the damage passed in to this
function covered the whole buffer (which is why I suggested that we DCHECK this
if we exit early due to lack of frontbuffer to read from).

> We copy from the backbuffer to the frontbuffer all the regions which changed,
> for the purpose of ensure that once the front swaps to back it will have the
> same contents as the previous backbuffer did.

The copy goes the other way: glCopyTexSubImage reads from the current FBO  (with
front buffer as colour attachment) to the currently bound texture (the
backbuffer). We make the backbuffer correct by copying the necessary pieces from
the front just before the swap.
> 
> Since we are not actually swapping buffers when !front, the current backbuffer
> will always have the right contents.
> 
> The only time we need a full frame update, is right after we re-create the
> backbuffer.  To test that, we would probably need a
> "backbuffer_was_just_recreated_" flag and then test for that case here.  Is
that
> worth adding?
> 
> On 2012/05/15 15:53:31, jonathan.backer wrote:
> > I think that we need to set |previous_damage_rect_| here. Plus we should
> > probably be asserting that update region is the full backbuffer --- we rely
on
> > being able to read from the frontbuffer for partial updates and we can't in
> this
> > case.
>

[commit-bot: I haz the power, 2012-05-16 14:36:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmocny@chromium.org/10388010/7015

[mmocny, 2012-05-16 14:41:49]

I think the |previous_damage_rect_| may need to be set at the time of discard
for the case of [discard fb = false, suggest fb = true, PostSubBuff].  Since
suggest_fb is true at the time of the PostSubBuff, we don't reset previous
damage rect in this case, given how it is currently written.  If the region
being posted is guaranteed to be the full frame, then this will not matter, but
that may not be guaranteed.

Either way, neither platform actually drops fb at the moment, so the problem is
punted to subsequent patches.

See notes below regarding your comments on the DCHECK.

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
File content/common/gpu/image_transport_surface_linux.cc (right):

https://chromiumcodereview.appspot.com/10388010/diff/13002/content/common/gpu...
content/common/gpu/image_transport_surface_linux.cc:434: if
(!frontbuffer_suggested_allocation_)
I see, thank you for clearing up the direction of the copy, I had thought of it
going the other direction.  I guess we do it this way because we save a region
copy when the same region is dirty each frame?


This actually has me a little worried about the state of the buffer when
rendering frames in the background (for example, for thumbnailer).  The renderer
is not guaranteed to push a full frame when we do not have a frontbuffer (it
actually has no idea about what state frontbuffer is in).

I put the DCHECK in and ran a bunch of tests and this was not an issue, since we
push a full frame after dropping backbuffer, and we only paint a single frame
for thumbnailer which ends up the whole frame.  So, at the moment, we
coincidentally always push a full frame when drawing without a frontbuffer.

However, I left the DCHECK out of this patch since it is not currently
guaranteed, and I will instead file a separate bug to figure this out.

Also, I think that as long as the buffer contents outside the PostSubBuff region
aren't clobbered, things should work fine without the copy from front in this
eary exit step.

[commit-bot: I haz the power, 2012-05-16 15:55:56]

Change committed as 137432