Introducing the diagnostic model classes...

Introducing the diagnostic model classes

Chrome diagnostics wants to be a MVC, this is the classes that compose the model part.

BUG=27885
TEST=included unit tests




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

[huanr, 2009-11-18 00:13:19]

This looks good.

http://codereview.chromium.org/385144/diff/2002/2004
File chrome/browser/diagnostics/diagnostics_model.cc (right):

http://codereview.chromium.org/385144/diff/2002/2004#newcode20
Line 20: // list of tests and can loop over the main job of the platform
specific
looks like a missing period somewhere.

http://codereview.chromium.org/385144/diff/2002/2004#newcode52
Line 52: observer->OnDoneAll(this);
So OnDoneAll is called whenever RunTest fails? This behavior is different from
the behavior described in header file.

http://codereview.chromium.org/385144/diff/2002/2004#newcode79
Line 79: virtual int GetId() { return 0; }
Consider having a central place to add IDs.

[cpu_(ooo_6.6-7.5), 2009-11-18 01:12:23]

CL updated please look again.

http://codereview.chromium.org/385144/diff/2002/2004
File chrome/browser/diagnostics/diagnostics_model.cc (right):

http://codereview.chromium.org/385144/diff/2002/2004#newcode20
Line 20: // list of tests and can loop over the main job of the platform
specific
On 2009/11/18 00:13:19, huanr wrote:
> looks like a missing period somewhere.

Done.

http://codereview.chromium.org/385144/diff/2002/2004#newcode52
Line 52: observer->OnDoneAll(this);
On 2009/11/18 00:13:19, huanr wrote:
> So OnDoneAll is called whenever RunTest fails? This behavior is different from
> the behavior described in header file.

I can see your confusion. My bad. clarified the comments.

http://codereview.chromium.org/385144/diff/2002/2004#newcode79
Line 79: virtual int GetId() { return 0; }
On 2009/11/18 00:13:19, huanr wrote:
> Consider having a central place to add IDs. 

Yes, I am going to have to change this but I want to do it with the next batch.

[huanr, 2009-11-18 05:12:59]

http://codereview.chromium.org/385144/diff/6001/6003
File chrome/browser/diagnostics/diagnostics_model.cc (right):

http://codereview.chromium.org/385144/diff/6001/6003#newcode67
Line 67: int tests_run_;
Add disallow_copy_and_assign micro for it and other classes.

http://codereview.chromium.org/385144/diff/6001/6004
File chrome/browser/diagnostics/diagnostics_test.h (right):

http://codereview.chromium.org/385144/diff/6001/6004#newcode25
Line 25: DiagnosticTest(const string16& title)
Define this explicit.

[cpu_(ooo_6.6-7.5), 2009-11-18 21:36:07]

all comments applied, please look again.

http://codereview.chromium.org/385144/diff/6001/6003
File chrome/browser/diagnostics/diagnostics_model.cc (right):

http://codereview.chromium.org/385144/diff/6001/6003#newcode67
Line 67: int tests_run_;
On 2009/11/18 05:12:59, huanr wrote:
> Add disallow_copy_and_assign micro for it and other classes.

Done.

http://codereview.chromium.org/385144/diff/6001/6004
File chrome/browser/diagnostics/diagnostics_test.h (right):

http://codereview.chromium.org/385144/diff/6001/6004#newcode25
Line 25: DiagnosticTest(const string16& title)
On 2009/11/18 05:12:59, huanr wrote:
> Define this explicit.
> 
> 

Done.

[huanr, 2009-11-18 22:24:29]

LGTM