Issue 20247003: Introduced base class for WebRTC browser tests, cleaned up duplication.

Issue 20247003: Introduced base class for WebRTC browser tests, cleaned up duplication. (Closed)

Created:
7 years, 5 months ago by phoglund_chromium

Modified:
7 years, 5 months ago

Reviewers:
tommi (sloooow) - chröme

CC:
chromium-reviews, feature-media-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Introduced base class for WebRTC browser tests, cleaned up duplication. I considered using delegation, but found that the test code read worse (e.g. test_helper_.GetUserMediaAndAccept(...) rather than just GetUserMediaAndAccept(...)). I could also have made the methods in the base class normal functions since they don't have any state, but we need to be able to friend-declare them, so a base class seemed to solve both problems. BUG=225698 R=tommi@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213917

Patch Set 1 #

Patch Set 2 : #

Total comments: 13

Patch Set 3 : Nit fixes #

Patch Set 4 : WebrtcTestBase #

Total comments: 1

Patch Set 5 : Rebased #

Patch Set 6 : #

Created: 7 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+217 lines, -207 lines)			Patch
M	chrome/browser/media/chrome_media_stream_infobar_browsertest.cc	View	1 2 3	10 chunks	+26 lines, -136 lines	0 comments	Download
M	chrome/browser/media/chrome_webrtc_browsertest.cc	View	1 2 3	6 chunks	+8 lines, -29 lines	0 comments	Download
M	chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc	View	1 2 3	6 chunks	+5 lines, -26 lines	0 comments	Download
M	chrome/browser/media/media_stream_infobar_delegate.h	View	1 2 3	1 chunk	+1 line, -3 lines	0 comments	Download
A	chrome/browser/media/webrtc_browsertest_base.h	View	1 2 3	1 chunk	+46 lines, -0 lines	0 comments	Download
A	chrome/browser/media/webrtc_browsertest_base.cc	View	1 2 3 4 5	1 chunk	+130 lines, -0 lines	0 comments	Download
M	chrome/browser/media/webrtc_browsertest_common.cc	View	1 2 3 4	1 chunk	+0 lines, -13 lines	0 comments	Download
M	chrome/chrome_tests.gypi	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

tommi (sloooow) - chröme

https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right): https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc#newcode41 chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:41: << "Since this test tests the UI we want ...

7 years, 5 months ago (2013-07-25 14:09:09 UTC) #2

phoglund_chromium

PTAL https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc File chrome/browser/media/chrome_media_stream_infobar_browsertest.cc (right): https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc#newcode41 chrome/browser/media/chrome_media_stream_infobar_browsertest.cc:41: << "Since this test tests the UI we ...

7 years, 5 months ago (2013-07-25 15:22:46 UTC) #3

tommi (sloooow) - chröme

I would prefer to be more consistent with the style guide since this is new ...

7 years, 5 months ago (2013-07-25 16:03:53 UTC) #4

I would prefer to be more consistent with the style guide since this is new
code rather than follow older code that needs to be fixed.
On Jul 25, 2013 5:22 PM, <phoglund@chromium.org> wrote:

> PTAL
>
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/chrome_media_**stream_infobar_browsertest.cc<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc>
> File chrome/browser/media/chrome_**media_stream_infobar_**browsertest.cc
> (right):
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/chrome_media_**stream_infobar_browsertest.cc#**newcode41<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc#newcode41>
> chrome/browser/media/chrome_**media_stream_infobar_**browsertest.cc:41: <<
> "Since this test tests the UI we want the real UI!";
> On 2013/07/25 14:09:09, tommi wrote:
>
>> fix indent (was previously ok)
>>
>
> Done.
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.cc<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.cc>
> File chrome/browser/media/webrtc_**browsertest_base.cc (right):
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.cc#newcode5<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.cc#newcode5>
> chrome/browser/media/webrtc_**browsertest_base.cc:5: #include
> "webrtc_browsertest_base.h"
> On 2013/07/25 14:09:09, tommi wrote:
>
>> full path
>>
>
> Done.
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.cc#newcode5<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.cc#newcode5>
> chrome/browser/media/webrtc_**browsertest_base.cc:5: #include
> "webrtc_browsertest_base.h"
> On 2013/07/25 14:09:09, tommi wrote:
>
>> full path
>>
>
> Done.
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.cc#newcode68<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.cc#newcode68>
> chrome/browser/media/webrtc_**browsertest_base.cc:68:
> kAudioVideoCallConstraints);
> On 2013/07/25 14:09:09, tommi wrote:
>
>> indent
>>
>
> Done.
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.cc#newcode87<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.cc#newcode87>
> chrome/browser/media/webrtc_**browsertest_base.cc:87:
> kAudioVideoCallConstraints);
> On 2013/07/25 14:09:09, tommi wrote:
>
>> indent
>>
>
> Done.
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.h<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.h>
> File chrome/browser/media/webrtc_**browsertest_base.h (right):
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.h#newcode16<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.h#newcode16>
> chrome/browser/media/webrtc_**browsertest_base.h:16: // Typical
> constraints.
> On 2013/07/25 14:09:09, tommi wrote:
>
>> can these be static members of WebrtcTestBase?
>>
>
> Done.
>
> https://codereview.chromium.**org/20247003/diff/2001/chrome/**
>
browser/media/webrtc_**browsertest_base.h#newcode27<https://codereview.chromium.org/20247003/diff/2001/chrome/browser/media/webrtc_browsertest_base.h#newcode27>
> chrome/browser/media/webrtc_**browsertest_base.h:27: class WebrtcTestBase
> : public InProcessBrowserTest {
> On 2013/07/25 14:09:09, tommi wrote:
>
>> WebRtcTestBase or WebRTCTestBase.  It seems we use both WebRTC and
>>
> WebRtc under
>
>> content/ so please just pick the one that's more common.
>>
> Hm. For the tests we are currently using Webrtc, like WebrtcBrowserTest.
> We could change that, but then we need to update the gtest filters on
> the bots for instance. I suggest doing this in a later CL and keep this
> one consistent with the tests.
>
>
https://codereview.chromium.**org/20247003/<https://codereview.chromium.org/2...
>

phoglund_chromium

On 2013/07/26 07:42:13, phoglund wrote: > Fair enough. PTAL And I checked, WebRtc seemed to ...

7 years, 5 months ago (2013-07-26 07:42:58 UTC) #6

tommi (sloooow) - chröme

lgtm https://codereview.chromium.org/20247003/diff/13001/chrome/browser/media/webrtc_browsertest_base.cc File chrome/browser/media/webrtc_browsertest_base.cc (right): https://codereview.chromium.org/20247003/diff/13001/chrome/browser/media/webrtc_browsertest_base.cc#newcode46 chrome/browser/media/webrtc_browsertest_base.cc:46: // Wait for the bar to pop up, ...

7 years, 5 months ago (2013-07-26 09:08:40 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phoglund@chromium.org/20247003/37001

7 years, 5 months ago (2013-07-26 09:46:00 UTC) #8

Message was sent while issue was closed.

Change committed as 213917

Expand Messages | Collapse Messages