Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873983002:
Add --enable-internal-media-session to trigger the MediaSession backend. (Closed)
|
Created:
3 years, 7 months ago by mlamouri (slow - plz ping) Modified:
3 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd --enable-internal-media-session to trigger the MediaSession backend.
This flag should be used by embedders that want to use the content::MediaSession
on platform on which it is not enabled by default.
BUG=719968
Review-Url: https://codereview.chromium.org/2873983002
Cr-Commit-Position: refs/heads/master@{#472058}
Committed: https://chromium.googlesource.com/chromium/src/+/d967040c162c7fb64299cd623f246da739962c38
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix android #Patch Set 3 : rebase #
Messages
Total messages: 32 (22 generated)
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
mlamouri@chromium.org changed reviewers: + hubbe@chromium.org
hubbe@, PTAL :)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
I'm afraid this is a bit outside my area of expertise. I don't see anything wrong with it, but I'm not sure I'm qualified to judge it's merits. Let me know if you can't find a better reviewer. https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h... media/base/media_switches.h:83: MEDIA_EXPORT extern const char kEnableInternalMediaSession[]; can we use base::Feature instead?
mlamouri@chromium.org changed reviewers: + avayvod@chromium.org
+avayvod@ for content/browser/media/session/ https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h... media/base/media_switches.h:83: MEDIA_EXPORT extern const char kEnableInternalMediaSession[]; On 2017/05/10 at 17:41:55, hubbe wrote: > can we use base::Feature instead? It is fairly painful and an uncommon pattern to toggle a feature during runtime. I can't find any precedent for this. The idea of this switch is that some embedders would turn on the switch when needed, the same way ChromeVox turns on the audio focus. Happy to make it a feature if this pattern can easily be achieved with base::Feature.
The CQ bit was checked by mlamouri@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
media/session lgtm
https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h... media/base/media_switches.h:83: MEDIA_EXPORT extern const char kEnableInternalMediaSession[]; On 2017/05/11 08:48:24, mlamouri wrote: > On 2017/05/10 at 17:41:55, hubbe wrote: > > can we use base::Feature instead? > > It is fairly painful and an uncommon pattern to toggle a feature during runtime. > I can't find any precedent for this. The idea of this switch is that some > embedders would turn on the switch when needed, the same way ChromeVox turns on > the audio focus. Happy to make it a feature if this pattern can easily be > achieved with base::Feature. Don't we have some internal place to store such things? Like content_settings or something? It seems unsanitary to mix up internally configurable things and command-line flags.
On 2017/05/11 at 17:58:26, hubbe wrote: > https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h > File media/base/media_switches.h (right): > > https://codereview.chromium.org/2873983002/diff/1/media/base/media_switches.h... > media/base/media_switches.h:83: MEDIA_EXPORT extern const char kEnableInternalMediaSession[]; > On 2017/05/11 08:48:24, mlamouri wrote: > > On 2017/05/10 at 17:41:55, hubbe wrote: > > > can we use base::Feature instead? > > > > It is fairly painful and an uncommon pattern to toggle a feature during runtime. > > I can't find any precedent for this. The idea of this switch is that some > > embedders would turn on the switch when needed, the same way ChromeVox turns on > > the audio focus. Happy to make it a feature if this pattern can easily be > > achieved with base::Feature. > > Don't we have some internal place to store such things? > Like content_settings or something? > It seems unsanitary to mix up internally configurable things and command-line flags. command line flags have always been used for this as far as I know. If you search for code that AppendSwitch() outside of tests, you will find many occurences.
The CQ bit was checked by mlamouri@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by mlamouri@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...
PTAL
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2873983002/#ps40001 (title: "rebase")
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": 40001, "attempt_start_ts": 1494929914135470,
"parent_rev": "0ac21bec152a95d8edb8e759f765a512d552357c", "commit_rev":
"d967040c162c7fb64299cd623f246da739962c38"}
Message was sent while issue was closed.
Description was changed from ========== Add --enable-internal-media-session to trigger the MediaSession backend. This flag should be used by embedders that want to use the content::MediaSession on platform on which it is not enabled by default. BUG=719968 ========== to ========== Add --enable-internal-media-session to trigger the MediaSession backend. This flag should be used by embedders that want to use the content::MediaSession on platform on which it is not enabled by default. BUG=719968 Review-Url: https://codereview.chromium.org/2873983002 Cr-Commit-Position: refs/heads/master@{#472058} Committed: https://chromium.googlesource.com/chromium/src/+/d967040c162c7fb64299cd623f24... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d967040c162c7fb64299cd623f24... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
