Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1381613003:
Defer media loading while on cellular networks. (Closed)
|
Created:
5 years, 2 months ago by DaleCurtis Modified:
5 years, 2 months ago CC:
chromium-reviews, blink-reviews-html_chromium.org, mlamouri+watch-blink_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, jkarlin+watch_chromium.org, blink-reviews, vcarbune.chromium, loading-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDefer media loading while on cellular networks.
We only populate networkStateNotifier()'s connectionType() field on
a couple platforms, see http://crbug.com/368358 for details, but it
does work on Android at least.
This prevents us from wasting bandwidth on videos when the device is
on a cellular connection.
BUG=516766
TEST=Load deferred when forced to true, tested on device.
Committed: https://crrev.com/58b4584a73adad4572cceb9e062c7338482ba36a
Cr-Commit-Position: refs/heads/master@{#355699}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments. #
Total comments: 13
Patch Set 3 : Comments. #Patch Set 4 : Rebase. #Messages
Total messages: 32 (10 generated)
dalecurtis@chromium.org changed reviewers: + philipj@opera.com, watk@chromium.org
(moved from WebKit checkout)
Some thoughts on how to best test this. If it's possible to simply write this as a unit test, that might be the most direct way, but LayoutTests isn't too bad here either. https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-preload-default-cellular.html (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-preload-default-cellular.html:4: /* Use a black background to see failures; the video clip is a white square. */ I ought not be necessary to use a reftest for this at all, instead just pass on the suspend event and fail on the progress event. Written using testharness.js this test could be more concince, and with no expectations file: <script> async_test(function(t) { internals.setNetworkStateNotifierTestOnly(true); internals.setNetworkConnectionInfo('cellular', 2.0); var video = document.createElement("video"); video.src = "resources/test-positive-start-time.webm"; video.onsuspend = t.step_func_done(); video.onprogress = t.unreached_func(); t.add_cleanup(function() { internals.setNetworkStateNotifierTestOnly(false); }); }); (Not tested, likely has typos.) https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-preload-override-cellular.html (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-preload-override-cellular.html:2: <title>Tests that preload is overridden to none for cellular connections.</title> Since this test is almost identical, what you could do is just have a video element in both tests and include the shared js that would do document.querySelector("video"). https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js:14: // Reset the state of the singleton network state notifier. Is this needed to prevent some other tests from failing? Would the t.add_cleanup() construct make this redundant? https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1967: if (networkStateNotifier().connectionType() == WebConnectionTypeCellular) Make this an early return at the top, which should make this diff very small. https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:109: if (updatesOnly || m_testUpdatesOnly) { Should this say updatesOnly != m_testUpdatseOnly in order to match the comment? If so an early `if (m_testUpdatesOnly == updatesOnly) return` would be equivalent and more clear. Also, are the new tests doing something special to make this necessary?
https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1967: if (networkStateNotifier().connectionType() == WebConnectionTypeCellular) On 2015/10/01 09:24:07, philipj wrote: > Make this an early return at the top, which should make this diff very small. As pointed out in the other review, this should then have its own use counter.
Unfortunately, I'm heading OOO today, so I won't get back to this until Oct 16, but here are some short replies. watk@ if you feel so inclined, feel free to pick this up while I'm out. It's not pressing though, so we can wait until I get back. https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-preload-default-cellular.html (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-preload-default-cellular.html:4: /* Use a black background to see failures; the video clip is a white square. */ On 2015/10/01 09:24:07, philipj wrote: > I ought not be necessary to use a reftest for this at all, instead just pass on > the suspend event and fail on the progress event. Written using testharness.js > this test could be more concince, and with no expectations file: > > <script> > async_test(function(t) { > internals.setNetworkStateNotifierTestOnly(true); > internals.setNetworkConnectionInfo('cellular', 2.0); > var video = document.createElement("video"); > video.src = "resources/test-positive-start-time.webm"; > video.onsuspend = t.step_func_done(); > video.onprogress = t.unreached_func(); > t.add_cleanup(function() { > internals.setNetworkStateNotifierTestOnly(false); > }); > }); > > (Not tested, likely has typos.) Good idea, though I think suspend might tick before progress always, will have to check later. https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js:14: // Reset the state of the singleton network state notifier. On 2015/10/01 09:24:07, philipj wrote: > Is this needed to prevent some other tests from failing? Would the > t.add_cleanup() construct make this redundant? No, it's not causing any failures, but w/o it the tests may have ordering dependencies since the settings of one will carry over into the other. https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:109: if (updatesOnly || m_testUpdatesOnly) { On 2015/10/01 09:24:07, philipj wrote: > Should this say updatesOnly != m_testUpdatseOnly in order to match the comment? > If so an early `if (m_testUpdatesOnly == updatesOnly) return` would be > equivalent and more clear. > > Also, are the new tests doing something special to make this necessary? w/o this if I disable the setWebConnectionFortest() in one layout test, but not the other, both will pass so long as the one that sets the values runs first. This ensures there's no ordering dependencies.
Won't have time to get to these comments before heading OOO until the 16th, watk@ if you feel inclined feel to take this over. Otherwise I'll pick it back up in a couple weeks.
On 2015/10/01 20:16:28, DaleCurtis_OOO_Until_Oct_16 wrote: > Won't have time to get to these comments before heading OOO until the 16th, > watk@ if you feel inclined feel to take this over. Otherwise I'll pick it back > up in a couple weeks. Whoops, double post :)
https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-preload-default-cellular.html (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-preload-default-cellular.html:4: /* Use a black background to see failures; the video clip is a white square. */ On 2015/10/01 19:25:54, DaleCurtis wrote: > On 2015/10/01 09:24:07, philipj wrote: > > I ought not be necessary to use a reftest for this at all, instead just pass > on > > the suspend event and fail on the progress event. Written using testharness.js > > this test could be more concince, and with no expectations file: > > > > <script> > > async_test(function(t) { > > internals.setNetworkStateNotifierTestOnly(true); > > internals.setNetworkConnectionInfo('cellular', 2.0); > > var video = document.createElement("video"); > > video.src = "resources/test-positive-start-time.webm"; > > video.onsuspend = t.step_func_done(); > > video.onprogress = t.unreached_func(); > > t.add_cleanup(function() { > > internals.setNetworkStateNotifierTestOnly(false); > > }); > > }); > > > > (Not tested, likely has typos.) > > Good idea, though I think suspend might tick before progress always, will have > to check later. This worked perfectly, thanks! https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-preload-override-cellular.html (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-preload-override-cellular.html:2: <title>Tests that preload is overridden to none for cellular connections.</title> On 2015/10/01 09:24:07, philipj wrote: > Since this test is almost identical, what you could do is just have a video > element in both tests and include the shared js that would do > document.querySelector("video"). Kind of done; refactored to a shared js, but async_test() seems to need to be in the main html or it won't wait for the DOM to load completely -- subsequently it can't find the video tag via querySelector. https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1967: if (networkStateNotifier().connectionType() == WebConnectionTypeCellular) On 2015/10/01 09:49:49, philipj wrote: > On 2015/10/01 09:24:07, philipj wrote: > > Make this an early return at the top, which should make this diff very small. > > As pointed out in the other review, this should then have its own use counter. Done -- but to be clear, doing this ruins the rest of the use counter stats; they will now go unlogged for all cellular traffic. https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:109: if (updatesOnly || m_testUpdatesOnly) { On 2015/10/01 19:25:54, DaleCurtis wrote: > On 2015/10/01 09:24:07, philipj wrote: > > Should this say updatesOnly != m_testUpdatseOnly in order to match the > comment? > > If so an early `if (m_testUpdatesOnly == updatesOnly) return` would be > > equivalent and more clear. > > > > Also, are the new tests doing something special to make this necessary? > > w/o this if I disable the setWebConnectionFortest() in one layout test, but not > the other, both will pass so long as the one that sets the values runs first. > This ensures there's no ordering dependencies. Done.
philipj@opera.com changed reviewers: + jkarlin@chromium.org
Media bits LGTM with nits, but I'd like jkarlin@ to take a second look at NetworkStateNotifier.cpp https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1381613003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1967: if (networkStateNotifier().connectionType() == WebConnectionTypeCellular) On 2015/10/20 21:56:51, DaleCurtis wrote: > On 2015/10/01 09:49:49, philipj wrote: > > On 2015/10/01 09:24:07, philipj wrote: > > > Make this an early return at the top, which should make this diff very > small. > > > > As pointed out in the other review, this should then have its own use counter. > > Done -- but to be clear, doing this ruins the rest of the use counter stats; > they will now go unlogged for all cellular traffic. Right, anyone looking at the stats has to understand that it's not a measurement of what attribute values appear in the page, but rather of what the effective preload state is and why. (That is the information I would find valuable in estimating the risk in changing the behavior of any given preload state.) https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-preload-cellular-test.js (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-preload-cellular-test.js:1: function CellularPreloadTest(preloadType) { LayoutTests/media/video-controls-fullscreen.js shows how you can move the async_test bit into the shared JS. Although atypical, I would go with cellular_preload_test() to match async_test() or cellularPreloadTest() if you think that's lame :) https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-preload-cellular-test.js:8: video.preload = preloadType; After this line, can you also assert_equals(video.preload, "none")? Otherwise we could do the coercing entirely internally without failing this test. https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js:15: window.onbeforeunload = function() { Using addEventListener here to not risk conflicting with tests seems like a good idea. https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:859: HTMLMediaElementPreloadForcedNone = 979, Repos are merged, so now we can run update_use_counter_feature_enum.py in the same CL :) https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:110: m_isOnLine = true; I didn't look very closely before, but I see that there's actually a mutex for changing these values, not sure if there's any chance of a race in testing code though. Also this duplicates the values from the constructor, a reset method would solve that but could be overkill. jkarlin@, can you review these bits?
https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:108: // Reset state to default when entering or leaving test mode. Update the comment in the header file with this change. https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:110: m_isOnLine = true; On 2015/10/21 11:11:29, philipj wrote: > I didn't look very closely before, but I see that there's actually a mutex for > changing these values, not sure if there's any chance of a race in testing code > though. Yes, the mutex is required here. > Also this duplicates the values from the constructor, a reset method > would solve that but could be overkill. This seems okay as is.
dalecurtis@chromium.org changed reviewers: + rkaplow@chromium.org
+rkaplow for histograms.xml https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/video-preload-cellular-test.js (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-preload-cellular-test.js:1: function CellularPreloadTest(preloadType) { On 2015/10/21 11:11:29, philipj wrote: > LayoutTests/media/video-controls-fullscreen.js shows how you can move the > async_test bit into the shared JS. Although atypical, I would go with > cellular_preload_test() to match async_test() or cellularPreloadTest() if you > think that's lame :) lowercase_for_the_win(). https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/video-preload-cellular-test.js:8: video.preload = preloadType; On 2015/10/21 11:11:29, philipj wrote: > After this line, can you also assert_equals(video.preload, "none")? Otherwise we > could do the coercing entirely internally without failing this test. Done. https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js:15: window.onbeforeunload = function() { On 2015/10/21 11:11:29, philipj wrote: > Using addEventListener here to not risk conflicting with tests seems like a good > idea. Done. https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.h:859: HTMLMediaElementPreloadForcedNone = 979, On 2015/10/21 11:11:29, philipj wrote: > Repos are merged, so now we can run update_use_counter_feature_enum.py in the > same CL :) Done. https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right): https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:108: // Reset state to default when entering or leaving test mode. On 2015/10/21 13:24:49, jkarlin wrote: > Update the comment in the header file with this change. Done. https://codereview.chromium.org/1381613003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:110: m_isOnLine = true; On 2015/10/21 13:24:49, jkarlin wrote: > On 2015/10/21 11:11:29, philipj wrote: > > I didn't look very closely before, but I see that there's actually a mutex for > > changing these values, not sure if there's any chance of a race in testing > code > > though. > > Yes, the mutex is required here. > > > Also this duplicates the values from the constructor, a reset method > > would solve that but could be overkill. > > This seems okay as is. Done.
Description was changed from ========== Defer media loading while on cellular networks. We only populate networkStateNotifier()'s connectionType() field on a couple platforms, see http://crbug.com/368358 for details, but it does work on Android at least. This prevents us from wasting bandwidth on videos when the device is on a cellular connection. BUG=516766 TEST=Load deferred when forced to true, TBD: actual device test. ========== to ========== Defer media loading while on cellular networks. We only populate networkStateNotifier()'s connectionType() field on a couple platforms, see http://crbug.com/368358 for details, but it does work on Android at least. This prevents us from wasting bandwidth on videos when the device is on a cellular connection. BUG=516766 TEST=Load deferred when forced to true, tested on device. ==========
Even more LGTM :)
I have to say that I'm ambivalent about this change, since we're using network type in order to conclude something about the user's cost and/or bandwidth, which is not inherently true. (e.g. WiFi can be slow and costly, cellular can be fast and unlimited). With that said, today we don't know any better, but it would be great if we planned towards a future where we do. (e.g. ask the user to set cost/monthly cap at the OS level, and let the OS communicate that to the browser). Also, I think this merits a PSA on blink-dev.
lgtm histograms lgtm
NetworkStateNotifier.* lgtm
PSA sent, I'll CQ later today after a period for comments.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381613003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, rkaplow@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/1381613003/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381613003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381613003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1381613003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1381613003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/58b4584a73adad4572cceb9e062c7338482ba36a Cr-Commit-Position: refs/heads/master@{#355699} |
