Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(414)

Issue 2711153006: Add RendererFactorySelector (Closed)

Created:
3 years, 10 months ago by tguilbert
Modified:
3 years, 8 months ago
CC:
apacible+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, erickung+watch_chromium.org, feature-media-reviews_chromium.org, jam, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, xjz+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add RendererFactorySelector There are 4 types of renderers that are used by WMPI directly, with a potential fifth type in the works. Currently, a single RendererFactory is chosen at WMPI creation time, and bound for the lifetime of WMPI. However, there is a growing need to allow the smooth transition between the use of different renderer types, after WMPI’s creation. This CL introduces the RendererFactorySelector, which will allow us to choose between different RendererFactory types at runtime. Its purpose is to aggregate the signals and centralize the logic necessary to choose which RendererFactory should be used when creating a new Renderer. The interface is still extremelly simple, but it is expected to grow as the number of concurrent RendererFactory types grows. In other words, if there are 3+ renderer factories at play, some complexity is inevitable, and the right place for that complexity to live is in the RendererFactorySelector. NOTE: The RendererFactorySelector uses a FactoryType enum as a key, in order to avoid having to take dependencies on different (sometimes platform specific) RendererFactories. BUG=663503 Review-Url: https://codereview.chromium.org/2711153006 Cr-Commit-Position: refs/heads/master@{#466837} Committed: https://chromium.googlesource.com/chromium/src/+/70d2a00a72c50c344436f139b2a3f5c09ec337bf

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Added UTs #

Total comments: 1

Patch Set 4 : Typo #

Total comments: 12

Patch Set 5 : Addressed comments #

Patch Set 6 : Removed flat_map. Temp fix of -Wsometimes-initialized #

Unified diffs Side-by-side diffs Delta from patch set Stats (+210 lines, -13 lines) Patch
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 7 chunks +22 lines, -1 line 0 comments Download
M media/base/BUILD.gn View 1 2 2 chunks +3 lines, -0 lines 0 comments Download
A media/base/renderer_factory_selector.h View 1 2 3 4 5 1 chunk +51 lines, -0 lines 0 comments Download
A media/base/renderer_factory_selector.cc View 1 2 3 4 5 1 chunk +44 lines, -0 lines 0 comments Download
A media/base/renderer_factory_selector_unittest.cc View 1 2 1 chunk +73 lines, -0 lines 0 comments Download
M media/blink/webmediaplayer_impl.h View 1 2 3 4 5 4 chunks +5 lines, -5 lines 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 3 4 5 4 chunks +4 lines, -4 lines 0 comments Download
M media/blink/webmediaplayer_impl_unittest.cc View 1 2 3 1 chunk +8 lines, -3 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 32 (16 generated)
tguilbert
PTAL! For context: This CL is part of a series of CLs. The simplest thing ...
3 years, 8 months ago (2017-04-19 03:24:14 UTC) #3
DaleCurtis
Sorry, probably won't get to this until tomorrow, finishing up manager perf and meetings all ...
3 years, 8 months ago (2017-04-19 21:15:00 UTC) #4
DaleCurtis
lgtm, interface looks nice!
3 years, 8 months ago (2017-04-20 18:41:50 UTC) #5
tguilbert
+ nick@, can you OWNERS review content/renderer/render_frame_impl.cc?
3 years, 8 months ago (2017-04-21 23:00:19 UTC) #7
whywhat
lgtm I haven't looked in much detail at the following CLs yet. https://codereview.chromium.org/2711153006/diff/80001/media/base/renderer_factory_selector.h File media/base/renderer_factory_selector.h ...
3 years, 8 months ago (2017-04-22 15:52:50 UTC) #12
ncarter (slow)
No objections here from a content/ perspective, but one question. https://codereview.chromium.org/2711153006/diff/80001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2711153006/diff/80001/content/renderer/render_frame_impl.cc#newcode2946 ...
3 years, 8 months ago (2017-04-24 17:14:33 UTC) #13
tguilbert
https://codereview.chromium.org/2711153006/diff/80001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2711153006/diff/80001/content/renderer/render_frame_impl.cc#newcode2946 content/renderer/render_frame_impl.cc:2946: std::move(media_renderer_factory), std::move(remoting_controller)); On 2017/04/24 17:14:33, ncarter wrote: > This ...
3 years, 8 months ago (2017-04-24 19:31:16 UTC) #14
ncarter (slow)
https://codereview.chromium.org/2711153006/diff/80001/content/renderer/render_frame_impl.cc File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2711153006/diff/80001/content/renderer/render_frame_impl.cc#newcode2946 content/renderer/render_frame_impl.cc:2946: std::move(media_renderer_factory), std::move(remoting_controller)); On 2017/04/24 19:31:16, tguilbert wrote: > On ...
3 years, 8 months ago (2017-04-24 19:50:29 UTC) #15
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/2711153006/100001
3 years, 8 months ago (2017-04-24 20:28:06 UTC) #18
whywhat
https://codereview.chromium.org/2711153006/diff/80001/media/base/renderer_factory_selector.h File media/base/renderer_factory_selector.h (right): https://codereview.chromium.org/2711153006/diff/80001/media/base/renderer_factory_selector.h#newcode41 media/base/renderer_factory_selector.h:41: base::flat_map<FactoryType, std::unique_ptr<RendererFactory>> factories_; On 2017/04/24 at 19:31:16, tguilbert wrote: ...
3 years, 8 months ago (2017-04-24 20:35:48 UTC) #19
commit-bot: I haz the power
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_linux/builds/353862)
3 years, 8 months ago (2017-04-24 20:48:09 UTC) #21
tguilbert
On 2017/04/24 20:35:48, whywhat wrote: > https://codereview.chromium.org/2711153006/diff/80001/media/base/renderer_factory_selector.h > File media/base/renderer_factory_selector.h (right): > > https://codereview.chromium.org/2711153006/diff/80001/media/base/renderer_factory_selector.h#newcode41 > ...
3 years, 8 months ago (2017-04-24 22:29:16 UTC) #22
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/2711153006/120001
3 years, 8 months ago (2017-04-24 22:30:38 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/278704)
3 years, 8 months ago (2017-04-24 23:29:38 UTC) #27
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/2711153006/120001
3 years, 8 months ago (2017-04-24 23:39:07 UTC) #29
commit-bot: I haz the power
3 years, 8 months ago (2017-04-25 00:31:00 UTC) #32
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/70d2a00a72c50c344436f139b2a3...

Powered by Google App Engine
This is Rietveld 408576698