Adds ImageWriterPrivate.destroyPartitions operation.

Adds ImageWriterPrivate.destroyPartitions operation.

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

[Drew Haven, 2013-12-14 00:11:56]

+tbarzic

Thanks for being available to review, Toni.

[tbarzic, 2013-12-14 01:01:04]

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:19:
// Small wrapper to remove the temporary file and display an error if it fails.
display error where?
If you meant DLOG part, I don't think it's worth mentioning in the comment.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:25:
}
  // namespace

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:31:
: Operation(manager, extension_id, storage_unit_id) {
indent+=2

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:33:
verify_write_ = false;
I'd add this to initializer list

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:37:
}
no need for a nl here

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:47:
char buffer[kPartitionTableSize];
I'd use scoped_array<char> here (32kB is not that small)

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation_unittest.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation_unittest.cc:24:
} // namespace
If possible the whole test file should go in an anonymous namespace

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation_unittest.cc:33:
= new DestroyPartitionsOperation(manager.AsWeakPtr(),
scoped_refptr<...>operation(
    new ....)

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:143:
BrowserThread::FILE,
indent+=2

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:146:
this));
can't this fit in the previous line?

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_manager.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_manager.cc:140: 
nuke the empty line

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_manager.cc:156:
operation.get()));
I don't think you need get()

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/operation_manager_unittest.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_manager_unittest.cc:68:
} // namespace
the tests should be in the namespace too

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/test_utils.h (right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/test_utils.h:23: const int
kTestFileSize = 1024 * 1024;
can't you use a bit smaller numbers :)
I'm not sure we want to create few MB files each test cycle.

[Drew Haven, 2013-12-16 20:15:33]

Thanks for the review, Toni.

I went over your feedback.  The one thing that I didn't get was the scoped_array
thing.  Right now codesearch doesn't have base/ indexed, but I couldn't find any
scoped_array in there with grep either.  Perhaps this is only a third_party
thing?

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:19:
// Small wrapper to remove the temporary file and display an error if it fails.
On 2013/12/14 01:01:04, tbarzic wrote:
> display error where?
> If you meant DLOG part, I don't think it's worth mentioning in the comment.

Good point.  It's such a trivial function, but I felt it should have a comment
anyway.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:25:
}
On 2013/12/14 01:01:04, tbarzic wrote:
>   // namespace

Done.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:31:
: Operation(manager, extension_id, storage_unit_id) {
On 2013/12/14 01:01:04, tbarzic wrote:
> indent+=2

Done.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:33:
verify_write_ = false;
On 2013/12/14 01:01:04, tbarzic wrote:
> I'd add this to initializer list

It's a protected variable of the superclass, so I can't put it in the
initializer list, I believe.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:37:
}
On 2013/12/14 01:01:04, tbarzic wrote:
> no need for a nl here

Done.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:47:
char buffer[kPartitionTableSize];
On 2013/12/14 01:01:04, tbarzic wrote:
> I'd use scoped_array<char> here (32kB is not that small)

I'm not finding scoped_arrays in base.  Though they show up in a few of our
third_party libraries.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation_unittest.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation_unittest.cc:24:
} // namespace
On 2013/12/14 01:01:04, tbarzic wrote:
> If possible the whole test file should go in an anonymous namespace

Done.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation_unittest.cc:33:
= new DestroyPartitionsOperation(manager.AsWeakPtr(),
On 2013/12/14 01:01:04, tbarzic wrote:
> scoped_refptr<...>operation(
>     new ....)

Done.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_linux.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:143:
BrowserThread::FILE,
On 2013/12/14 01:01:04, tbarzic wrote:
> indent+=2

Done.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_linux.cc:146:
this));
On 2013/12/14 01:01:04, tbarzic wrote:
> can't this fit in the previous line?

Yep, oops.  Hold-over from a longer set of arguments.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_manager.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_manager.cc:140: 
On 2013/12/14 01:01:04, tbarzic wrote:
> nuke the empty line

And I nuked a few others.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_manager.cc:156:
operation.get()));
On 2013/12/14 01:01:04, tbarzic wrote:
> I don't think you need get()

Indeed.  I also fixed the other places this pattern was being used.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/operation_manager_unittest.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_manager_unittest.cc:68:
} // namespace
On 2013/12/14 01:01:04, tbarzic wrote:
> the tests should be in the namespace too

