Issue 743463002: Browser watcher, part deux.

Issue 743463002: Browser watcher, part deux. (Closed)

Created:
6 years, 1 month ago by Sigurður Ásgeirsson

Modified:
6 years, 1 month ago

Reviewers:
erikwright (departed)

CC:
chromium-reviews

Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr

Project:
chromium

Visibility:
Public.

More Reviews

Description

Browser watcher, part deux. Implementation of the watcher client class and its tests. See https://codereview.chromium.org/717223002/ for full change. BUG=412384 Committed: https://crrev.com/420541c797295004d91fb44099a36f4d66548e87 Cr-Commit-Position: refs/heads/master@{#304693}

Patch Set 2 : Address Erik's comments. #

Total comments: 4

Patch Set 3 : Address Erik's remaining comments. #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+369 lines, -400 lines)			Patch
M	components/browser_watcher.gypi	View	1	1 chunk	+17 lines, -2 lines	0 comments	Download
A +	components/browser_watcher/exit_code_watcher_win.h	View	1	2 chunks	+3 lines, -3 lines	0 comments	Download
A +	components/browser_watcher/exit_code_watcher_win.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
A +	components/browser_watcher/exit_code_watcher_win_unittest.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
A	components/browser_watcher/watcher_client_win.h	View		1 chunk	+59 lines, -0 lines	0 comments	Download
A	components/browser_watcher/watcher_client_win.cc	View	1 2	1 chunk	+75 lines, -0 lines	0 comments	Download
A	components/browser_watcher/watcher_client_win_unittest.cc	View	1	1 chunk	+210 lines, -0 lines	0 comments	Download
D	components/browser_watcher/watcher_win.h	View	1	1 chunk	+0 lines, -62 lines	0 comments	Download
D	components/browser_watcher/watcher_win.cc	View	1	1 chunk	+0 lines, -103 lines	0 comments	Download
D	components/browser_watcher/watcher_win_unittest.cc	View	1	1 chunk	+0 lines, -227 lines	0 comments	Download
M	components/components_tests.gyp	View	1 2	2 chunks	+3 lines, -1 line	0 comments	Download

Messages

Total messages: 10 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

erikwright (departed)

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher.gypi File components/browser_watcher.gypi (right): https://codereview.chromium.org/743463002/diff/1/components/browser_watcher.gypi#newcode10 components/browser_watcher.gypi:10: # This is a separate lib to minimize the ...

6 years, 1 month ago (2014-11-18 20:00:06 UTC) #3

Sigurður Ásgeirsson

Thanks - please take another look? https://codereview.chromium.org/743463002/diff/1/components/browser_watcher.gypi File components/browser_watcher.gypi (right): https://codereview.chromium.org/743463002/diff/1/components/browser_watcher.gypi#newcode10 components/browser_watcher.gypi:10: # This is ...

6 years, 1 month ago (2014-11-18 21:18:57 UTC) #4

Thanks - please take another look?

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher.gypi
File components/browser_watcher.gypi (right):

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher.g...
components/browser_watcher.gypi:10: # This is a separate lib to minimize the
dependencies for its
On 2014/11/18 20:00:05, erikwright wrote:
> personally I find the architecture self-explanatory.

