SkColorType instead of (deprecated) SkBitmap::Config

SkColorType instead of (deprecated) SkBitmap::Config

Part of this refactoring was the recognition of a common pattern:
- setConfig + alloc + setImmutable + pass_to_UIResourceBitmap

This CL introduces a direct way on UIResourceBitmap to create such a bitmap, by just specifying its dimensions. This encapsulates internal requirements (e.g. colortype and immutability).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259349

[enne (OOO), 2014-03-18 19:20:20]

https://codereview.chromium.org/197883017/diff/20001/cc/layers/ui_resource_la...
File cc/layers/ui_resource_layer_unittest.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/layers/ui_resource_la...
cc/layers/ui_resource_layer_unittest.cc:68: bitmap.allocN32Pixels(10, 10);
Can this be part of the ctor call too?

Or, I guess I don't understand why some calls to
setConfig+allocPixels+setImmutable get moved to the constructor and others like
this just get changed to allocN32Pixels.

[reed1, 2014-03-18 19:33:07]

https://codereview.chromium.org/197883017/diff/20001/cc/layers/ui_resource_la...
File cc/layers/ui_resource_layer_unittest.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/layers/ui_resource_la...
cc/layers/ui_resource_layer_unittest.cc:68: bitmap.allocN32Pixels(10, 10);
On 2014/03/18 19:20:21, enne wrote:
> Can this be part of the ctor call too?
> 
> Or, I guess I don't understand why some calls to
> setConfig+allocPixels+setImmutable get moved to the constructor and others
like
> this just get changed to allocN32Pixels.

I tried to move all of the older calling pattern over to the new when I could.
In this case, the caller is not creating a UIResourceBitmap, but is calling
SetBitmap() on a layer. Does that api (SetBitmap) really want a UIResourceBitmap
instead?

[enne (OOO), 2014-03-18 19:43:02]

I think the UIResource cleanup looks fine to me.  I don't know that I know
enough about Skia code to say anything about the rest of the changes.

+tomhudson, senorblanco if either of you want to look at this and I can
rubberstamp it?

https://codereview.chromium.org/197883017/diff/20001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (left):

https://codereview.chromium.org/197883017/diff/20001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:174: SkAutoLockPixels locker(*bitmap);
Are these locks not needed anymore?

https://codereview.chromium.org/197883017/diff/20001/cc/layers/ui_resource_la...
File cc/layers/ui_resource_layer_unittest.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/layers/ui_resource_la...
cc/layers/ui_resource_layer_unittest.cc:68: bitmap.allocN32Pixels(10, 10);
On 2014/03/18 19:33:08, reed1 wrote:
> On 2014/03/18 19:20:21, enne wrote:
> > Can this be part of the ctor call too?
> > 
> > Or, I guess I don't understand why some calls to
> > setConfig+allocPixels+setImmutable get moved to the constructor and others
> like
> > this just get changed to allocN32Pixels.
> 
> I tried to move all of the older calling pattern over to the new when I could.
> In this case, the caller is not creating a UIResourceBitmap, but is calling
> SetBitmap() on a layer. Does that api (SetBitmap) really want a
UIResourceBitmap
> instead?

Oh, I see! Sorry for misreading this.  I don't think other locations need a
UIResourceBitmap.

https://codereview.chromium.org/197883017/diff/20001/cc/test/layer_tree_pixel...
File cc/test/layer_tree_pixel_test.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/test/layer_tree_pixel...
cc/test/layer_tree_pixel_test.cc:323: DCHECK_EQ(kPMColor_SkColorType,
bitmap.colorType());
Is this the same check?

[Stephen White, 2014-03-18 20:14:06]

https://codereview.chromium.org/197883017/diff/20001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:175: const void* pixels =
hud_canvas_->peekPixels(&info, &rowBytes);
It looks like peekPixels() can return NULL in some cases. Should be be checking
for that here?

https://codereview.chromium.org/197883017/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:933: image += dy * rowBytes + dx * 4;
IWBN if we could still put the full image in an SkBitmap, and use getAddr32() to
hide the pointer math (but still skip the extractSubset). Not sure if that would
work against what you're trying to accomplish here, though.