Done.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/test_utils.h (right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/test_utils.h:23: const int
kTestFileSize = 1024 * 1024;
On 2013/12/14 01:01:04, tbarzic wrote:
> can't you use a bit smaller numbers :)
> I'm not sure we want to create few MB files each test cycle.

I want to make sure we get a few cycles through.  1MB test files are still
result in tests that take <100ms (locally).

I'll do 32kB.  Turns out I can reduce the partition-table size variable to 1k
and I think our writing chunks are around 8k.

[tbarzic, 2013-12-16 20:20:51]

Oh, looks like scoped_array has been replaced by scoped_ptr<T[]>
https://codereview.chromium.org/14081006


On Mon, Dec 16, 2013 at 12:15 PM, <haven@chromium.org> wrote:

> Thanks for the review, Toni.
>
> I went over your feedback.  The one thing that I didn't get was the
> scoped_array
> thing.  Right now codesearch doesn't have base/ indexed, but I couldn't
> find any
> scoped_array in there with grep either.  Perhaps this is only a third_party
> thing?
>
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc
> File
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc
> (right):
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc#newcode19
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc:19:
> // Small wrapper to remove the temporary file and display an error if it
> fails.
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> display error where?
>> If you meant DLOG part, I don't think it's worth mentioning in the
>>
> comment.
>
> Good point.  It's such a trivial function, but I felt it should have a
> comment anyway.
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc#newcode25
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc:25:
> }
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>>    // namespace
>>
>
> Done.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc#newcode31
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc:31:
> : Operation(manager, extension_id, storage_unit_id) {
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> indent+=2
>>
>
> Done.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc#newcode33
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc:33:
> verify_write_ = false;
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> I'd add this to initializer list
>>
>
> It's a protected variable of the superclass, so I can't put it in the
> initializer list, I believe.
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc#newcode37
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc:37:
>
> }
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> no need for a nl here
>>
>
> Done.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc#newcode47
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation.cc:47:
> char buffer[kPartitionTableSize];
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> I'd use scoped_array<char> here (32kB is not that small)
>>
>
> I'm not finding scoped_arrays in base.  Though they show up in a few of
> our third_party libraries.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation_unittest.cc
> File
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation_unittest.cc
> (right):
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation_unittest.cc#newcode24
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation_unittest.cc:24:
> } // namespace
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> If possible the whole test file should go in an anonymous namespace
>>
>
> Done.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation_unittest.cc#newcode33
> chrome/browser/extensions/api/image_writer_private/destroy_
> partitions_operation_unittest.cc:33:
> = new DestroyPartitionsOperation(manager.AsWeakPtr(),
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> scoped_refptr<...>operation(
>>      new ....)
>>
>
> Done.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/operation_linux.cc
> File
> chrome/browser/extensions/api/image_writer_private/operation_linux.cc
> (right):
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/
> operation_linux.cc#newcode143
> chrome/browser/extensions/api/image_writer_private/operation_linux.cc:143:
> BrowserThread::FILE,
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> indent+=2
>>
>
> Done.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/
> operation_linux.cc#newcode146
> chrome/browser/extensions/api/image_writer_private/operation_linux.cc:146:
> this));
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> can't this fit in the previous line?
>>
>
> Yep, oops.  Hold-over from a longer set of arguments.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/operation_manager.cc
> File
> chrome/browser/extensions/api/image_writer_private/operation_manager.cc
> (right):
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/operation_manager.cc#
> newcode140
> chrome/browser/extensions/api/image_writer_private/
> operation_manager.cc:140:
>
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> nuke the empty line
>>
>
> And I nuked a few others.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/operation_manager.cc#
> newcode156
> chrome/browser/extensions/api/image_writer_private/
> operation_manager.cc:156:
> operation.get()));
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> I don't think you need get()
>>
>
> Indeed.  I also fixed the other places this pattern was being used.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/
> operation_manager_unittest.cc
> File
> chrome/browser/extensions/api/image_writer_private/
> operation_manager_unittest.cc
> (right):
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/
> operation_manager_unittest.cc#newcode68
> chrome/browser/extensions/api/image_writer_private/
> operation_manager_unittest.cc:68:
> } // namespace
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> the tests should be in the namespace too
>>
>
> Done.
>
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/test_utils.h
> File chrome/browser/extensions/api/image_writer_private/test_utils.h
> (right):
>
> https://codereview.chromium.org/109793006/diff/20001/
> chrome/browser/extensions/api/image_writer_private/test_utils.h#newcode23
> chrome/browser/extensions/api/image_writer_private/test_utils.h:23:
> const int kTestFileSize = 1024 * 1024;
> On 2013/12/14 01:01:04, tbarzic wrote:
>
>> can't you use a bit smaller numbers :)
>> I'm not sure we want to create few MB files each test cycle.
>>
>
> I want to make sure we get a few cycles through.  1MB test files are
> still result in tests that take <100ms (locally).
>
> I'll do 32kB.  Turns out I can reduce the partition-table size variable
> to 1k and I think our writing chunks are around 8k.
>
> https://codereview.chromium.org/109793006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tbarzic, 2013-12-16 20:33:44]

lgtm

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:33:
verify_write_ = false;
On 2013/12/16 20:15:33, Drew Haven wrote:
> On 2013/12/14 01:01:04, tbarzic wrote:
> > I'd add this to initializer list
> 
> It's a protected variable of the superclass, so I can't put it in the
> initializer list, I believe.

