Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2979893002:
media: Allow suspend on HTTP 200 videos (Closed)
|
Created:
3 years, 5 months ago by hubbe Modified:
3 years, 5 months ago Reviewers:
sandersd (OOO until July 31) CC:
chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, apacible+watch_chromium.org, xjz+watch_chromium.org, mfoltz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Allow suspend on HTTP 200 videos
Currently, connections are kept open indefinitely, which
can block future loads from a server if there are 6 or more
<video> tags on the same page. This should fix that in most
cases.
BUG=716748
Review-Url: https://codereview.chromium.org/2979893002
Cr-Commit-Position: refs/heads/master@{#486919}
Committed: https://chromium.googlesource.com/chromium/src/+/e2cc88c09afab5f066c83032548c1180d99c5ae7
Patch Set 1 #
Total comments: 9
Patch Set 2 : comments adressed #Patch Set 3 : comments adressed #Patch Set 4 : fixed tests and bugs #Patch Set 5 : test fix #Patch Set 6 : layout test fix #
Messages
Total messages: 33 (25 generated)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hubbe@chromium.org changed reviewers: + sandersd@chromium.org
https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2085: int64_t size; nit: move after comment (and possibly rename to |unused|). https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2089: // video, and only if we know the length of the video. (If we don't know the length -> size https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2093: CurrentTime() != 0.0 && data_source_ && data_source_->GetSize(&size)) { What's the reasoning for kReadyStateHaveNothing? It seems like we should be fine to do this whenever CurrentTime() is 0.0. Does the data source not create a new connection in those cases?
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2085: int64_t size; On 2017/07/12 21:19:20, sandersd wrote: > nit: move after comment (and possibly rename to |unused|). Done. https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2089: // video, and only if we know the length of the video. (If we don't know the On 2017/07/12 21:19:19, sandersd wrote: > length -> size size seems confusing how about "file size" ? https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2093: CurrentTime() != 0.0 && data_source_ && data_source_->GetSize(&size)) { On 2017/07/12 21:19:20, sandersd wrote: > What's the reasoning for kReadyStateHaveNothing? It seems like we should be fine > to do this whenever CurrentTime() is 0.0. Does the data source not create a new > connection in those cases? Not allowed to call CurrentTime() if ready_state_ is kReadyStateHaveNothing
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2089: // video, and only if we know the length of the video. (If we don't know the On 2017/07/12 21:28:47, hubbe wrote: > On 2017/07/12 21:19:19, sandersd wrote: > > length -> size > > size seems confusing > how about "file size" ? sgtm https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2093: CurrentTime() != 0.0 && data_source_ && data_source_->GetSize(&size)) { On 2017/07/12 21:28:47, hubbe wrote: > On 2017/07/12 21:19:20, sandersd wrote: > > What's the reasoning for kReadyStateHaveNothing? It seems like we should be > fine > > to do this whenever CurrentTime() is 0.0. Does the data source not create a > new > > connection in those cases? > > Not allowed to call CurrentTime() if ready_state_ is kReadyStateHaveNothing Isn't that the same as saying that we can skip the time check entirely before then?
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2979893002/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:2093: CurrentTime() != 0.0 && data_source_ && data_source_->GetSize(&size)) { On 2017/07/12 21:37:05, sandersd wrote: > On 2017/07/12 21:28:47, hubbe wrote: > > On 2017/07/12 21:19:20, sandersd wrote: > > > What's the reasoning for kReadyStateHaveNothing? It seems like we should be > > fine > > > to do this whenever CurrentTime() is 0.0. Does the data source not create a > > new > > > connection in those cases? > > > > Not allowed to call CurrentTime() if ready_state_ is kReadyStateHaveNothing > > Isn't that the same as saying that we can skip the time check entirely before > then? I think I had some of this logic wrong. Changed it so to be more obvious what's going on. Better now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hubbe@chromium.org
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2979893002/#ps100001 (title: "layout test fix")
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": 100001, "attempt_start_ts": 1500070684689690,
"parent_rev": "b1573039b77716775b1309715ef8c9d3b34eef70", "commit_rev":
"ad1e9d9be0061e55bd55e6657d4d0c8f08d61ebd"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500070684689690,
"parent_rev": "69c312efddbfbcbe648bf4435f5bbb2b52441b14", "commit_rev":
"e2cc88c09afab5f066c83032548c1180d99c5ae7"}
Message was sent while issue was closed.
Description was changed from ========== media: Allow suspend on HTTP 200 videos Currently, connections are kept open indefinitely, which can block future loads from a server if there are 6 or more <video> tags on the same page. This should fix that in most cases. BUG=716748 ========== to ========== media: Allow suspend on HTTP 200 videos Currently, connections are kept open indefinitely, which can block future loads from a server if there are 6 or more <video> tags on the same page. This should fix that in most cases. BUG=716748 Review-Url: https://codereview.chromium.org/2979893002 Cr-Commit-Position: refs/heads/master@{#486919} Committed: https://chromium.googlesource.com/chromium/src/+/e2cc88c09afab5f066c83032548c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e2cc88c09afab5f066c83032548c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
