Issue 7101001: move EventHandler out of VideoCaptureController to make VS2005 happy

Issue 7101001: move EventHandler out of VideoCaptureController to make VS2005 happy (Closed)

Created:
9 years, 6 months ago by wjia(left Chromium)

Modified:
9 years, 6 months ago

Reviewers:
jam, scherkus (not reviewing)

CC:
chromium-reviews, joi+watch-content_chromium.org, Paweł Hajdan Jr.

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

move EventHandler out of VideoCaptureController to make VS2005 happy BUG=none TEST=try bots Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87417

Patch Set 2 : address code review #

Total comments: 8

Patch Set 3 : add new header file in gyp #

Patch Set 4 : change VideoCaptureControllerID to struct #

Patch Set 5 : remove one line of test code #

Patch Set 6 : use default constructor #

Created: 9 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+114 lines, -68 lines)			Patch
M	content/browser/renderer_host/video_capture_controller.h	View	1 2 3	3 chunks	+12 lines, -35 lines	0 comments	Download
M	content/browser/renderer_host/video_capture_controller.cc	View	1 2 3	1 chunk	+2 lines, -2 lines	0 comments	Download
A	content/browser/renderer_host/video_capture_controller_event_handler.h	View	1 2 3 4 5	1 chunk	+51 lines, -0 lines	0 comments	Download
A	content/browser/renderer_host/video_capture_controller_event_handler.cc	View	1 2 3 4 5	1 chunk	+20 lines, -0 lines	0 comments	Download
M	content/browser/renderer_host/video_capture_host.h	View	1 2 3	4 chunks	+9 lines, -9 lines	0 comments	Download
M	content/browser/renderer_host/video_capture_host.cc	View	1 2 3 4 5	6 chunks	+17 lines, -21 lines	0 comments	Download
M	content/browser/renderer_host/video_capture_host_unittest.cc	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	content/content_browser.gypi	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

wjia(left Chromium)

EventHandler and ControllerId have been moved out of VideoCaptureController. This is the step to make ...

9 years, 6 months ago (2011-05-31 18:51:20 UTC) #1

jam

http://codereview.chromium.org/7101001/diff/1/content/browser/renderer_host/video_capture_controller.h File content/browser/renderer_host/video_capture_controller.h (right): http://codereview.chromium.org/7101001/diff/1/content/browser/renderer_host/video_capture_controller.h#newcode32 content/browser/renderer_host/video_capture_controller.h:32: class VideoCaptureControllerEventHandler { nit: this should be in its ...

9 years, 6 months ago (2011-05-31 19:00:07 UTC) #2

scherkus (not reviewing)

http://codereview.chromium.org/7101001/diff/1/content/browser/renderer_host/video_capture_controller.h File content/browser/renderer_host/video_capture_controller.h (right): http://codereview.chromium.org/7101001/diff/1/content/browser/renderer_host/video_capture_controller.h#newcode30 content/browser/renderer_host/video_capture_controller.h:30: typedef std::pair<int32, int> VideoCaptureControllerId; nit: while you're changing this ...

9 years, 6 months ago (2011-05-31 19:37:23 UTC) #3

wjia(left Chromium)

please take another look. The file name is so long that some lines exceed 80 ...

9 years, 6 months ago (2011-05-31 20:38:00 UTC) #4

jam

lgtm with gyp change http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_host/video_capture_controller_event_handler.h File content/browser/renderer_host/video_capture_controller_event_handler.h (right): http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_host/video_capture_controller_event_handler.h#newcode1 content/browser/renderer_host/video_capture_controller_event_handler.h:1: // Copyright (c) 2011 The ...

9 years, 6 months ago (2011-05-31 20:40:22 UTC) #5

scherkus (not reviewing)

nits but LGTM as well http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_host/video_capture_controller_event_handler.h File content/browser/renderer_host/video_capture_controller_event_handler.h (right): http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_host/video_capture_controller_event_handler.h#newcode13 content/browser/renderer_host/video_capture_controller_event_handler.h:13: // Id used for ...

9 years, 6 months ago (2011-05-31 20:42:00 UTC) #6

wjia(left Chromium)

9 years, 6 months ago (2011-06-01 03:30:52 UTC) #7

http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/video_capture_controller_event_handler.h
(right):

http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/video_capture_controller_event_handler.h:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2011/05/31 20:40:22, John Abd-El-Malek wrote:
> you need to add this new file to content_browser.gypi.

Done.

http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/video_capture_controller_event_handler.h:13: // Id
used for identifying an object of VideoCaptureController.
On 2011/05/31 20:42:00, scherkus wrote:
> Id -> ID

Done.

http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/video_capture_controller_event_handler.h:14:
typedef std::pair<int32, int> VideoCaptureControllerID;
On 2011/05/31 20:42:00, scherkus wrote:
> did you see darin's comment about using a struct instead of std::pair?

Done.

http://codereview.chromium.org/7101001/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/video_capture_controller_event_handler.h:17: //
VideoCaptureController to use VideoCaptureHost services.
On 2011/05/31 20:42:00, scherkus wrote:
> pedantic nit: VideoCaptureController doesn't know that VideoCaptureHost exists
> hence this interface
> 
> Otherwise you'd be passing in VideoCaptureHost pointer directly and do away w/
> this class :)

Done.

Expand Messages | Collapse Messages