Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(37)

Issue 2731953003: [DeviceService] Replace vibration_browsertest.cc with layout tests. (Closed)

Created:
2 years, 5 months ago by leonhsl(Using Gerrit)
Modified:
2 years, 5 months ago
CC:
chromium-reviews, mlamouri+watch-blink_chromium.org, mvanouwerkerk+watch_chromium.org, blink-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[DeviceService] Replace vibration_browsertest.cc with layout tests. vibration_browsertest.cc tests vibration integration between Blink and the browser. It does this by injecting a fake VibrationManager impl into RenderFrameHost's InterfaceRegistry. However, this blocks us to port VibrationManager to be hosted in Device Service because after that there is no such mechanism to do this. So this CL replaces these browsertests with layout tests, with a TODO to add 'main frame reload' tests later, because for now we consider that 'sub frame reload' tests have actually covered what we want to verify for lifecycle of NavigatorVibration. BUG=686692 TEST=blink_tests Review-Url: https://codereview.chromium.org/2731953003 Cr-Commit-Position: refs/heads/master@{#457710} Committed: https://chromium.googlesource.com/chromium/src/+/5033377133364937998758c5b77bf63e95824e73

Patch Set 1 #

Total comments: 1

Patch Set 2 #

Total comments: 13

Patch Set 3 : Add case: reload subframe #

Total comments: 2

Patch Set 4 : Add case: destroy subframe #

Patch Set 5 : Add TODO #

Total comments: 14

Patch Set 6 : Address comments from Tim #

Patch Set 7 : 'git cl format --js' for js/html #

Unified diffs Side-by-side diffs Delta from patch set Stats (+227 lines, -205 lines) Patch
D content/browser/vibration_browsertest.cc View 1 1 chunk +0 lines, -203 lines 0 comments Download
M content/test/BUILD.gn View 1 2 chunks +0 lines, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/vibration/resources/vibrate-from-iframe.html View 1 2 1 chunk +11 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/vibration/resources/vibration-helpers.js View 1 2 3 4 5 6 1 chunk +72 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/vibration/vibration.html View 1 2 3 4 5 1 chunk +53 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/vibration/vibration-iframe.html View 1 2 3 4 5 6 1 chunk +91 lines, -0 lines 0 comments Download

Messages

