Enable default media session when ChromeVox is toggled.

Enable default media session when ChromeVox is toggled.

BUG=621697
Committed: https://crrev.com/d50debe3d51323319ba9b42e2a7d1fde593f3b06
Cr-Commit-Position: refs/heads/master@{#440942}

[David Tseng, 2016-12-14 18:10:21]

Description was changed from

==========
Enable default media session when ChromeVox is toggled.

BUG=
==========

to

==========
Enable default media session when ChromeVox is toggled.

BUG=621697
==========

[David Tseng, 2016-12-14 18:10:21]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[David Tseng, 2016-12-14 18:12:46]

dmazzoni: reviewer
mlamouri: fyi

I realize this is a one-way flip, but I wanted to get something easily
revertable (if needed).

[dmazzoni, 2016-12-15 17:01:37]

What will happen to existing media playing?

Adding a command-line switch anytime other than
on startup is a rather unusual way to flip on a feature.

[David Tseng, 2016-12-15 17:17:02]

On Thu, Dec 15, 2016 at 9:01 AM, <dmazzoni@chromium.org> wrote:
What will happen to existing media playing?

This was tested explicitly. Was there a concern here I missed?

Adding a command-line switch anytime other than
on startup is a rather unusual way to flip on a feature.

If you have a suggestion other than plumbing through the content -> chrome
abstraction, I'm open to it. Seeing that the flag is temporary, I'm
inclined to not fuss about it.


https://codereview.chromium.org/2577713002/
-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-12-15 18:27:42]

lgtm

OK with this if mlamouri@ doesn't prefer refactoring it.
Wouldn't need a content/public API, could just add a
more explicit API in media/base/. But since it's a temporary
flag maybe that's fine.

Just one request, then.

https://codereview.chromium.org/2577713002/diff/1/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2577713002/diff/1/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:1315:
base::CommandLine::ForCurrentProcess()->AppendSwitch(
Add a guard to do this only if the flag isn't already set, for two reasons:
1. Don't want to keep appending it when toggling, and
2. The flag may have already been set to kEnableDefaultMediaSessionDuckFlash
and you don't want to disable that

[David Tseng, 2016-12-15 23:05:55]

On Thu, Dec 15, 2016 at 10:27 AM, <dmazzoni@chromium.org> wrote:

> lgtm
>
> OK with this if mlamouri@ doesn't prefer refactoring it.
> Wouldn't need a content/public API, could just add a
> more explicit API in media/base/. But since it's a temporary
> flag maybe that's fine.
>
> I did actually look into doing what you suggested before going with
command line.

The thing that checks IsDefaultMediaSessionEnabled is in content, so I'm
not sure if I'm missing what you're saying.



> Just one request, then.
>
>
>
> https://codereview.chromium.org/2577713002/diff/1/chrome/browser/chromeos/
> accessibility/accessibility_manager.cc
> File chrome/browser/chromeos/accessibility/accessibility_manager.cc
> (right):
>
> https://codereview.chromium.org/2577713002/diff/1/chrome/browser/chromeos/
> accessibility/accessibility_manager.cc#newcode1315
> chrome/browser/chromeos/accessibility/accessibility_manager.cc:1315:
> base::CommandLine::ForCurrentProcess()->AppendSwitch(
> Add a guard to do this only if the flag isn't already set, for two
> reasons:
> 1. Don't want to keep appending it when toggling, and
> 2. The flag may have already been set to
> kEnableDefaultMediaSessionDuckFlash
> and you don't want to disable that
>
> Yes, good point. Done.



> https://codereview.chromium.org/2577713002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2016-12-15 23:36:53]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 23:37:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 01:27:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 01:27:53]

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_...)

[David Tseng, 2016-12-18 17:47:56]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-12-18 17:47:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2577713002/#ps20001
(title: "Enable default media session when ChromeVox is toggled.")

[commit-bot: I haz the power, 2016-12-18 17:48:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-18 17:48:05]

This CL has an open dependency (Issue 2563013003 Patch 100001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[David Tseng, 2016-12-18 17:48:45]

The CQ bit was unchecked by dtseng@chromium.org

[David Tseng, 2016-12-29 05:57:09]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-12-29 05:57:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 06:23:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482991029433280,
"parent_rev": "bdcae291a5c81774063b8d120c38282d181120e7", "commit_rev":
"608598208378f1a52b4f2e2af620732ccc816870"}

[commit-bot: I haz the power, 2016-12-29 06:24:55]

Description was changed from

==========
Enable default media session when ChromeVox is toggled.

BUG=621697
==========

to

==========
Enable default media session when ChromeVox is toggled.

BUG=621697

Review-Url: https://codereview.chromium.org/2577713002
==========

[commit-bot: I haz the power, 2016-12-29 06:24:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2017-01-02 15:51:54]

Description was changed from

==========
Enable default media session when ChromeVox is toggled.

BUG=621697

Review-Url: https://codereview.chromium.org/2577713002
==========

to

==========
Enable default media session when ChromeVox is toggled.

BUG=621697

Committed: https://crrev.com/d50debe3d51323319ba9b42e2a7d1fde593f3b06
Cr-Commit-Position: refs/heads/master@{#440942}
==========

[commit-bot: I haz the power, 2017-01-02 15:51:55]

Patchset 2 (id:??) landed as
https://crrev.com/d50debe3d51323319ba9b42e2a7d1fde593f3b06
Cr-Commit-Position: refs/heads/master@{#440942}