ok. Though, it may make sense to pass verify_write value as the Operation ctor
argument (instead of resetting it here)

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/test_utils.h (right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/test_utils.h:23: const int
kTestFileSize = 1024 * 1024;
On 2013/12/16 20:15:33, Drew Haven wrote:
> On 2013/12/14 01:01:04, tbarzic wrote:
> > can't you use a bit smaller numbers :)
> > I'm not sure we want to create few MB files each test cycle.
> 
> I want to make sure we get a few cycles through.  1MB test files are still
> result in tests that take <100ms (locally).
> 
> I'll do 32kB.  Turns out I can reduce the partition-table size variable to 1k
> and I think our writing chunks are around 8k.

Another option would be to set different values in tests (e.g. by passing
partition_table_size to DetroyPartitionOperation ctor)

[Drew Haven, 2013-12-16 22:05:24]

I switched over to using more scoped_ptr for such buffers.  I'll try to do that
more often in the future if I have more than a few kB.

Thanks again for the review.

Now to make sure it compiles/tests on all platforms and I'll find someone who
can LGTM the comment change in the idl file.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc
(right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/destroy_partitions_operation.cc:33:
verify_write_ = false;
On 2013/12/16 20:33:45, tbarzic wrote:
> On 2013/12/16 20:15:33, Drew Haven wrote:
> > On 2013/12/14 01:01:04, tbarzic wrote:
> > > I'd add this to initializer list
> > 
> > It's a protected variable of the superclass, so I can't put it in the
> > initializer list, I believe.
> 
> ok. Though, it may make sense to pass verify_write value as the Operation ctor
> argument (instead of resetting it here)

I think we'll set that up when/if we need it.

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/test_utils.h (right):

https://codereview.chromium.org/109793006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/test_utils.h:23: const int
kTestFileSize = 1024 * 1024;
On 2013/12/16 20:33:45, tbarzic wrote:
> On 2013/12/16 20:15:33, Drew Haven wrote:
> > On 2013/12/14 01:01:04, tbarzic wrote:
> > > can't you use a bit smaller numbers :)
> > > I'm not sure we want to create few MB files each test cycle.
> > 
> > I want to make sure we get a few cycles through.  1MB test files are still
> > result in tests that take <100ms (locally).
> > 
> > I'll do 32kB.  Turns out I can reduce the partition-table size variable to
1k
> > and I think our writing chunks are around 8k.
> 
> Another option would be to set different values in tests (e.g. by passing
> partition_table_size to DetroyPartitionOperation ctor)

That's a good idea.  I figured it was more dependent on the partion type and so
the caller of the DestroyPartitionOperation shouldn't have to figure out what
value to use.  Maybe some set-for-testing methods.  I'm going to punt it for now
though.  My goal was to set up a good generic file size for running a lot of
things that would be large enough to actually cause some significant use of the
task queues.

[Drew Haven, 2013-12-17 02:22:13]

+asargent

Antony, can you take a quick look to approve the idl file change?

[asargent_no_longer_on_chrome, 2013-12-17 02:48:41]

idl change lgtm

[commit-bot: I haz the power, 2013-12-17 04:34:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/109793006/110001

[commit-bot: I haz the power, 2013-12-17 05:06:23]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests,
media_unittests, message_center_unittests, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-19 19:46:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/109793006/130001

[commit-bot: I haz the power, 2013-12-19 20:52:33]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Drew Haven, 2013-12-20 08:17:00]

On 2013/12/19 20:52:33, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_chromeos for step(s) unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

I'm doing something wrong here.  I'm trying to set up the FakeDBusThreadManager
in test_utils.cc, however, I'm still getting "DBusThreadManager::Get() called
before Initialize()" when I run the Chrome OS tests.  Any idea what that is?

As an aside, is there a quick way to run Chrome OS tests through simple-chrome? 
It seems to only have compile instructions.

[tbarzic, 2013-12-20 18:14:39]

On 2013/12/20 08:17:00, Drew Haven wrote:
> On 2013/12/19 20:52:33, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_chromeos for step(s) unit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> 
> I'm doing something wrong here.  I'm trying to set up the
FakeDBusThreadManager
> in test_utils.cc, however, I'm still getting "DBusThreadManager::Get() called
> before Initialize()" when I run the Chrome OS tests.  Any idea what that is?
> 
> As an aside, is there a quick way to run Chrome OS tests through
simple-chrome? 
> It seems to only have compile instructions.

You set the fake dbus thread manager, but you don't initialize
g_dbus_thread_manager.
Use DBusThreadManager::InitializeForTesting instead of
DBusThreadManager::SetInstanceForTesting, and the crash should go away.

You can run browser tests without simple-chrome (just build them with
GYP_DEFINES having chromeos=1).
If you wanted to run them on ChromeOS, I don't know if it's possible with
simple-chrome workflow (I've never tried it).
It should be possible using run_remote_tests.sh from chroot, but I have to
refresh my memory about this :)

[commit-bot: I haz the power, 2013-12-20 23:48:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/109793006/180001

[commit-bot: I haz the power, 2013-12-21 02:22:02]

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

[commit-bot: I haz the power, 2013-12-24 18:22:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/109793006/180001

[commit-bot: I haz the power, 2013-12-24 19:10:23]

Change committed as 242452