First cut of the about:conflicts page, listing all DLLs loaded in the Chrome ...

First cut of the about:conflicts page, listing all DLLs loaded in the Chrome process.

BUG=http://crbug.com/51105, http://crbug.com/57239
TEST=Unit tests included.


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

[tfarina, 2010-11-05 15:36:10]

http://codereview.chromium.org/4524002/diff/1/14
File chrome/browser/enumerate_modules_model.h (right):

http://codereview.chromium.org/4524002/diff/1/14#newcode31
chrome/browser/enumerate_modules_model.h:31: typedef enum {
Please, do not use typedef enum. in C++ we do not need to do:
enum Foo foo; when declaring a variable, so the typedef is not needed.

http://codereview.chromium.org/4524002/diff/1/14#newcode49
chrome/browser/enumerate_modules_model.h:49: NONE      = 1 << 0,
use 0 instead. 1 << 0 will give you 0001 for example, instead of 0000.

[Finnur, 2010-11-05 15:44:19]

You are jumping the gun just a little bit. I'm just seeing if this compiles on
the try servers. :) 

But I'll take this into consideration. Thanks. :)

On 2010/11/05 15:36:10, tfarina wrote:
> http://codereview.chromium.org/4524002/diff/1/14
> File chrome/browser/enumerate_modules_model.h (right):
> 
> http://codereview.chromium.org/4524002/diff/1/14#newcode31
> chrome/browser/enumerate_modules_model.h:31: typedef enum {
> Please, do not use typedef enum. in C++ we do not need to do:
> enum Foo foo; when declaring a variable, so the typedef is not needed.
> 
> http://codereview.chromium.org/4524002/diff/1/14#newcode49
> chrome/browser/enumerate_modules_model.h:49: NONE      = 1 << 0,
> use 0 instead. 1 << 0 will give you 0001 for example, instead of 0000.

[Paweł Hajdan Jr., 2010-11-05 17:07:30]

Drive-by with a test comment, no need to wait for another review by me.

http://codereview.chromium.org/4524002/diff/5002/9013
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/5002/9013#newcode61
chrome/browser/enumerate_modules_model_unittest_win.cc:61: std::string msg =
"Test case no: " + base::IntToString(i);
nit: You can use SCOPED_TRACE instead of repeating msg for each assertion (and
making it possible to forget to do that).

[Finnur, 2010-11-05 20:00:59]

Huan, Nico, Ben, Pavel and Thiago (see below).

Pavel and Thiago, please look at the review comments I responded to.

Ben, this is a refinement of my "upgrade detection" notification dot, which you
reviewed and I've now made slightly more generic to handle an additional case:

- toolbar_view.cc
- toolbar_view.h
- wrench_menu_model.cc
- browser.cc
- browser.h
- chrome_dll_resource.h
- chrome_switches.cc
- chrome_switches.h
- theme_resources.grd

Huan, can you please take a look at the following files (you are familiar with
the gist of this):

- enumerate_modules_model_win.cc
- enumerate_modules_model_win.h
- enumerate_modules_model_unittest_win.cc
- notification_type.h
- about_flags.cc
- chrome_browser.gypi
- chrome_tests.gypi

And Nico, can you please take a look at the following files, which are based on
your Flags DOMUI page:

- browser_about_handler.cc
- browser_resources.grd
- conflicts_ui.cc
- conflicts_ui.h
- dom_ui_factory.cc
- about_conflicts.html
- url_constants.cc
- url_constants.h
- generated_resources.grd

http://codereview.chromium.org/4524002/diff/1/14
File chrome/browser/enumerate_modules_model.h (right):

http://codereview.chromium.org/4524002/diff/1/14#newcode31
chrome/browser/enumerate_modules_model.h:31: typedef enum {
typedef is needed here. This is used in the struct as a type.

On 2010/11/05 15:36:11, tfarina wrote:
> Please, do not use typedef enum. in C++ we do not need to do:
> enum Foo foo; when declaring a variable, so the typedef is not needed.

http://codereview.chromium.org/4524002/diff/1/14#newcode49
chrome/browser/enumerate_modules_model.h:49: NONE      = 1 << 0,
Changed. I needed it to be 1 since I used that to test whether to use a generic
string, but I guess I should just keep that separate. Done.

On 2010/11/05 15:36:11, tfarina wrote:
> use 0 instead. 1 << 0 will give you 0001 for example, instead of 0000.

http://codereview.chromium.org/4524002/diff/5002/9013
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/5002/9013#newcode61
chrome/browser/enumerate_modules_model_unittest_win.cc:61: std::string msg =
"Test case no: " + base::IntToString(i);
Good point. Thanks!

On 2010/11/05 17:07:31, Paweł Hajdan Jr. wrote:
> nit: You can use SCOPED_TRACE instead of repeating msg for each assertion (and
> making it possible to forget to do that).

[Ben Goodger (Google), 2010-11-05 20:09:00]

Nice! UI bits LGTM with the one naming nit below.

http://codereview.chromium.org/4524002/diff/60001/20018
File chrome/browser/views/toolbar_view.h (right):

http://codereview.chromium.org/4524002/diff/60001/20018#newcode146
chrome/browser/views/toolbar_view.h:146: bool ShowIncompatibilityWarning();
ShouldShow... instead of just Show... which sounds imperative rather than
informational.

[tfarina, 2010-11-05 20:09:25]

http://codereview.chromium.org/4524002/diff/1/14
File chrome/browser/enumerate_modules_model.h (right):

http://codereview.chromium.org/4524002/diff/1/14#newcode31
chrome/browser/enumerate_modules_model.h:31: typedef enum {
On 2010/11/05 20:01:04, Finnur wrote:
> typedef is needed here. This is used in the struct as a type.
> 
I don't see a reason as you can't write this as enum ModuleType; and use it in
the struct below. Which error are you getting? I just tried locally with a
simple class and it compiled without the typedef.

[Finnur, 2010-11-05 20:22:47]

If I remove the typedef from ModuleType, when it tries to compile this:

struct Module {
  ModuleType type;
  ...
}

... it throws this error:
error C2146: syntax error : missing ';' before identifier 'type'

If I change struct module (as you suggest) to read:

struct Module {
  enum ModuleType type;
  ...
}

I get failures here instead:

Module entry;
entry.type = LOADED_MODULE;

error C2440: '=' : cannot convert from
'EnumerateModules::<unnamed-type-ModuleType>' to 'ModuleType'

Adding a ModuleType:: or EnumerateModules:: prefix just gives other errors.

Let me take a step back here... a search of 'typedef enum' brings up over 700
hits globally in our codebase. What exactly is the problem with the typedef? 


On 2010/11/05 20:09:25, tfarina wrote:
> http://codereview.chromium.org/4524002/diff/1/14
> File chrome/browser/enumerate_modules_model.h (right):
> 
> http://codereview.chromium.org/4524002/diff/1/14#newcode31
> chrome/browser/enumerate_modules_model.h:31: typedef enum {
> On 2010/11/05 20:01:04, Finnur wrote:
> > typedef is needed here. This is used in the struct as a type.
> > 
> I don't see a reason as you can't write this as enum ModuleType; and use it in
> the struct below. Which error are you getting? I just tried locally with a
> simple class and it compiled without the typedef.

[tfarina, 2010-11-05 20:39:00]

On 2010/11/05 20:22:47, Finnur wrote:
> If I remove the typedef from ModuleType, when it tries to compile this:
> 
> struct Module {
>   ModuleType type;
>   ...
> }
> 
> ... it throws this error:
> error C2146: syntax error : missing ';' before identifier 'type'
> 
> If I change struct module (as you suggest) to read:
> 
> struct Module {
>   enum ModuleType type;
>   ...
> }
> 
No, I didn't suggest that.

class Foo {
  enum T {
  };
  struct S {
    T t;
  };
};

That doesn't compile?

[Finnur, 2010-11-05 20:52:15]

Yes, that does compile. I'm still not sure we care about one way or the other,
though?

On 2010/11/05 20:39:00, tfarina wrote:
> On 2010/11/05 20:22:47, Finnur wrote:
> > If I remove the typedef from ModuleType, when it tries to compile this:
> > 
> > struct Module {
> >   ModuleType type;
> >   ...
> > }
> > 
> > ... it throws this error:
> > error C2146: syntax error : missing ';' before identifier 'type'
> > 
> > If I change struct module (as you suggest) to read:
> > 
> > struct Module {
> >   enum ModuleType type;
> >   ...
> > }
> > 
> No, I didn't suggest that.
> 
> class Foo {
>   enum T {
>   };
>   struct S {
>     T t;
>   };
> };
> 
> That doesn't compile?

[Nico, 2010-11-05 21:31:48]

A bunch of nits, but looks good overall. The two comments I care about are
* The += localization note
* The consistent naming (conflicts/modules/compatibility)

http://codereview.chromium.org/4524002/diff/60001/20003
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4524002/diff/60001/20003#newcode4171
chrome/app/generated_resources.grd:4171: A module with the same name has been
known to conflict with <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.
i recently learned that putting in the product name at runtime doesn't localize
correctly in korean; it's better to put this into chrome_strings and hardcode
"Google Chrome"

http://codereview.chromium.org/4524002/diff/60001/20003#newcode4183
chrome/app/generated_resources.grd:4183: Uninstalling it
can you remove "it" from this and the next 2 strings?

http://codereview.chromium.org/4524002/diff/60001/20008
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20008#newcode105
chrome/browser/browser_about_handler.cc:105: const char kConflictsPath[] =
"conflicts";
#if defined(OS_WIN)

http://codereview.chromium.org/4524002/diff/60001/20008#newcode271
chrome/browser/browser_about_handler.cc:271: kAllAboutPaths[i] == kConflictsPath
||
#if defined(OS_WIN)

http://codereview.chromium.org/4524002/diff/60001/20010
File chrome/browser/dom_ui/conflicts_ui.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20010#newcode28
chrome/browser/dom_ui/conflicts_ui.cc:28: #include "grit/theme_resources.h"
please remove unnecessary includes. all the pref stuff seems to be unused;
there's probably more

http://codereview.chromium.org/4524002/diff/60001/20010#newcode54
chrome/browser/dom_ui/conflicts_ui.cc:54: ~ConflictsUIHTMLSource() {}
remove?

http://codereview.chromium.org/4524002/diff/60001/20010#newcode67
chrome/browser/dom_ui/conflicts_ui.cc:67:
localized_strings.SetString("modulesBlurb",
why are the strings prefixed with "modules" but the constants with
IDS_COMPATIBILITY? can this be made consistent? …and the url is about:conflicts!
It should probably all be prefixed with conflicts / IDS_CONFLICTS then?

http://codereview.chromium.org/4524002/diff/60001/20010#newcode120
chrome/browser/dom_ui/conflicts_ui.cc:120: ConflictsDOMHandler() {}
remove?

http://codereview.chromium.org/4524002/diff/60001/20010#newcode150
chrome/browser/dom_ui/conflicts_ui.cc:150:
EnumerateModulesModel::GetSingleton()->ScanNow();
nice!

http://codereview.chromium.org/4524002/diff/60001/20010#newcode175
chrome/browser/dom_ui/conflicts_ui.cc:175: }
this will localize poorly. instead, use strings with placeholders, and use
GetStringUTF16F (you probably need distinct strings for the two branches).

http://codereview.chromium.org/4524002/diff/60001/20011
File chrome/browser/dom_ui/conflicts_ui.h (right):

http://codereview.chromium.org/4524002/diff/60001/20011#newcode15
chrome/browser/dom_ui/conflicts_ui.h:15: class ConflictsUI : public DOMUI {
Add a short class comment ("// The dom ui handler for about:conflicts")

http://codereview.chromium.org/4524002/diff/60001/20012
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20012#newcode12
chrome/browser/dom_ui/dom_ui_factory.cc:12: #include
"chrome/browser/dom_ui/conflicts_ui.h"
i'd put this in |#if windows|, but feel free to not do that if you disagree

http://codereview.chromium.org/4524002/diff/60001/20016
File chrome/browser/resources/about_conflicts.html (right):

http://codereview.chromium.org/4524002/diff/60001/20016#newcode148
chrome/browser/resources/about_conflicts.html:148: 'type_description': 'The type
of module (string), defaults to blank for regular modules',
80 col (also later in this file)

http://codereview.chromium.org/4524002/diff/60001/20016#newcode178
chrome/browser/resources/about_conflicts.html:178: * and return detailed data
about the configuration. The FlagsDOMHandler
ConflictsDOMHandler

http://codereview.chromium.org/4524002/diff/60001/20020
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/4524002/diff/60001/20020#newcode1289
chrome/chrome_browser.gypi:1289: 'browser/dom_ui/conflicts_ui.h',
nit: i would've added that into the windows-only files block

[Finnur, 2010-11-05 22:39:43]

Thanks for the quick comments, Nico. 

I have addressed almost all and have a couple of questions. Would love to close
the loop before EOD your time, since I'll be 8hrs ahead come timeshift (Monday
comments mean Tuesday response for me).

http://codereview.chromium.org/4524002/diff/60001/20003
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4524002/diff/60001/20003#newcode4171
chrome/app/generated_resources.grd:4171: A module with the same name has been
known to conflict with <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.
Without details it is hard for me to justify doing that. It seems... hacky.

On 2010/11/05 21:31:48, Nico wrote:
> i recently learned that putting in the product name at runtime doesn't
localize
> correctly in korean; it's better to put this into chrome_strings and hardcode
> "Google Chrome"

http://codereview.chromium.org/4524002/diff/60001/20003#newcode4183
chrome/app/generated_resources.grd:4183: Uninstalling it
Without the 'it' word, it looks like we are suggesting uninstalling Chrome,
which is not the intent. But, I'll remove it and see what people say.

On 2010/11/05 21:31:48, Nico wrote:
> can you remove "it" from this and the next 2 strings?

http://codereview.chromium.org/4524002/diff/60001/20008
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20008#newcode105
chrome/browser/browser_about_handler.cc:105: const char kConflictsPath[] =
"conflicts";
Purist! :)

OK, done.

On 2010/11/05 21:31:48, Nico wrote:
> #if defined(OS_WIN)

http://codereview.chromium.org/4524002/diff/60001/20010
File chrome/browser/dom_ui/conflicts_ui.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20010#newcode28
chrome/browser/dom_ui/conflicts_ui.cc:28: #include "grit/theme_resources.h"
Good catch. 

I usually remember to go through at least every new file I add and remove all
includes and see if it compiles. I did so with this file, but then made changes
to it that removed redundant prefs functions. I did another pass just now and it
only removed the prefs stuff.

On 2010/11/05 21:31:48, Nico wrote:
> please remove unnecessary includes. all the pref stuff seems to be unused;
> there's probably more

http://codereview.chromium.org/4524002/diff/60001/20010#newcode54
chrome/browser/dom_ui/conflicts_ui.cc:54: ~ConflictsUIHTMLSource() {}
On 2010/11/05 21:31:48, Nico wrote:
> remove?

Done.

http://codereview.chromium.org/4524002/diff/60001/20010#newcode67
chrome/browser/dom_ui/conflicts_ui.cc:67:
localized_strings.SetString("modulesBlurb",
Another good catch. This used to be called about:compatibility, but it was
confusing because of web-compat. Guess I forgot to rename this. The strings are
prefixed with modules because we list all modules and not just conflicts.

On 2010/11/05 21:31:48, Nico wrote:
> why are the strings prefixed with "modules" but the constants with
> IDS_COMPATIBILITY? can this be made consistent? …and the url is
about:conflicts!
> It should probably all be prefixed with conflicts / IDS_CONFLICTS then?

http://codereview.chromium.org/4524002/diff/60001/20010#newcode120
chrome/browser/dom_ui/conflicts_ui.cc:120: ConflictsDOMHandler() {}
On 2010/11/05 21:31:48, Nico wrote:
> remove?

I haven't looked to closely, but this default constructor seems to be needed for
line 204 below.

http://codereview.chromium.org/4524002/diff/60001/20010#newcode175
chrome/browser/dom_ui/conflicts_ui.cc:175: }
I don't follow. I have two branches and two strings right now. They are:

Modules ($1) - No conflicts detected
Modules ($1) - Known conflicts: $2, suspected: $3

... where $1 is the total count and $2 and $3 are ints as well.

How does this not localize well?


On 2010/11/05 21:31:48, Nico wrote:
> this will localize poorly. instead, use strings with placeholders, and use
> GetStringUTF16F (you probably need distinct strings for the two branches).

http://codereview.chromium.org/4524002/diff/60001/20011
File chrome/browser/dom_ui/conflicts_ui.h (right):

http://codereview.chromium.org/4524002/diff/60001/20011#newcode15
chrome/browser/dom_ui/conflicts_ui.h:15: class ConflictsUI : public DOMUI {
Good point. Thanks for the suggestion.

http://codereview.chromium.org/4524002/diff/60001/20012
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20012#newcode12
chrome/browser/dom_ui/dom_ui_factory.cc:12: #include
"chrome/browser/dom_ui/conflicts_ui.h"
No, I don't disagree. Done.

On 2010/11/05 21:31:48, Nico wrote:
> i'd put this in |#if windows|, but feel free to not do that if you disagree

http://codereview.chromium.org/4524002/diff/60001/20016
File chrome/browser/resources/about_conflicts.html (right):

http://codereview.chromium.org/4524002/diff/60001/20016#newcode148
chrome/browser/resources/about_conflicts.html:148: 'type_description': 'The type
of module (string), defaults to blank for regular modules',
On 2010/11/05 21:31:48, Nico wrote:
> 80 col (also later in this file)

Done.

http://codereview.chromium.org/4524002/diff/60001/20016#newcode178
chrome/browser/resources/about_conflicts.html:178: * and return detailed data
about the configuration. The FlagsDOMHandler
On 2010/11/05 21:31:48, Nico wrote:
> ConflictsDOMHandler

Done.

[tfarina, 2010-11-05 23:04:23]

On 2010/11/05 20:52:15, Finnur wrote:
> Yes, that does compile. I'm still not sure we care about one way or the other,
> though?

I think we should stay with (the sorter):
enum Foo { ... };

Instead of:
typedef enum {
...
} Foo;

No?

I don't see any advantages of using the typedef version, it just hides more
information. I think it has advantages when used in the c land (e.g, the GTK
APIs).

[Finnur, 2010-11-05 23:11:34]

Sure. If you look at the latest patch you'll see I've already changed it.

On 2010/11/05 23:04:23, tfarina wrote:
> On 2010/11/05 20:52:15, Finnur wrote:
> > Yes, that does compile. I'm still not sure we care about one way or the
other,
> > though?
> 
> I think we should stay with (the sorter):
> enum Foo { ... };
> 
> Instead of:
> typedef enum {
> ...
> } Foo;
> 
> No?
> 
> I don't see any advantages of using the typedef version, it just hides more
> information. I think it has advantages when used in the c land (e.g, the GTK
> APIs).

[tfarina, 2010-11-06 00:07:17]

http://codereview.chromium.org/4524002/diff/76001/77009
File chrome/browser/dom_ui/conflicts_ui.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77009#newcode126
chrome/browser/dom_ui/conflicts_ui.cc:126: NotificationRegistrar registrar_;
nit: this should be below the Observer function.

http://codereview.chromium.org/4524002/diff/76001/77009#newcode150
chrome/browser/dom_ui/conflicts_ui.cc:150: DictionaryValue results;
nit: move this below |list|, so it's closer to where it's used.

http://codereview.chromium.org/4524002/diff/76001/77012
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77012#newcode76
chrome/browser/enumerate_modules_model_unittest_win.cc:76: const
EnumerateModules::Module kStandardModule =
nit: static const ?

http://codereview.chromium.org/4524002/diff/76001/77012#newcode81
chrome/browser/enumerate_modules_model_unittest_win.cc:81: static const char*
kMatchName = "88e8c9e0";             // "bar.dll".
nit: would be better if it was static const char kFoo[]. (this is optional :))

http://codereview.chromium.org/4524002/diff/76001/77012#newcode93
chrome/browser/enumerate_modules_model_unittest_win.cc:93: const struct
MatchingEntryList {
nit: static const?

http://codereview.chromium.org/4524002/diff/76001/77013
File chrome/browser/enumerate_modules_model_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77013#newcode35
chrome/browser/enumerate_modules_model_win.cc:35: static bool module_sort(const
EnumerateModules::Module& a,
nit: name this function using CamelCase (ModuleSort, or maybe ModuleCompare,
ModuleEqual) ?

http://codereview.chromium.org/4524002/diff/76001/77013#newcode47
chrome/browser/enumerate_modules_model_win.cc:47: Lock* lock = NULL;
I think this is not allowed. Why the global lock? Can't be moved into the class?

http://codereview.chromium.org/4524002/diff/76001/77013#newcode59
chrome/browser/enumerate_modules_model_win.cc:59: const
EnumerateModules::BlacklistEntry EnumerateModules::kModuleBlacklist[] = {
nit: static?

http://codereview.chromium.org/4524002/diff/76001/77013#newcode103
chrome/browser/enumerate_modules_model_win.cc:103: void GenerateHash(const
std::string& input, std::string* output) {
nit: static?

http://codereview.chromium.org/4524002/diff/76001/77015
File chrome/browser/resources/about_conflicts.html (right):

http://codereview.chromium.org/4524002/diff/76001/77015#newcode173
chrome/browser/resources/about_conflicts.html:173: var output =
document.getElementById('modulesTemplate');
nit: can't you use $('modulesTemplate') here? I think this function ($) is
available in one of the shared js files.

http://codereview.chromium.org/4524002/diff/76001/77015#newcode203
chrome/browser/resources/about_conflicts.html:203: 
nit: It seems this and the below spaces are for readability. But they can be
removed though.

http://codereview.chromium.org/4524002/diff/76001/77015#newcode219
chrome/browser/resources/about_conflicts.html:219: >TITLE</span>
nit: move the > into the previous line? Or indent jscontent in the next line?

http://codereview.chromium.org/4524002/diff/76001/77015#newcode231
chrome/browser/resources/about_conflicts.html:231:
>NO_MODULES_ARE_AVAILABLE</div>
nit: same thing here, this > should be on the previous line?

[Nico, 2010-11-06 00:22:40]

LG for the files I looked at.

http://codereview.chromium.org/4524002/diff/60001/20003
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4524002/diff/60001/20003#newcode4171
chrome/app/generated_resources.grd:4171: A module with the same name has been
known to conflict with <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.
On 2010/11/05 22:39:43, Finnur wrote:
> Without details it is hard for me to justify doing that. It seems... hacky.

I believe the issue was that in some languages (maybe Korean and two others?),
the declination of a noun depends on something else in the sentence. For 
example, there might be a rule like "if a sentence contains 'in', then the
object needs to end in '-y' if it normally ends in e", so depending on the
sentence the translator would need to say "Google Chromy". If the fixed,
non-declined product name is substituted programmatically, this is not possible.

http://codereview.chromium.org/4524002/diff/60001/20010
File chrome/browser/dom_ui/conflicts_ui.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20010#newcode120
chrome/browser/dom_ui/conflicts_ui.cc:120: ConflictsDOMHandler() {}
On 2010/11/05 22:39:43, Finnur wrote:
> On 2010/11/05 21:31:48, Nico wrote:
> > remove?
> 
> I haven't looked to closely, but this default constructor seems to be needed
for
> line 204 below.

Huh, I always thought c++ creates an implicit default constructor if you don't
define one explicitly. Try writing line 204 like:

AddMessageHandler((new ConflictsDOMHandler)->Attach(this));

(i.e. no parens – this isn't java after all ;-) )

http://codereview.chromium.org/4524002/diff/60001/20010#newcode175
chrome/browser/dom_ui/conflicts_ui.cc:175: }
On 2010/11/05 22:39:43, Finnur wrote:
> I don't follow. I have two branches and two strings right now. They are:
> 
> Modules ($1) - No conflicts detected
> Modules ($1) - Known conflicts: $2, suspected: $3
> 
> ... where $1 is the total count and $2 and $3 are ints as well.
> 
> How does this not localize well?

It's at least conceivable that there are languages where if you have a phrase
with a dash like "A - B", that the exact look of B depends on A. If A and B are
in separate strings and concatenated programmatically, localizers won't be able
to do handle this correctly.

I don't know if this is a real problem in this specific case, but not
concatenating strings programmatically is generally a good idea (at least that's
what some i18n class taught me).

http://codereview.chromium.org/4524002/diff/76001/77012
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77012#newcode93
chrome/browser/enumerate_modules_model_unittest_win.cc:93: const struct
MatchingEntryList {
On 2010/11/06 00:07:17, tfarina wrote:
> nit: static const?

const already has implicit linkage in c++, there's no need to make consts static
or to put them into an unnamed namespace.

http://codereview.chromium.org/4524002/diff/76001/77015
File chrome/browser/resources/about_conflicts.html (right):

http://codereview.chromium.org/4524002/diff/76001/77015#newcode79
chrome/browser/resources/about_conflicts.html:79: .section-header > table tr
td:first-child {
table > tr > td

('>' means direct child, leaving it out means 'any descendant'. applying rules
to 'any descendant' causes layout engines to do a lot of tree walking and is
slower.)

http://codereview.chromium.org/4524002/diff/76001/77015#newcode131
chrome/browser/resources/about_conflicts.html:131: color: Orange;
i believe we normally but colors all in lowercase. also "Red" below.

[tfarina, 2010-11-06 00:26:28]

http://codereview.chromium.org/4524002/diff/76001/77012
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77012#newcode93
chrome/browser/enumerate_modules_model_unittest_win.cc:93: const struct
MatchingEntryList {
On 2010/11/06 00:22:40, Nico wrote:
> On 2010/11/06 00:07:17, tfarina wrote:
> > nit: static const?
> 
> const already has implicit linkage in c++, there's no need to make consts
static
> or to put them into an unnamed namespace.

OK, but that doesn't match with what we do in all other places. Is better to
keep the consistency and use static const or wrap them into an anonymous
namespace.

[Nico, 2010-11-06 00:27:29]

On Fri, Nov 5, 2010 at 8:26 PM,  <tfarina@chromium.org> wrote:
>
> http://codereview.chromium.org/4524002/diff/76001/77012
> File chrome/browser/enumerate_modules_model_unittest_win.cc (right):
>
> http://codereview.chromium.org/4524002/diff/76001/77012#newcode93
> chrome/browser/enumerate_modules_model_unittest_win.cc:93: const struct
> MatchingEntryList {
> On 2010/11/06 00:22:40, Nico wrote:
>>
>> On 2010/11/06 00:07:17, tfarina wrote:
>> > nit: static const?
>
>> const already has implicit linkage in c++, there's no need to make
>
> consts static
>>
>> or to put them into an unnamed namespace.
>
> OK, but that doesn't match with what we do in all other places. Is
> better to keep the consistency and use static const or wrap them into an
> anonymous namespace.

No.

[huanr, 2010-11-06 03:37:02]

Awesome patch!

http://codereview.chromium.org/4524002/diff/60001/20015
File chrome/browser/enumerate_modules_model_win.h (right):

http://codereview.chromium.org/4524002/diff/60001/20015#newcode90
chrome/browser/enumerate_modules_model_win.h:90: static void
NormalizeModule(Module* module);
Is there a flag to tell whether a Module is normalized or not? If the function
is only used when comparing against the blacklist, you can put it in private
section.

http://codereview.chromium.org/4524002/diff/76001/77013
File chrome/browser/enumerate_modules_model_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77013#newcode40
chrome/browser/enumerate_modules_model_win.cc:40: return a.name < b.name;
why compare name here and location below?

http://codereview.chromium.org/4524002/diff/76001/77013#newcode48
chrome/browser/enumerate_modules_model_win.cc:48: 
Another option is putting it to anonymous name space.

http://codereview.chromium.org/4524002/diff/76001/77013#newcode58
chrome/browser/enumerate_modules_model_win.cc:58: // marked as confirmed bad
(only as suspicious).
Shall we put this list into a separate file so it can be independently updated
(possibly with tools)?

http://codereview.chromium.org/4524002/diff/76001/77014
File chrome/browser/enumerate_modules_model_win.h (right):

http://codereview.chromium.org/4524002/diff/76001/77014#newcode33
chrome/browser/enumerate_modules_model_win.h:33: WINSOCK_MODULE_REGISTRATION,
Is there a way to get shell extensions? or we only know once it is loaded?

http://codereview.chromium.org/4524002/diff/76001/77014#newcode41
chrome/browser/enumerate_modules_model_win.h:41: NOT_MATCHED,
Since the rest values are about "good" and "bad", at first I did not understand
the meaning of "not_matched". Seems to be  "not_on_black_list"?

http://codereview.chromium.org/4524002/diff/76001/77014#newcode78
chrome/browser/enumerate_modules_model_win.h:78: const char* version_from;
A description of fields will be helpful. Are we consider the case when there are
regressions and thus two range of version have conflicts?

[Paweł Hajdan Jr., 2010-11-06 11:58:57]

Just to clarify, my drive-by comment is non-blocking, i.e. you don't have to
wait for me.

I think it's better now, but it should be possible to simplify the code inside
SCOPED_TRACE a little bit.

http://codereview.chromium.org/4524002/diff/76001/77012
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77012#newcode62
chrome/browser/enumerate_modules_model_unittest_win.cc:62:
SCOPED_TRACE((ASCIIToUTF16("Test case no: ") +
nit: Why nit just string? I've seen examples with StringPrintf etc, so
ASCIIToUTF16 shouldn't be needed.

http://codereview.chromium.org/4524002/diff/76001/77012#newcode63
chrome/browser/enumerate_modules_model_unittest_win.cc:63:
base::IntToString16(i)).c_str());
nit: This c_str is also probably not needed.

[Finnur, 2010-11-06 12:46:52]

> Awesome patch!

Thank you. :)

I have updated the patch to address the comments (or replied where appropriate).

Please take another look.

http://codereview.chromium.org/4524002/diff/60001/20003
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4524002/diff/60001/20003#newcode4171
chrome/app/generated_resources.grd:4171: A module with the same name has been
known to conflict with <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph>.
Thanks.

Moved to {chromium | google_chrome}_strings.grd

On 2010/11/06 00:22:40, Nico wrote:
> On 2010/11/05 22:39:43, Finnur wrote:
> > Without details it is hard for me to justify doing that. It seems... hacky.
> 
> I believe the issue was that in some languages (maybe Korean and two others?),
> the declination of a noun depends on something else in the sentence. For 
> example, there might be a rule like "if a sentence contains 'in', then the
> object needs to end in '-y' if it normally ends in e", so depending on the
> sentence the translator would need to say "Google Chromy". If the fixed,
> non-declined product name is substituted programmatically, this is not
possible.

http://codereview.chromium.org/4524002/diff/60001/20010
File chrome/browser/dom_ui/conflicts_ui.cc (right):

http://codereview.chromium.org/4524002/diff/60001/20010#newcode120
chrome/browser/dom_ui/conflicts_ui.cc:120: ConflictsDOMHandler() {}
Same error:
1>.\browser\dom_ui\conflicts_ui.cc(199) : error C2512:
'`anonymous-namespace'::ConflictsDOMHandler' : no appropriate default
constructor available

On 2010/11/06 00:22:40, Nico wrote:
> On 2010/11/05 22:39:43, Finnur wrote:
> > On 2010/11/05 21:31:48, Nico wrote:
> > > remove?
> > 
> > I haven't looked to closely, but this default constructor seems to be needed
> for
> > line 204 below.
> 
> Huh, I always thought c++ creates an implicit default constructor if you don't
> define one explicitly. Try writing line 204 like:
> 
> AddMessageHandler((new ConflictsDOMHandler)->Attach(this));
> 
> (i.e. no parens – this isn't java after all ;-) )

http://codereview.chromium.org/4524002/diff/60001/20010#newcode175
chrome/browser/dom_ui/conflicts_ui.cc:175: }
Yeah, you are right. Done.

On 2010/11/06 00:22:40, Nico wrote:
> On 2010/11/05 22:39:43, Finnur wrote:
> > I don't follow. I have two branches and two strings right now. They are:
> > 
> > Modules ($1) - No conflicts detected
> > Modules ($1) - Known conflicts: $2, suspected: $3
> > 
> > ... where $1 is the total count and $2 and $3 are ints as well.
> > 
> > How does this not localize well?
> 
> It's at least conceivable that there are languages where if you have a phrase
> with a dash like "A - B", that the exact look of B depends on A. If A and B
are
> in separate strings and concatenated programmatically, localizers won't be
able
> to do handle this correctly.
> 
> I don't know if this is a real problem in this specific case, but not
> concatenating strings programmatically is generally a good idea (at least
that's
> what some i18n class taught me).

http://codereview.chromium.org/4524002/diff/76001/77009#newcode126
chrome/browser/dom_ui/conflicts_ui.cc:126: // Callback for the
"requestModuleList" message.
On 2010/11/06 00:07:17, tfarina wrote:
> nit: this should be below the Observer function.

Done.

http://codereview.chromium.org/4524002/diff/76001/77009#newcode150
chrome/browser/dom_ui/conflicts_ui.cc:150:
EnumerateModulesModel::GetSingleton()->ScanNow();
On 2010/11/06 00:07:17, tfarina wrote:
> nit: move this below |list|, so it's closer to where it's used.

Done.

http://codereview.chromium.org/4524002/diff/76001/77012
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77012#newcode63
chrome/browser/enumerate_modules_model_unittest_win.cc:63:
base::IntToString16(i)).c_str());
Good points. Thanks! Done.

On 2010/11/06 11:58:57, Paweł Hajdan Jr. wrote:
> nit: This c_str is also probably not needed.

http://codereview.chromium.org/4524002/diff/76001/77012#newcode81
chrome/browser/enumerate_modules_model_unittest_win.cc:81: static const char*
kMatchName = "88e8c9e0";             // "bar.dll".
Sure.

On 2010/11/06 00:07:17, tfarina wrote:
> nit: would be better if it was static const char kFoo[]. (this is optional :))

http://codereview.chromium.org/4524002/diff/76001/77012#newcode93
chrome/browser/enumerate_modules_model_unittest_win.cc:93: const struct
MatchingEntryList {
If it is not necessary, then it is not necessary. :)

On 2010/11/06 00:26:28, tfarina wrote:
> On 2010/11/06 00:22:40, Nico wrote:
> > On 2010/11/06 00:07:17, tfarina wrote:
> > > nit: static const?
> > 
> > const already has implicit linkage in c++, there's no need to make consts
> static
> > or to put them into an unnamed namespace.
> 
> OK, but that doesn't match with what we do in all other places. Is better to
> keep the consistency and use static const or wrap them into an anonymous
> namespace.
>

http://codereview.chromium.org/4524002/diff/76001/77013
File chrome/browser/enumerate_modules_model_win.cc (right):

http://codereview.chromium.org/4524002/diff/76001/77013#newcode35
chrome/browser/enumerate_modules_model_win.cc:35: static bool module_sort(const
EnumerateModules::Module& a,
On 2010/11/06 00:07:17, tfarina wrote:
> nit: name this function using CamelCase (ModuleSort, or maybe ModuleCompare,
> ModuleEqual) ?

Done.

http://codereview.chromium.org/4524002/diff/76001/77013#newcode40
chrome/browser/enumerate_modules_model_win.cc:40: return a.name < b.name;
Because:
C:\path\file1.dll  (location == c:\path\, name=file1.dll)
C:\path\file2.dll  (location == c:\path\, name=file2.dll)

Here, the locations are equal so we want to sort by filename.
On 2010/11/06 03:37:03, huanr wrote:
> why compare name here and location below?

http://codereview.chromium.org/4524002/diff/76001/77013#newcode47
chrome/browser/enumerate_modules_model_win.cc:47: Lock* lock = NULL;
This isn't a class level lock, this is a file level lock used across two classes
to protect the STL data structure from being accessed at the same time from
different threads.

On 2010/11/06 00:07:17, tfarina wrote:
> I think this is not allowed. Why the global lock? Can't be moved into the
class?

http://codereview.chromium.org/4524002/diff/76001/77013#newcode48
chrome/browser/enumerate_modules_model_win.cc:48: 
Umm... sure.

On 2010/11/06 03:37:03, huanr wrote:
> Another option is putting it to anonymous name space.

http://codereview.chromium.org/4524002/diff/76001/77013#newcode58
chrome/browser/enumerate_modules_model_win.cc:58: // marked as confirmed bad
(only as suspicious).
If we get to that point, I'll be happy to move it out. Currently, I'm not sure
we'll ever have such a tool since the process of identifying the DLLs and
constructing the info for the blacklist is not something that I see us
automating any time soon... If you still feel strongly about this, I can submit
a follow-up changelist. Let me know.


On 2010/11/06 03:37:03, huanr wrote:
> Shall we put this list into a separate file so it can be independently updated
> (possibly with tools)?

http://codereview.chromium.org/4524002/diff/76001/77013#newcode103
chrome/browser/enumerate_modules_model_win.cc:103: void GenerateHash(const
std::string& input, std::string* output) {
On 2010/11/06 00:07:17, tfarina wrote:
> nit: static?

Done.

http://codereview.chromium.org/4524002/diff/76001/77014
File chrome/browser/enumerate_modules_model_win.h (right):

http://codereview.chromium.org/4524002/diff/76001/77014#newcode33
chrome/browser/enumerate_modules_model_win.h:33: WINSOCK_MODULE_REGISTRATION,
I think there is a way, but I left it out of this changelist to try to limit the
scope of it.

On 2010/11/06 03:37:03, huanr wrote:
> Is there a way to get shell extensions? or we only know once it is loaded?

http://codereview.chromium.org/4524002/diff/76001/77014#newcode41
chrome/browser/enumerate_modules_model_win.h:41: NOT_MATCHED,
GOOD is more of a not_on_blacklist entry. This is closer to UNKNOWN, but
NOT_MATCHED fits better for the Match function and the unit tests. It is kind of
a 'The module was not matched against the blacklist, but we don't know if it is
good or bad'. I'll comment the struct.

On 2010/11/06 03:37:03, huanr wrote:
> Since the rest values are about "good" and "bad", at first I did not
understand
> the meaning of "not_matched". Seems to be  "not_on_black_list"?

http://codereview.chromium.org/4524002/diff/76001/77014#newcode78
chrome/browser/enumerate_modules_model_win.h:78: const char* version_from;
Commented. 

> Are we consider the case when there are
> regressions and thus two range of version have conflicts? 

Yes, these will be two entries in the blacklist.

On 2010/11/06 03:37:03, huanr wrote:
> A description of fields will be helpful. Are we consider the case when there
are
> regressions and thus two range of version have conflicts?

http://codereview.chromium.org/4524002/diff/76001/77015
File chrome/browser/resources/about_conflicts.html (right):

http://codereview.chromium.org/4524002/diff/76001/77015#newcode79
chrome/browser/resources/about_conflicts.html:79: .section-header > table tr
td:first-child {
This is taken straight from your flags.html. :)
But sure. Done.

On 2010/11/06 00:22:40, Nico wrote:
> table > tr > td
> 
> ('>' means direct child, leaving it out means 'any descendant'. applying rules
> to 'any descendant' causes layout engines to do a lot of tree walking and is
> slower.)

http://codereview.chromium.org/4524002/diff/76001/77015#newcode131
chrome/browser/resources/about_conflicts.html:131: color: Orange;
On 2010/11/06 00:22:40, Nico wrote:
> i believe we normally but colors all in lowercase. also "Red" below.

Done.

http://codereview.chromium.org/4524002/diff/76001/77015#newcode173
chrome/browser/resources/about_conflicts.html:173: var output =
document.getElementById('modulesTemplate');
Is that better somehow?

On 2010/11/06 00:07:17, tfarina wrote:
> nit: can't you use $('modulesTemplate') here? I think this function ($) is
> available in one of the shared js files.

http://codereview.chromium.org/4524002/diff/76001/77015#newcode203
chrome/browser/resources/about_conflicts.html:203: 
Can be, yes. But make it less readable as you point out.

On 2010/11/06 00:07:17, tfarina wrote:
> nit: It seems this and the below spaces are for readability. But they can be
> removed though.

http://codereview.chromium.org/4524002/diff/76001/77015#newcode219
chrome/browser/resources/about_conflicts.html:219: >TITLE</span>
On 2010/11/06 00:07:17, tfarina wrote:
> nit: move the > into the previous line? Or indent jscontent in the next line?

Done.

http://codereview.chromium.org/4524002/diff/76001/77015#newcode231
chrome/browser/resources/about_conflicts.html:231:
>NO_MODULES_ARE_AVAILABLE</div>
On 2010/11/06 00:07:17, tfarina wrote:
> nit: same thing here, this > should be on the previous line?

Done.

[Paweł Hajdan Jr., 2010-11-06 12:51:08]

Code I commented in the drive-by LGTM.

http://codereview.chromium.org/4524002/diff/15004/96014
File chrome/browser/enumerate_modules_model_unittest_win.cc (right):

http://codereview.chromium.org/4524002/diff/15004/96014#newcode178
chrome/browser/enumerate_modules_model_unittest_win.cc:178: ": '" +
UTF16ToASCII(test.name) + "'");
nit: Align with the previous line (one space to the left).

[tfarina, 2010-11-06 13:48:27]

My nits LGTM with the following addressed.

http://codereview.chromium.org/4524002/diff/15004/96015
File chrome/browser/enumerate_modules_model_win.cc (right):

http://codereview.chromium.org/4524002/diff/15004/96015#newcode120
chrome/browser/enumerate_modules_model_win.cc:120:
EnumerateModules::EnumerateModules(EnumerateModulesModel* observer)
nit: This and the destructor should be moved below the NormalizeModule function.
The order should math with the declaration in the header file.

http://codereview.chromium.org/4524002/diff/15004/96015#newcode462
chrome/browser/enumerate_modules_model_win.cc:462:
CERT_NAME_SIMPLE_DISPLAY_TYPE,
nit: would be better to align with |cert_context|, but I guess it will not fit
into 80 characters.

http://codereview.chromium.org/4524002/diff/15004/96015#newcode493
chrome/browser/enumerate_modules_model_win.cc:493: void
EnumerateModulesModel::ScanNow() {
nit: This should be above of the constructor.

http://codereview.chromium.org/4524002/diff/15004/96015#newcode504
chrome/browser/enumerate_modules_model_win.cc:504: module_enumerator_ = new
EnumerateModules(this);
nit: would be better to rename the class to ModuleEnumerator? EnumerateModules,
looks like a function name. (no need to address, just an idea).

http://codereview.chromium.org/4524002/diff/15004/96015#newcode597
chrome/browser/enumerate_modules_model_win.cc:597:
NotificationType::MODULE_LIST_ENUMERATED,
nit: indent 4 spaces.

http://codereview.chromium.org/4524002/diff/15004/96015#newcode604
chrome/browser/enumerate_modules_model_win.cc:604:
NotificationType::MODULE_INCOMPATIBILITY_DETECTED,
nit: indent 4 spaces.

http://codereview.chromium.org/4524002/diff/15004/96016
File chrome/browser/enumerate_modules_model_win.h (right):

http://codereview.chromium.org/4524002/diff/15004/96016#newcode21
chrome/browser/enumerate_modules_model_win.h:21: class FilePath;
nit: sort alphabetical

http://codereview.chromium.org/4524002/diff/15004/96016#newcode126
chrome/browser/enumerate_modules_model_win.h:126: typedef std::vector<
std::pair<string16, string16> > PathMapping;
nit: Move this and |path_mapping_| below GetSubjectNameFromDigitalSignature.

http://codereview.chromium.org/4524002/diff/15004/96017
File chrome/browser/resources/about_conflicts.html (right):

http://codereview.chromium.org/4524002/diff/15004/96017#newcode248
chrome/browser/resources/about_conflicts.html:248: >MODULE_TYPE</span>: </span>
nit: same thing here, could you go through the file and fix all the occurrences
below?

[huanr, 2010-11-06 16:35:46]

LGTM

[Finnur, 2010-11-08 10:06:36]

I have uploaded a new patch. All nits adressed. Will submit once the try bots
approve. Thanks to all for reviewing.

http://codereview.chromium.org/4524002/diff/15004/96015
File chrome/browser/enumerate_modules_model_win.cc (right):

http://codereview.chromium.org/4524002/diff/15004/96015#newcode462
chrome/browser/enumerate_modules_model_win.cc:462:
CERT_NAME_SIMPLE_DISPLAY_TYPE,
No, it does fit. 

On 2010/11/06 13:48:27, tfarina wrote:
> nit: would be better to align with |cert_context|, but I guess it will not fit
> into 80 characters.

http://codereview.chromium.org/4524002/diff/15004/96015#newcode504
chrome/browser/enumerate_modules_model_win.cc:504: module_enumerator_ = new
EnumerateModules(this);
No, I think that's a good idea. I was never happy having both classes having the
word EnumerateModules in the name. This makes it more distinct.

On 2010/11/06 13:48:27, tfarina wrote:
> nit: would be better to rename the class to ModuleEnumerator?
EnumerateModules,
> looks like a function name. (no need to address, just an idea).

[tfarina, 2010-11-08 14:57:47]

http://codereview.chromium.org/4524002/diff/110001/111015
File chrome/browser/enumerate_modules_model_win.cc (right):

http://codereview.chromium.org/4524002/diff/110001/111015#newcode214
chrome/browser/enumerate_modules_model_win.cc:214: : observer_(observer) {
nit: indent 4 spaces.

http://codereview.chromium.org/4524002/diff/110001/111015#newcode564
chrome/browser/enumerate_modules_model_win.cc:564: : scanning_(false),
nit: indent 4 spaces.

http://codereview.chromium.org/4524002/diff/110001/111015#newcode565
chrome/browser/enumerate_modules_model_win.cc:565:
confirmed_bad_modules_detected_(0),
mit: align with |scanning_|.

http://codereview.chromium.org/4524002/diff/110001/111015#newcode570
chrome/browser/enumerate_modules_model_win.cc:570:
base::TimeDelta::FromMilliseconds(kModuleCheckDelayMs),
nit: indent 4 spaces.

http://codereview.chromium.org/4524002/diff/110001/111016
File chrome/browser/enumerate_modules_model_win.h (right):

http://codereview.chromium.org/4524002/diff/110001/111016#newcode224
chrome/browser/enumerate_modules_model_win.h:224: virtual
~EnumerateModulesModel();
This destructor needs to be virtual?

[Finnur, 2010-11-09 12:08:18]

I already submitted, but these comments are good. Thanks for catching those.

I have made the changes locally and they will show up in my next changelist for
this project.

On 2010/11/08 14:57:47, tfarina wrote:
> http://codereview.chromium.org/4524002/diff/110001/111015
> File chrome/browser/enumerate_modules_model_win.cc (right):
> 
> http://codereview.chromium.org/4524002/diff/110001/111015#newcode214
> chrome/browser/enumerate_modules_model_win.cc:214: : observer_(observer) {
> nit: indent 4 spaces.
> 
> http://codereview.chromium.org/4524002/diff/110001/111015#newcode564
> chrome/browser/enumerate_modules_model_win.cc:564: : scanning_(false),
> nit: indent 4 spaces.
> 
> http://codereview.chromium.org/4524002/diff/110001/111015#newcode565
> chrome/browser/enumerate_modules_model_win.cc:565:
> confirmed_bad_modules_detected_(0),
> mit: align with |scanning_|.
> 
> http://codereview.chromium.org/4524002/diff/110001/111015#newcode570
> chrome/browser/enumerate_modules_model_win.cc:570:
> base::TimeDelta::FromMilliseconds(kModuleCheckDelayMs),
> nit: indent 4 spaces.
> 
> http://codereview.chromium.org/4524002/diff/110001/111016
> File chrome/browser/enumerate_modules_model_win.h (right):
> 
> http://codereview.chromium.org/4524002/diff/110001/111016#newcode224
> chrome/browser/enumerate_modules_model_win.h:224: virtual
> ~EnumerateModulesModel();
> This destructor needs to be virtual?