Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886893010:
Add Video.testVideoSeeking test. (Closed)
|
Created:
3 years, 7 months ago by Tom Bergan Modified:
3 years, 7 months ago CC:
chromium-reviews, posciak+watch_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Video.testVideoSeeking test.
Not enabled on Android because it requires calling video.play().
BUG=717732
Review-Url: https://codereview.chromium.org/2886893010
Cr-Commit-Position: refs/heads/master@{#473744}
Committed: https://chromium.googlesource.com/chromium/src/+/f0417c18b1e1f6b239780ea9c8eb6637f245a780
Patch Set 1 #
Total comments: 13
Patch Set 2 : address comments #Patch Set 3 : split >80 char line #Patch Set 4 : split >80 char line #Patch Set 5 : fix comment #
Messages
Total messages: 19 (8 generated)
Description was changed from ========== Add Video.testVideoSeeking test. Not enabled on Android because it requires calling play(). BUG=717732 ========== to ========== Add Video.testVideoSeeking test. Not enabled on Android because it requires calling video.play(). BUG=717732 ==========
tombergan@google.com changed reviewers: + robertogden@chromium.org
lgtm. You'll need a committer to look at it too though sinc my lgtm isn't enough. Ryan or Tarun is probably best
tombergan@chromium.org changed reviewers: + ryansturm@chromium.org
https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:89: @NotAndroid t.FindElement(By.ID, 'video').click() might work on android if you can make a click work instead of .play() https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:94: t.LoadURL('http://check.googlezip.net/cacheable/video/buck_bunny_640x360_24fps.html') nit: can you split this over 2 lines (80 char limit) https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:134: response.request_headers['range'] != 'bytes=0-'): Are all the range requests of the form 0-? I thought you might want to look for others as well.
https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:89: @NotAndroid On 2017/05/22 21:32:01, RyanSturm wrote: > t.FindElement(By.ID, 'video').click() might work on android if you can make a > click work instead of .play() this test can't be android because ` with TestDriver(control_network_connection=True) as t:`
https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:89: @NotAndroid On 2017/05/22 21:34:53, Robert Ogden wrote: > On 2017/05/22 21:32:01, RyanSturm wrote: > > t.FindElement(By.ID, 'video').click() might work on android if you can make a > > click work instead of .play() > > this test can't be android because ` with > TestDriver(control_network_connection=True) as t:` I added a comment explaining why I added control_network_connection=True. Any thoughts about how we might get this to run on Android? I believe network emulation works on android when you open DevTools via device inspection. Any idea why control_network_connection doesn't work on Android? https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:94: t.LoadURL('http://check.googlezip.net/cacheable/video/buck_bunny_640x360_24fps.html') On 2017/05/22 21:32:02, RyanSturm wrote: > nit: can you split this over 2 lines (80 char limit) Done. https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:134: response.request_headers['range'] != 'bytes=0-'): On 2017/05/22 21:32:01, RyanSturm wrote: > Are all the range requests of the form 0-? I thought you might want to look for > others as well. s/num_range_requests/num_partial_requests/ When the media player plays a video, it first makes a request with "Range: bytes=0-" (not the trailing slash). This is a "full" request as it requests everything from byte 0 onwards. It's equivalent to an ordinary request that doesn't have a range header. To implement seeking, the player makes a partial request (like "Range: bytes=500-600"). The intent is to make sure the media player made at least one partial request, since the goal of the test is to ensure that we make an initial request followed by at least one partial request. Let me know if I should clarify this further.
https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:89: @NotAndroid On 2017/05/22 22:18:36, Tom Bergan wrote: > On 2017/05/22 21:34:53, Robert Ogden wrote: > > On 2017/05/22 21:32:01, RyanSturm wrote: > > > t.FindElement(By.ID, 'video').click() might work on android if you can make > a > > > click work instead of .play() > > > > this test can't be android because ` with > > TestDriver(control_network_connection=True) as t:` > > I added a comment explaining why I added control_network_connection=True. Any > thoughts about how we might get this to run on Android? > > I believe network emulation works on android when you open DevTools via device > inspection. Any idea why control_network_connection doesn't work on Android? From what I came across implementing the feature, network emulation only worked with device emulation which only works on desktop. But that was all external sites, maybe there's a way to do it I don't know about. Feel free to file a bug on me if you think there should be a way to do this and I'll look into it
lgtm % splitting the line over two lines not showing up? https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:89: @NotAndroid On 2017/05/22 23:00:27, Robert Ogden wrote: > On 2017/05/22 22:18:36, Tom Bergan wrote: > > On 2017/05/22 21:34:53, Robert Ogden wrote: > > > On 2017/05/22 21:32:01, RyanSturm wrote: > > > > t.FindElement(By.ID, 'video').click() might work on android if you can > make > > a > > > > click work instead of .play() > > > > > > this test can't be android because ` with > > > TestDriver(control_network_connection=True) as t:` > > > > I added a comment explaining why I added control_network_connection=True. Any > > thoughts about how we might get this to run on Android? > > > > I believe network emulation works on android when you open DevTools via device > > inspection. Any idea why control_network_connection doesn't work on Android? > > From what I came across implementing the feature, network emulation only worked > with device emulation which only works on desktop. But that was all external > sites, maybe there's a way to do it I don't know about. Feel free to file a bug > on me if you think there should be a way to do this and I'll look into it Thanks for the comment. Let's land as is. https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:94: t.LoadURL('http://check.googlezip.net/cacheable/video/buck_bunny_640x360_24fps.html') On 2017/05/22 22:18:36, Tom Bergan wrote: > On 2017/05/22 21:32:02, RyanSturm wrote: > > nit: can you split this over 2 lines (80 char limit) > > Done. Not seeing the change here. https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:134: response.request_headers['range'] != 'bytes=0-'): On 2017/05/22 22:18:36, Tom Bergan wrote: > On 2017/05/22 21:32:01, RyanSturm wrote: > > Are all the range requests of the form 0-? I thought you might want to look > for > > others as well. > > s/num_range_requests/num_partial_requests/ > > When the media player plays a video, it first makes a request with "Range: > bytes=0-" (not the trailing slash). This is a "full" request as it requests > everything from byte 0 onwards. It's equivalent to an ordinary request that > doesn't have a range header. > > To implement seeking, the player makes a partial request (like "Range: > bytes=500-600"). The intent is to make sure the media player made at least one > partial request, since the goal of the test is to ensure that we make an initial > request followed by at least one partial request. > > Let me know if I should clarify this further. Ah I see. I read the condition wrong. Thanks for renaming the variable.
https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:94: t.LoadURL('http://check.googlezip.net/cacheable/video/buck_bunny_640x360_24fps.html') On 2017/05/22 23:09:16, RyanSturm wrote: > On 2017/05/22 22:18:36, Tom Bergan wrote: > > On 2017/05/22 21:32:02, RyanSturm wrote: > > > nit: can you split this over 2 lines (80 char limit) > > > > Done. > > Not seeing the change here. Done for real.
https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/video.py (right): https://codereview.chromium.org/2886893010/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/video.py:94: t.LoadURL('http://check.googlezip.net/cacheable/video/buck_bunny_640x360_24fps.html') On 2017/05/22 23:15:14, Tom Bergan wrote: > On 2017/05/22 23:09:16, RyanSturm wrote: > > On 2017/05/22 22:18:36, Tom Bergan wrote: > > > On 2017/05/22 21:32:02, RyanSturm wrote: > > > > nit: can you split this over 2 lines (80 char limit) > > > > > > Done. > > > > Not seeing the change here. > > Done for real. Err, I fixed the wrong one. Now fixed for real.
The CQ bit was checked by tombergan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertogden@chromium.org, ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2886893010/#ps80001 (title: "fix comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495495184448990,
"parent_rev": "8d5f674e5ca5d6a9f0a2a9163bdba3615b2a83dd", "commit_rev":
"2c1060505b3bb790fd73a583e626a210c9d7ea6f"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495495184448990,
"parent_rev": "365428e3a1707aa11bebd13e170846c332347aa3", "commit_rev":
"f0417c18b1e1f6b239780ea9c8eb6637f245a780"}
Message was sent while issue was closed.
Description was changed from ========== Add Video.testVideoSeeking test. Not enabled on Android because it requires calling video.play(). BUG=717732 ========== to ========== Add Video.testVideoSeeking test. Not enabled on Android because it requires calling video.play(). BUG=717732 Review-Url: https://codereview.chromium.org/2886893010 Cr-Commit-Position: refs/heads/master@{#473744} Committed: https://chromium.googlesource.com/chromium/src/+/f0417c18b1e1f6b239780ea9c8eb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f0417c18b1e1f6b239780ea9c8eb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
