Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1528843004:
Only force preload=none over cellular for http, https, ftp URLs. (Closed)
|
Created:
5 years ago by DaleCurtis Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@media Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly force preload=none over cellular for http, https, ftp URLs.
There's no reason to force a limited preload for local or in memory
resources. This is important for WebView applications which may
have substantial content locally.
BUG=516766
TEST=new layout test, moved existing ones into http.
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update. #
Total comments: 1
Messages
Total messages: 11 (2 generated)
dalecurtis@chromium.org changed reviewers: + philipj@opera.com
WDYT?
Some ideas. I've been meaning to clean up the HTML spec around preload for a while, so this might be a good time if we can only decide how it should work. https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1911: // Force preload to none for cellular connections for online resources. I think the idea is right, but that modifying preloadType() depending on currentSrc isn't quite right. There's a spec issue relevant to this: https://github.com/whatwg/html/issues/295 My thinking is that the cleanest way to deal with this would be at the point where preload=none has an effect, in the resource fetch algorithm: https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... In other words, specify the conditions under which those suspend steps should be skipped, and I think that doing it for just data: and blob: URLs should be OK? (Carving out an exception for data: and friends seems easier than listing the normal cases.) A bit of searching suggests that Android WebView uses file: URLs for local resources, is that right? That's a bit tricky, because people might implicitly be using file: for local testing, and it's unfortunate if that should change the preload behavior...
srirama.m@samsung.com changed reviewers: + srirama.m@samsung.com
https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1911: // Force preload to none for cellular connections for online resources. On 2015/12/16 14:39:05, philipj wrote: > I think the idea is right, but that modifying preloadType() depending on > currentSrc isn't quite right. There's a spec issue relevant to this: > > https://github.com/whatwg/html/issues/295 > > My thinking is that the cleanest way to deal with this would be at the point > where preload=none has an effect, in the resource fetch algorithm: > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > In other words, specify the conditions under which those suspend steps should be > skipped, and I think that doing it for just data: and blob: URLs should be OK? > (Carving out an exception for data: and friends seems easier than listing the > normal cases.) > > A bit of searching suggests that Android WebView uses file: URLs for local > resources, is that right? That's a bit tricky, because people might implicitly > be using file: for local testing, and it's unfortunate if that should change the > preload behavior... Should we consider content:// (content providers in android) as well?
https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1911: // Force preload to none for cellular connections for online resources. On 2015/12/16 14:54:12, Srirama wrote: > On 2015/12/16 14:39:05, philipj wrote: > > I think the idea is right, but that modifying preloadType() depending on > > currentSrc isn't quite right. There's a spec issue relevant to this: > > > > https://github.com/whatwg/html/issues/295 > > > > My thinking is that the cleanest way to deal with this would be at the point > > where preload=none has an effect, in the resource fetch algorithm: > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > > > In other words, specify the conditions under which those suspend steps should > be > > skipped, and I think that doing it for just data: and blob: URLs should be OK? > > (Carving out an exception for data: and friends seems easier than listing the > > normal cases.) > > > > A bit of searching suggests that Android WebView uses file: URLs for local > > resources, is that right? That's a bit tricky, because people might implicitly > > be using file: for local testing, and it's unfortunate if that should change > the > > preload behavior... > > Should we consider content:// (content providers in android) as well? Does that currently work with media elements? http://developer.android.com/reference/android/content/ContentResolver.html looks complicated, does content:// always return a local resource, or could it redirect to something on the network?
On 2015/12/16 15:25:31, philipj wrote: > https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1911: // Force preload > to none for cellular connections for online resources. > On 2015/12/16 14:54:12, Srirama wrote: > > On 2015/12/16 14:39:05, philipj wrote: > > > I think the idea is right, but that modifying preloadType() depending on > > > currentSrc isn't quite right. There's a spec issue relevant to this: > > > > > > https://github.com/whatwg/html/issues/295 > > > > > > My thinking is that the cleanest way to deal with this would be at the point > > > where preload=none has an effect, in the resource fetch algorithm: > > > > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > > > > > In other words, specify the conditions under which those suspend steps > should > > be > > > skipped, and I think that doing it for just data: and blob: URLs should be > OK? > > > (Carving out an exception for data: and friends seems easier than listing > the > > > normal cases.) > > > > > > A bit of searching suggests that Android WebView uses file: URLs for local > > > resources, is that right? That's a bit tricky, because people might > implicitly > > > be using file: for local testing, and it's unfortunate if that should change > > the > > > preload behavior... > > > > Should we consider content:// (content providers in android) as well? > > Does that currently work with media elements? > http://developer.android.com/reference/android/content/ContentResolver.html > looks complicated, does content:// always return a local resource, or could it > redirect to something on the network? Not sure if it is currently working with media elements (I remember working with old android webview atleast for images using content://). And as far as i know content:// is only for local resources. Mainly to provide access to a local content which otherwise they will not be able to access directly.
Won't have time to look at this again until January. Happy holidays! https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1528843004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1911: // Force preload to none for cellular connections for online resources. On 2015/12/16 15:25:31, philipj wrote: > On 2015/12/16 14:54:12, Srirama wrote: > > On 2015/12/16 14:39:05, philipj wrote: > > > I think the idea is right, but that modifying preloadType() depending on > > > currentSrc isn't quite right. There's a spec issue relevant to this: > > > > > > https://github.com/whatwg/html/issues/295 > > > > > > My thinking is that the cleanest way to deal with this would be at the point > > > where preload=none has an effect, in the resource fetch algorithm: > > > > > > https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo... > > > > > > In other words, specify the conditions under which those suspend steps > should > > be > > > skipped, and I think that doing it for just data: and blob: URLs should be > OK? > > > (Carving out an exception for data: and friends seems easier than listing > the > > > normal cases.) > > > > > > A bit of searching suggests that Android WebView uses file: URLs for local > > > resources, is that right? That's a bit tricky, because people might > implicitly > > > be using file: for local testing, and it's unfortunate if that should change > > the > > > preload behavior... > > > > Should we consider content:// (content providers in android) as well? > > Does that currently work with media elements? > http://developer.android.com/reference/android/content/ContentResolver.html > looks complicated, does content:// always return a local resource, or could it > redirect to something on the network? There are a few different injectables AFAIK, WebView has one too: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android... ...so I went with an explicit white list of kinda-known network connections instead (service workers may be serving locally too over http/https, but not sure how that might be checked).
On 2015/12/17 00:02:19, DaleCurtis_OOO_Until_Jan_5 wrote: > Won't have time to look at this again until January. Happy holidays! Happy holidays! I'll be away until January 12, but may peek at my inbox before that.
Oof, finally got back to this. Haven't updated the tests yet, but is this what you were thinking about when you suggested modifying the load algorithm philipj@? Sites will no longer be able to detect that "preload=none" is in effect since there won't be a visible attribute change this way.
https://codereview.chromium.org/1528843004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1528843004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1011: } else if (networkStateNotifier().isCellularConnectionType() && (m_currentSrc.protocolIsInHTTPFamily() || m_currentSrc.protocolIs("ftp"))) { OK, so since we last discussed this, srirama@ has done some refactoring so that effectivePreloadType() is now the single source of truth for preload. So you should be able to simply move these bits into effectivePreloadType(), which would also make it very clear how it interacts with autoplay and the m_ignorePreloadNone state. (I assume that preload should not matter, but that m_ignorePreloadNone should take precedent, so some reshuffling will be needed.) If the log message is important, maybe effectivePreloadType() could take an extra out argument to say why preload was none. Seems like overkill, though. |
