Added code for AVFoundation Finch Experiment enabling

Added code for AVFoundation Finch Experiment enabling

BUG=288562, 332034
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243933

[mcasas, 2014-01-08 15:02:10]

mathp@ could you PTAL? Thanks!
perkj@ OWNERS stamp please.

+rsesek FYI.

[perkj_chrome, 2014-01-09 10:31:16]

code lgtm

[Mathieu, 2014-01-09 14:10:26]

On 2014/01/09 10:31:16, perkj wrote:
> code lgtm

I've put asvitkine, he's more used to it.

[Alexei Svitkine (slow), 2014-01-09 15:50:38]

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
File media/video/capture/mac/avfoundation_glue.mm (right):

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
media/video/capture/mac/avfoundation_glue.mm:68:
media::UseAVFoundationExperiment()) && base::mac::IsOSLionOrLater() &&
This is a bit tricky. One thing to understand is that querying the field trial
will mark it as "active", which in this case (since you're OS-version based and
also will have a forcing_flag) means that the order you do it in is important.

Here's the order I suggest:

bool UseAVFoundationExperiment() {
   if (base::mac::IsOSLionOrLater())
     return false;
   const std::string group_name =
       base::FieldTrialList::FindFullName("AVFoundationMacVideoCapture");
   const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
   return cmd_line->HasSwitch(switches::kEnableAVFoundation) || group_name ==
"Enabled";
}

Why this order?

First, if you're on the wrong OS version, we shouldn't be marking you as being
in any experimental group (since it would skew results since the reality is this
feature wouldn't be being used there anyway). That's why that's done before
querying the field trial.

Next, we query the field trial API to mark it as active before checking the
command-line. That way, if a forcing flag is used, then the trial is still
marked as active (since there's no mechanism to mark a trial as active if a
given command-line flag is queried - so you have to to it in this order). Then,
if there's no forcing flag used, default to the experiment group.

Makes sense?

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
media/video/capture/mac/avfoundation_glue.mm:69: [AVFoundationBundle() load];
Not super familiar with this code, but is it safe to call -load multiple times
if IsAVFoundationSupported() is called multiple times? If not, seems like the
API can result in an error in that case...

[Alexei Svitkine (slow), 2014-01-09 15:56:20]

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
File media/video/capture/mac/avfoundation_glue.mm (right):

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
media/video/capture/mac/avfoundation_glue.mm:68:
media::UseAVFoundationExperiment()) && base::mac::IsOSLionOrLater() &&
On 2014/01/09 15:50:39, Alexei Svitkine wrote:
> This is a bit tricky. One thing to understand is that querying the field trial
> will mark it as "active", which in this case (since you're OS-version based
and
> also will have a forcing_flag) means that the order you do it in is important.
> 
> Here's the order I suggest:
> 
> bool UseAVFoundationExperiment() {
>    if (base::mac::IsOSLionOrLater())
>      return false;
>    const std::string group_name =
>        base::FieldTrialList::FindFullName("AVFoundationMacVideoCapture");
>    const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
>    return cmd_line->HasSwitch(switches::kEnableAVFoundation) || group_name ==
> "Enabled";
> }
> 
> Why this order?
> 
> First, if you're on the wrong OS version, we shouldn't be marking you as being
> in any experimental group (since it would skew results since the reality is
this
> feature wouldn't be being used there anyway). That's why that's done before
> querying the field trial.
> 
> Next, we query the field trial API to mark it as active before checking the
> command-line. That way, if a forcing flag is used, then the trial is still
> marked as active (since there's no mechanism to mark a trial as active if a
> given command-line flag is queried - so you have to to it in this order).
Then,
> if there's no forcing flag used, default to the experiment group.
> 
> Makes sense?

Just to clarify, since I think I didn't give enough context. When a field trial
is "active", then it will be reported to UMA and its metrics can be analyzed. If
it's not marked as active, then it's as if it doesn't exist. Hence it's
important to not set it as active if it doesn't apply (i.e. wrong OS version).

[mcasas, 2014-01-09 16:20:34]

asvitkine@ PTAL

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
File media/video/capture/mac/avfoundation_glue.mm (right):

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
media/video/capture/mac/avfoundation_glue.mm:68:
media::UseAVFoundationExperiment()) && base::mac::IsOSLionOrLater() &&
On 2014/01/09 15:56:20, Alexei Svitkine wrote:
> On 2014/01/09 15:50:39, Alexei Svitkine wrote:
> > This is a bit tricky. One thing to understand is that querying the field
trial
> > will mark it as "active", which in this case (since you're OS-version based
> and
> > also will have a forcing_flag) means that the order you do it in is
important.
> > 
> > Here's the order I suggest:
> > 
> > bool UseAVFoundationExperiment() {
> >    if (base::mac::IsOSLionOrLater())
> >      return false;
> >    const std::string group_name =
> >        base::FieldTrialList::FindFullName("AVFoundationMacVideoCapture");
> >    const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
> >    return cmd_line->HasSwitch(switches::kEnableAVFoundation) || group_name
==
> > "Enabled";
> > }
> > 
> > Why this order?
> > 
> > First, if you're on the wrong OS version, we shouldn't be marking you as
being
> > in any experimental group (since it would skew results since the reality is
> this
> > feature wouldn't be being used there anyway). That's why that's done before
> > querying the field trial.
> > 
> > Next, we query the field trial API to mark it as active before checking the
> > command-line. That way, if a forcing flag is used, then the trial is still
> > marked as active (since there's no mechanism to mark a trial as active if a
> > given command-line flag is queried - so you have to to it in this order).
> Then,
> > if there's no forcing flag used, default to the experiment group.
> > 
> > Makes sense?
> 
> Just to clarify, since I think I didn't give enough context. When a field
trial
> is "active", then it will be reported to UMA and its metrics can be analyzed.
If
> it's not marked as active, then it's as if it doesn't exist. Hence it's
> important to not set it as active if it doesn't apply (i.e. wrong OS version).

I think it makes sense: only use the experiment code for those platforms
where it can be used. I didn't know that every time the FileTrialList class is 
used, it already counts as a YES/NO experiment.

Only thing is that [AVFoundationBundle() load] should still be there
somewhere.

https://codereview.chromium.org/128373002/diff/50001/media/video/capture/mac/...
media/video/capture/mac/avfoundation_glue.mm:69: [AVFoundationBundle() load];
On 2014/01/09 15:50:39, Alexei Svitkine wrote:
> Not super familiar with this code, but is it safe to call -load multiple times
> if IsAVFoundationSupported() is called multiple times? If not, seems like the
> API can result in an error in that case...

NSBundle -load can be called safely several times (is a 
special beast).

[Alexei Svitkine (slow), 2014-01-09 16:25:01]

LGTM, thanks!

[commit-bot: I haz the power, 2014-01-09 16:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mcasas@chromium.org/128373002/150001

[commit-bot: I haz the power, 2014-01-09 18:39:31]

Change committed as 243933