https://codereview.chromium.org/197883017/diff/20001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:52: SetOpaque(isOpaque);
This new constructor doesn't seem to be calling setImmutable() on the pixelref.
Is that done elsewhere? (Create() asserts on it anyway, so I assume tests would
blow up in Debug if it wasn't.)

https://codereview.chromium.org/197883017/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1114:
layer_tree_host()->CompositeAndReadback(bitmap->getPixels(),
Possibly-dumb question: why do we no longer need an autolock here? Won't
getPixels() return NULL without it?

[reed1, 2014-03-21 20:49:01]

ptal -- uploaded changes in response to your comments, and to fix unitttest
failures.

https://codereview.chromium.org/197883017/diff/20001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:175: const void* pixels =
hud_canvas_->peekPixels(&info, &rowBytes);
On 2014/03/18 20:14:07, Stephen White wrote:
> It looks like peekPixels() can return <span id="abbrex-spnAbbr_33474"
class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">NULL</span> in some cases. Should be be checking
> for that here?

Done.

https://codereview.chromium.org/197883017/diff/20001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:933: image += dy * rowBytes + dx * 4;
On 2014/03/18 20:14:07, Stephen White wrote:
> <span id="abbrex-spnAbbr_33578" class="abbrex-Abbr abbrex-Abbr_IWBN
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">IWBN</span> if we could still put
the full image in an SkBitmap, and use getAddr32() to
> hide the pointer math (but still skip the extractSubset). Not sure if that
would
> work against what you're trying to accomplish here, though.

Doesn't work against it per-se, but given that this method hands us a direct
pointer, it was straight-forward to work directly with it. Also, a temp bitmap
allocates a temp pixel-ref behind the scenes, and that churn is unnecessary.

https://codereview.chromium.org/197883017/diff/20001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:52: SetOpaque(isOpaque);
On 2014/03/18 20:14:07, Stephen White wrote:
> This new constructor doesn't seem to be calling setImmutable() on the
pixelref.
> Is that done elsewhere? (Create() asserts on it anyway, so I assume tests
would
> blow up in Debug if it wasn't.)

Done.

https://codereview.chromium.org/197883017/diff/20001/cc/test/layer_tree_pixel...
File cc/test/layer_tree_pixel_test.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/test/layer_tree_pixel...
cc/test/layer_tree_pixel_test.cc:323: DCHECK_EQ(kPMColor_SkColorType,
bitmap.colorType());
On 2014/03/18 19:43:03, enne wrote:
> Is this the same check?

Yes. In SkColorType world, we now have two explicit swizzles named: kRGBA and
kBGRA. kPMColor is an alias for which ever one corresponds to the default for
skia's backend (which is exactly what kARGB_8888_Config was used for).

In the future, when we fully support both swizzles (at least on input), the
caller for this may want to pick either, and then we should relax this assert
AND update the swizzle code below.

My change was to just maintain parity with the existing functionality.

https://codereview.chromium.org/197883017/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/197883017/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1114:
layer_tree_host()->CompositeAndReadback(bitmap->getPixels(),
On 2014/03/18 20:14:07, Stephen White wrote:
> Possibly-dumb question: why do we no longer need an autolock here? Won't
> getPixels() return <span id="abbrex-spnAbbr_33603" class="abbrex-Abbr
abbrex-Abbr_NULL abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">NULL</span>
without it?

allocPixels (and its helper variants like allocN32Pixels) *always* also call
lock. This was true before this CL as well, in that the previous AutoLockPixels
code is unnecessary.

[reed1, 2014-03-25 11:39:38]

ptal - are there other questions about the CL?

[Stephen White, 2014-03-25 12:08:54]

LGTM

[reed1, 2014-03-25 12:12:53]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-03-25 12:13:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reed@google.com/197883017/60001

[commit-bot: I haz the power, 2014-03-25 12:15:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 12:15:39]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[reed1, 2014-03-25 13:29:44]

ptal -- cc/ owner

[danakj, 2014-03-25 16:13:59]

https://codereview.chromium.org/197883017/diff/70001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/197883017/diff/70001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:174: size_t rowBytes;
size_t row_bytes = 0;

(name style + always initialize)

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:935: DCHECK(source_rect.width() <=
image_rect.width());
check right/bottom instead of width/height?

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:937: SkImageInfo info =
source_info

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:939: size_t rowBytes = image_rect.width() * 4;
image_row_bytes

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:940: int dx = source_rect.x() -
image_rect.x();
how about:

gfx::Vector2d source_offset = source_rect.origin() - image_rect.origin()
image += source_offset.y() * image_row_bytes + source_offset.x() * 4;

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.cc (right):

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.cc:45: SkAlphaType alphaType = isOpaque ?
kOpaque_SkAlphaType : kPremul_SkAlphaType;
alpha_type

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:46: explicit UIResourceBitmap(const SkBitmap&
skbitmap);
Are there no callers of this constructor anymore then? Can we remove it?

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:47: UIResourceBitmap(int width, int height,
bool isOpaque = false);
this should be |is_opaque| or just |opaque|. however, default argument values
are forbidden by the style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

function overloading is also strongly discouraged, so i'm not sold on just
splitting this into 2 constructors with/out the bool. probably all callsites
should be passing a bool.

also, can you pass a const gfx::Size& instead of a width/height separately?

i'll point out now that in cc we don't pass bool literals to methods directly
since it's unclear what they mean, so callsites would end up looking like:

bool is_opaque = false;
UIResourceBitmap(gfx::Size(10, 10), is_opaque);

https://codereview.chromium.org/197883017/diff/70001/cc/test/layer_tree_json_...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/197883017/diff/70001/cc/test/layer_tree_json_...
cc/test/layer_tree_json_parser.cc:72:
bitmap.setConfig(SkImageInfo::MakeN32Premul(image_width, image_height));
why isn't this allocN32Pixels()?

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (left):

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1117:
scoped_ptr<SkAutoLockPixels> lock(new SkAutoLockPixels(*bitmap));
So what will unlock the bitmap's pixels, or why don't we need to worry about
that anymore?

[reed1, 2014-03-25 17:19:40]

ptal

https://codereview.chromium.org/197883017/diff/70001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/197883017/diff/70001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:174: size_t rowBytes;
On 2014/03/25 16:13:59, danakj wrote:
> size_t row_bytes = 0;
> 
> (name style + always initialize)

Done.

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:935: DCHECK(source_rect.width() <=
image_rect.width());
On 2014/03/25 16:13:59, danakj wrote:
> check right/bottom instead of width/height?

good point. what I really want is image_rect.contains(source_rect)

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:937: SkImageInfo info =
On 2014/03/25 16:13:59, danakj wrote:
> source_info

Done.

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:939: size_t rowBytes = image_rect.width() * 4;
On 2014/03/25 16:13:59, danakj wrote:
> image_row_bytes

Done.

https://codereview.chromium.org/197883017/diff/70001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:940: int dx = source_rect.x() -
image_rect.x();
On 2014/03/25 16:13:59, danakj wrote:
> how about:
> 
> gfx::Vector2d source_offset = source_rect.origin() - image_rect.origin()
> image += source_offset.y() * image_row_bytes + source_offset.x() * 4;

Done.

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:46: explicit UIResourceBitmap(const SkBitmap&
skbitmap);
On 2014/03/25 16:13:59, danakj wrote:
> Are there no callers of this constructor anymore then? Can we remove it?

still used. e.g. cc/layers/painted_scrollbar_layer.cc:275

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:47: UIResourceBitmap(int width, int height,
bool isOpaque = false);
On 2014/03/25 16:13:59, danakj wrote:
> this should be |is_opaque| or just |opaque|. however, default argument values
> are forbidden by the style guide:

I'll remove the defaultness of the argument.

> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
> 
> function overloading is also strongly discouraged, so i'm not sold on just
> splitting this into 2 constructors with/out the bool. probably all callsites
> should be passing a bool.

not sure how "function overloading" meshes with the existing2 constructors (soon
to be 3 or 4).

> 
> also, can you pass a const gfx::Size& instead of a width/height separately?

Done. I see that the exising constructor that takes a pixelref takes a Size
instead of const Size&. I will change it to const& as well.

> 
> i'll point out now that in cc we don't pass bool literals to methods directly
> since it's unclear what they mean, so callsites would end up looking like:
> 
> bool is_opaque = false;
> UIResourceBitmap(gfx::Size(10, 10), is_opaque);

Agreed, this is better than just passing 'false'.

https://codereview.chromium.org/197883017/diff/70001/cc/test/layer_tree_json_...
File cc/test/layer_tree_json_parser.cc (right):

https://codereview.chromium.org/197883017/diff/70001/cc/test/layer_tree_json_...
cc/test/layer_tree_json_parser.cc:72:
bitmap.setConfig(SkImageInfo::MakeN32Premul(image_width, image_height));
On 2014/03/25 16:13:59, danakj wrote:
> why isn't this allocN32Pixels()?

good catch. Done.

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (left):

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1117:
scoped_ptr<SkAutoLockPixels> lock(new SkAutoLockPixels(*bitmap));
On 2014/03/25 16:13:59, danakj wrote:
> So what will unlock the bitmap's pixels, or why don't we need to worry about
> that anymore?

allocPixels() guarantees to lock the pixels automatically. That behavior is not
new, so I am just removing unnecessary code.

[danakj, 2014-03-25 17:31:29]

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/197883017/diff/70001/cc/resources/ui_resource...
cc/resources/ui_resource_bitmap.h:47: UIResourceBitmap(int width, int height,
bool isOpaque = false);
On 2014/03/25 17:19:41, reed1 wrote:
> On 2014/03/25 16:13:59, danakj wrote:
> > this should be |is_opaque| or just |opaque|. however, default argument
values
> > are forbidden by the style guide:
> 
> I'll remove the defaultness of the argument.
> 
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
> > 
> > function overloading is also strongly discouraged, so i'm not sold on just
> > splitting this into 2 constructors with/out the bool. probably all callsites
> > should be passing a bool.
> 
> not sure how "function overloading" meshes with the existing2 constructors
(soon
> to be 3 or 4).

Yeh, I'm not a fan of so many constructors either. Style guide doesn't outright
ban it, it says: "Use overloaded functions (including constructors) only if a
reader looking at a call site can get a good idea of what is happening without
having to first figure out exactly which overload is being called."

When the arguments are very different, call sites should be somewhat clear. But
when one argument is optionally present, it becomes less so, as you are
essentially making a default value.

> 
> > 
> > also, can you pass a const gfx::Size& instead of a width/height separately?
> 
> Done. I see that the exising constructor that takes a pixelref takes a Size
> instead of const Size&. I will change it to const& as well.
> 
> > 
> > i'll point out now that in cc we don't pass bool literals to methods
directly
> > since it's unclear what they mean, so callsites would end up looking like:
> > 
> > bool is_opaque = false;
> > UIResourceBitmap(gfx::Size(10, 10), is_opaque);
> 
> Agreed, this is better than just passing 'false'.

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (left):

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1117:
scoped_ptr<SkAutoLockPixels> lock(new SkAutoLockPixels(*bitmap));
On 2014/03/25 17:19:41, reed1 wrote:
> On 2014/03/25 16:13:59, danakj wrote:
> > So what will unlock the bitmap's pixels, or why don't we need to worry about
> > that anymore?
> 
> allocPixels() guarantees to lock the pixels automatically. That behavior is
not
> new, so I am just removing unnecessary code.

Ya, I wasn't aware of that before, but doing a scoped lock would have unlocked
the pixels. Do we need to worry about leaving the pixels locked indefinitely, is
what I am wondering?

[danakj, 2014-03-25 17:33:43]

Thanks, LGTM

[reed1, 2014-03-25 17:35:53]

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (left):

https://codereview.chromium.org/197883017/diff/70001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1117:
scoped_ptr<SkAutoLockPixels> lock(new SkAutoLockPixels(*bitmap));
On 2014/03/25 17:31:29, danakj wrote:
> On 2014/03/25 17:19:41, reed1 wrote:
> > On 2014/03/25 16:13:59, danakj wrote:
> > > So what will unlock the bitmap's pixels, or why don't we need to worry
about
> > > that anymore?
> > 
> > allocPixels() guarantees to lock the pixels automatically. That behavior is
> not
> > new, so I am just removing unnecessary code.
> 
> Ya, I wasn't aware of that before, but doing a scoped lock would have unlocked
> the pixels. Do we need to worry about leaving the pixels locked indefinitely,
is
> what I am wondering?

For non-lazy bitmaps (as when they are the target for drawing like this),
staying locked all the time is fine, and basically the norm.

The old code performed a lock/unlock pair, so it was also leaving the bitmap
locked (net), since it started out locked.

[reed1, 2014-03-25 17:36:09]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-03-25 17:36:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reed@google.com/197883017/180001

[commit-bot: I haz the power, 2014-03-25 19:35:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 19:35:46]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[danakj, 2014-03-25 19:37:46]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-25 19:38:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reed@google.com/197883017/180001

[commit-bot: I haz the power, 2014-03-25 22:00:53]

Change committed as 259349