I've seen plenty of build configuration regressions resulting in shipping
bloated binaries. A comment is inexpensive insurance against maintenance
regressions.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_client_win.cc (right):

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win.cc:11: #include
"base/win/scoped_handle.h"
On 2014/11/18 20:00:05, erikwright wrote:
> dup from header

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win.cc:28: DCHECK(cmd_line);
On 2014/11/18 20:00:05, erikwright wrote:
> base/logging.h
> 
> Though it's probably overkill for a single helper function in an anonymous
> namespace in such a small file.

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win.cc:30:
cmd_line->AppendSwitchASCII(ExitCodeWatcher::kParenthHandleSwitch,
On 2014/11/18 20:00:05, erikwright wrote:
> Wow. My bad. In your other CL, I should have pointed out that the file name
> should correspond to the class name.
> 
> Please rename watcher_win.{cc,h} to exit_code_watcher_win.{cc,h}
> 
> Can be a separate CL or this one as you see fit.

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win.cc:58:
base::LaunchProcess(cmd_line, options, &process);
On 2014/11/18 20:00:05, erikwright wrote:
> Would some logging be appropriate if this fails?

Done, though in practice I don't think you'll get any logging from Chrome.exe?

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win.cc:63:
DCHECK(!process_.IsValid());
On 2014/11/18 20:00:05, erikwright wrote:
> base/logging.h

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win.cc:72: process_ =
LaunchWatcherProcess(cmd_line, self.Get());
On 2014/11/18 20:00:05, erikwright wrote:
> I wonder if the destructor of WatcherClient should check whether process_ is
> still alive and report if it's not. Or maybe set a timer to check the
launcher's
> status after a few minutes of runtime. Or use an Event to signal successful
> initialization of the watcher and report if it fails to signal in some delay.
As
> it stands, a failure during launch would be unreported. Or a crash after
launch.

This is baked into chrome.exe, there's no infrastructure for what you request of
any kind.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
File components/browser_watcher/watcher_client_win_unittest.cc (right):

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win_unittest.cc:32: std::string
str_handle =
On 2014/11/18 20:00:06, erikwright wrote:
> include <string>

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win_unittest.cc:45:
base::ProcessHandle handle =
On 2014/11/18 20:00:06, erikwright wrote:
> base/process/process_handle.h

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win_unittest.cc:130:
EXPECT_FALSE("Impossible handle_policy");
On 2014/11/18 20:00:05, erikwright wrote:
> ADD_FAILURE() << "..."

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win_unittest.cc:158: // Inheritable
event handle used for testing.
On 2014/11/18 20:00:05, erikwright wrote:
> "event" ?

Done.

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win_unittest.cc:179:
TEST_F(BrowserWatcherClientTest, LaunchWatcherLegacyModeSucceeds) {
On 2014/11/18 20:00:06, erikwright wrote:
> This test uses a hack to verify that the XP-specific functionality works, even
> on non-XP platforms.
> 
> It might be useful to write a test that verifies that we correctly detect the
XP
> platform and activate the legacy mode.
> 
> So, basically, a test that exits early when run on non-XP, and otherwise runs
> without using the explicit legacy mode and asserts that things still work.

Good idea!

https://codereview.chromium.org/743463002/diff/1/components/browser_watcher/w...
components/browser_watcher/watcher_client_win_unittest.cc:184: // USe the legacy
launch mode.
On 2014/11/18 20:00:05, erikwright wrote:
> USe -> Use

Done.

erikwright (departed)

LGTM with nits. https://codereview.chromium.org/743463002/diff/20001/components/browser_watcher/watcher_client_win.cc File components/browser_watcher/watcher_client_win.cc (right): https://codereview.chromium.org/743463002/diff/20001/components/browser_watcher/watcher_client_win.cc#newcode56 components/browser_watcher/watcher_client_win.cc:56: if (!base::LaunchProcess(cmd_line, options, &process)) { nit: ...

6 years, 1 month ago (2014-11-18 21:25:32 UTC) #5

Sigurður Ásgeirsson

Thanks - committing. https://codereview.chromium.org/743463002/diff/20001/components/browser_watcher/watcher_client_win.cc File components/browser_watcher/watcher_client_win.cc (right): https://codereview.chromium.org/743463002/diff/20001/components/browser_watcher/watcher_client_win.cc#newcode56 components/browser_watcher/watcher_client_win.cc:56: if (!base::LaunchProcess(cmd_line, options, &process)) { On ...

6 years, 1 month ago (2014-11-18 21:33:20 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/743463002/40001

6 years, 1 month ago (2014-11-18 21:33:50 UTC) #8

commit-bot: I haz the power

6 years, 1 month ago (2014-11-18 23:17:28 UTC) #10

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/420541c797295004d91fb44099a36f4d66548e87
Cr-Commit-Position: refs/heads/master@{#304693}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages