Chromium Code Reviews
Help | Chromium Project | Sign in
(783)

Issue 4524002: First cut of the about:conflicts page, listing all DLLs loaded in the Chrome ... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 5 months ago by Finnur
Modified:
2 years, 10 months ago
CC:
chromium-reviews_chromium.org, arv, ben+cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 5

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 2

Patch Set 5 : '' #

Total comments: 35

Patch Set 6 : '' #

Total comments: 45

Patch Set 7 : '' #

Total comments: 12

Patch Set 8 : '' #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+1868 lines, -32 lines) Lint Patch
M chrome/app/chrome_dll_resource.h View 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M chrome/app/chromium_strings.grd View 1 chunk +10 lines, -0 lines 0 comments 0 errors Download
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 4 chunks +71 lines, -0 lines 0 comments 0 errors Download
M chrome/app/google_chrome_strings.grd View 1 chunk +10 lines, -0 lines 0 comments 0 errors Download
M chrome/app/theme/theme_resources.grd View 2 chunks +7 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/about_flags.cc View 1 chunk +7 lines, -0 lines 0 comments 2 errors Download
M chrome/browser/browser.h View 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M chrome/browser/browser.cc View 4 chunks +10 lines, -3 lines 0 comments 0 errors Download
M chrome/browser/browser_about_handler.cc View 1 2 3 4 5 5 chunks +18 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/browser_resources.grd View 1 chunk +3 lines, -0 lines 0 comments 0 errors Download
A chrome/browser/dom_ui/conflicts_ui.h View 1 2 3 4 5 1 chunk +28 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/dom_ui/conflicts_ui.cc View 1 2 3 4 5 6 1 chunk +213 lines, -0 lines 0 comments 1 errors Download
M chrome/browser/dom_ui/dom_ui_factory.cc View 1 2 3 4 5 3 chunks +13 lines, -0 lines 0 comments 0 errors Download
A chrome/browser/enumerate_modules_model_unittest_win.cc View 4 5 6 7 1 chunk +182 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/enumerate_modules_model_win.h View 4 5 6 7 1 chunk +259 lines, -0 lines 1 comment 1 errors Download
A chrome/browser/enumerate_modules_model_win.cc View 4 5 6 7 1 chunk +626 lines, -0 lines 4 comments 2 errors Download
A chrome/browser/resources/about_conflicts.html View 1 2 3 4 5 6 7 1 chunk +292 lines, -0 lines 0 comments 0 errors Download
M chrome/browser/views/toolbar_view.h View 1 2 3 4 5 3 chunks +10 lines, -7 lines 0 comments 0 errors Download
M chrome/browser/views/toolbar_view.cc View 1 2 3 4 5 9 chunks +64 lines, -22 lines 0 comments 0 errors Download
M chrome/browser/wrench_menu_model.cc View 1 2 3 chunks +16 lines, -0 lines 0 comments 0 errors Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments 0 errors Download
M chrome/chrome_tests.gypi View 1 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M chrome/common/chrome_switches.h View 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M chrome/common/chrome_switches.cc View 1 chunk +4 lines, -0 lines 0 comments 0 errors Download
M chrome/common/notification_type.h View 1 chunk +11 lines, -0 lines 0 comments 0 errors Download
M chrome/common/url_constants.h View 3 chunks +3 lines, -0 lines 0 comments 0 errors Download
M chrome/common/url_constants.cc View 3 chunks +3 lines, -0 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 26
tfarina
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. ...
3 years, 5 months ago #1
Finnur
You are jumping the gun just a little bit. I'm just seeing if this compiles ...
3 years, 5 months ago #2
Paweł Hajdan Jr.
Drive-by with a test comment, no need to wait for another review by me. http://codereview.chromium.org/4524002/diff/5002/9013 ...
3 years, 5 months ago #3
Finnur
Huan, Nico, Ben, Pavel and Thiago (see below). Pavel and Thiago, please look at the ...
3 years, 5 months ago #4
Ben Goodger (Google)
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 ...
3 years, 5 months ago #5
tfarina
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: > ...
3 years, 5 months ago #6
Finnur
If I remove the typedef from ModuleType, when it tries to compile this: struct Module ...
3 years, 5 months ago #7
tfarina
On 2010/11/05 20:22:47, Finnur wrote: > If I remove the typedef from ModuleType, when it ...
3 years, 5 months ago #8
Finnur
Yes, that does compile. I'm still not sure we care about one way or the ...
3 years, 5 months ago #9
Nico
A bunch of nits, but looks good overall. The two comments I care about are ...
3 years, 5 months ago #10
Finnur
Thanks for the quick comments, Nico. I have addressed almost all and have a couple ...
3 years, 5 months ago #11
tfarina
On 2010/11/05 20:52:15, Finnur wrote: > Yes, that does compile. I'm still not sure we ...
3 years, 5 months ago #12
Finnur
Sure. If you look at the latest patch you'll see I've already changed it. On ...
3 years, 5 months ago #13
tfarina
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 ...
3 years, 5 months ago #14
Nico
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 ...
3 years, 5 months ago #15
tfarina
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: ...
3 years, 5 months ago #16
Nico
On Fri, Nov 5, 2010 at 8:26 PM, <tfarina@chromium.org> wrote: > > http://codereview.chromium.org/4524002/diff/76001/77012 > File ...
3 years, 5 months ago #17
huanr
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 ...
3 years, 5 months ago #18
Paweł Hajdan Jr.
Just to clarify, my drive-by comment is non-blocking, i.e. you don't have to wait for ...
3 years, 5 months ago #19
Finnur
> Awesome patch! Thank you. :) I have updated the patch to address the comments ...
3 years, 5 months ago #20
Paweł Hajdan Jr.
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: ": '" ...
3 years, 5 months ago #21
tfarina
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) ...
3 years, 5 months ago #22
huanr
LGTM
3 years, 5 months ago #23
Finnur
I have uploaded a new patch. All nits adressed. Will submit once the try bots ...
3 years, 5 months ago #24
tfarina
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: ...
3 years, 5 months ago #25
Finnur
3 years, 5 months ago #26
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6