Total messages: 78 (49 generated)
leonhsl(Using Gerrit)
Hi, would you PTAL at ps#1 firstly? So that I can know whether I'm on ...
2 years, 5 months ago (2017-03-07 15:00:42 UTC) #6
blundell
Hi Han Leon, This looks like a good start! Could you complete the first layout ...
2 years, 5 months ago (2017-03-07 22:47:56 UTC) #7
leonhsl(Using Gerrit)
I'm also a newbie in writing layout tests.. And I suppose we do not need ...
2 years, 5 months ago (2017-03-08 09:14:34 UTC) #11
blundell
Hi Tim, Could you take the lead on reviewing this CL? Han Leon is looking ...
2 years, 5 months ago (2017-03-08 17:42:18 UTC) #14
blundell
On 2017/03/08 17:42:18, blundell wrote: > Hi Tim, > > Could you take the lead ...
2 years, 5 months ago (2017-03-08 17:43:26 UTC) #15
timvolodine
On 2017/03/08 17:43:26, blundell wrote: > On 2017/03/08 17:42:18, blundell wrote: > > Hi Tim, ...
2 years, 5 months ago (2017-03-08 17:49:45 UTC) #16
timvolodine
Structure looks fine to me. Thanks for adding these Han Leon. Some initial comments below. ...
2 years, 5 months ago (2017-03-08 19:38:32 UTC) #17
leonhsl(Using Gerrit)
Uploaded ps#3, PTAnL with my inline questions.. Thanks! https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibration_browsertest.cc File content/browser/vibration_browsertest.cc (left): https://codereview.chromium.org/2731953003/diff/20001/content/browser/vibration_browsertest.cc#oldcode140 content/browser/vibration_browsertest.cc:140: CancelVibrationFromMainFrameWhenMainFrameIsReloaded) ...
2 years, 5 months ago (2017-03-09 08:34:10 UTC) #20
blundell
https://codereview.chromium.org/2731953003/diff/40001/third_party/WebKit/LayoutTests/vibration/vibration-iframe.html File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/40001/third_party/WebKit/LayoutTests/vibration/vibration-iframe.html#newcode19 third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:19: iframe.src = 'resources/vibrate-from-iframe.html'; On 2017/03/09 08:34:10, leonhsl wrote: > ...
2 years, 5 months ago (2017-03-09 16:56:35 UTC) #23
leonhsl(Using Gerrit)
Uploaded ps#4, I've confirmed PASS locally and all trybots are green. Now the question is ...
2 years, 5 months ago (2017-03-10 09:29:45 UTC) #31
blundell
On 2017/03/10 09:29:45, leonhsl wrote: > Uploaded ps#4, I've confirmed PASS locally and all trybots ...
2 years, 5 months ago (2017-03-13 15:49:21 UTC) #32
timvolodine
On 2017/03/13 15:49:21, blundell wrote: > On 2017/03/10 09:29:45, leonhsl wrote: > > Uploaded ps#4, ...
2 years, 5 months ago (2017-03-13 17:52:24 UTC) #33
leonhsl(Using Gerrit)
On 2017/03/13 17:52:24, timvolodine wrote: > On 2017/03/13 15:49:21, blundell wrote: > > On 2017/03/10 ...
2 years, 5 months ago (2017-03-14 03:01:49 UTC) #34
blundell
On 2017/03/14 03:01:49, leonhsl wrote: > On 2017/03/13 17:52:24, timvolodine wrote: > > On 2017/03/13 ...
2 years, 5 months ago (2017-03-14 09:25:07 UTC) #35
leonhsl(Using Gerrit)
Uploaded ps#5 which adds a TODO and deletes useless comments, PTAnL, Thanks. And, I'm confused ...
2 years, 5 months ago (2017-03-14 10:03:46 UTC) #38
blundell
On 2017/03/14 10:03:46, leonhsl wrote: > Uploaded ps#5 which adds a TODO and deletes useless ...
2 years, 5 months ago (2017-03-14 12:16:12 UTC) #41
leonhsl(Using Gerrit)
On 2017/03/14 12:16:12, blundell wrote: > On 2017/03/14 10:03:46, leonhsl wrote: > > Uploaded ps#5 ...
2 years, 5 months ago (2017-03-14 14:50:16 UTC) #42
timvolodine
Thanks Han Leon! (BTW I have to admit I am slightly confused should I call ...
2 years, 5 months ago (2017-03-15 18:20:18 UTC) #43
leonhsl(Using Gerrit)
Thanks Tim a lot for kindly review! Uploaded ps#6, PTAnL. I'm Chinese so 'Han' is ...
2 years, 5 months ago (2017-03-16 06:32:42 UTC) #47
blundell
Naive question: Is there any meaningful distinction between testing behavior on subframe destruction and testing ...
2 years, 5 months ago (2017-03-16 10:46:20 UTC) #58
timvolodine
Thanks Leon, Maybe try formatting js files as mentioned below. Also would be good to ...
2 years, 5 months ago (2017-03-16 15:32:21 UTC) #59
timvolodine
On 2017/03/16 10:46:20, blundell wrote: > Naive question: Is there any meaningful distinction between testing ...
2 years, 5 months ago (2017-03-16 15:41:21 UTC) #60
Michael van Ouwerkerk
lgtm
2 years, 5 months ago (2017-03-16 15:52:52 UTC) #61
leonhsl(Using Gerrit)
Thanks all! Uploaded ps#7 to format js/html files. https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/LayoutTests/vibration/vibration-iframe.html File third_party/WebKit/LayoutTests/vibration/vibration-iframe.html (right): https://codereview.chromium.org/2731953003/diff/100001/third_party/WebKit/LayoutTests/vibration/vibration-iframe.html#newcode51 third_party/WebKit/LayoutTests/vibration/vibration-iframe.html:51: }, ...
2 years, 5 months ago (2017-03-17 03:55:15 UTC) #65
leonhsl(Using Gerrit)
+kinuko@, need your approval again for removal of content/browser/vibration_browsertest.cc, Thanks:)
2 years, 5 months ago (2017-03-17 03:56:41 UTC) #67
kinuko
lgtm
2 years, 5 months ago (2017-03-17 07:23:02 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731953003/160001
2 years, 5 months ago (2017-03-17 07:39:53 UTC) #74
commit-bot: I haz the power
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5033377133364937998758c5b77bf63e95824e73
2 years, 5 months ago (2017-03-17 07:45:15 UTC) #77
blundell
2 years, 5 months ago (2017-03-17 08:30:43 UTC) #78
Message was sent while issue was closed.
On 2017/03/17 07:45:15, commit-bot: I haz the power wrote:
> Committed patchset #7 (id:160001) as
>
https://chromium.googlesource.com/chromium/src/+/5033377133364937998758c5b77b...

This was really nice work, thanks!

Powered by Google App Engine
This is Rietveld 408576698