Don't add fake flash when command-line flash exists.

Don't add fake flash when command-line flash exists.

BUG=650442
Committed: https://crrev.com/a1512fef16d73a51f020954dc81e51d84bc0e921
Cr-Commit-Position: refs/heads/master@{#422347}

[waffles, 2016-09-26 22:28:12]

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

[commit-bot: I haz the power, 2016-09-26 22:28:54]

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

[waffles, 2016-09-26 22:35:27]

Patchset #2 (id:20001) has been deleted

[waffles, 2016-09-26 22:35:34]

Patchset #1 (id:1) has been deleted

[waffles, 2016-09-26 22:35:42]

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

[commit-bot: I haz the power, 2016-09-26 22:36:18]

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

[waffles, 2016-09-26 22:47:12]

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

[waffles, 2016-09-26 22:47:26]

Patchset #1 (id:2) has been deleted

[commit-bot: I haz the power, 2016-09-26 22:47:28]

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

[waffles, 2016-09-26 22:49:08]

waffles@chromium.org changed reviewers:
+ kerrnel@chromium.org, thakis@chromium.org, wfh@chromium.org

[waffles, 2016-09-26 22:49:09]

Hi all, PTAL.

This should only be a difference in behavior for Google Chrome builds, as
FLAPPER_AVAILABLE is not defined for Chromium builds.

[Greg K, 2016-09-26 22:54:14]

https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:500: if (has_cli_flash)
I'm confused by this: why do we want to push back the command line version here?
That check, which I coincidentally rewrote today into a more accurate check,
tests for file system access. In this if clause, there is no filesystem access
and Chrome won't be able to load any files at this point.

[waffles, 2016-09-26 23:02:32]

https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:500: if (has_cli_flash)
On 2016/09/26 22:54:14, Greg K wrote:
> I'm confused by this: why do we want to push back the command line version
here?
> That check, which I coincidentally rewrote today into a more accurate check,
> tests for file system access. In this if clause, there is no filesystem access
> and Chrome won't be able to load any files at this point.

I don't know what the purpose of this is, I just decided it was wisest to retain
the existing behavior (command-line flash is added in
AddPepperFlashFromCommandLine before this `return`).

[commit-bot: I haz the power, 2016-09-26 23:39:32]

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

[commit-bot: I haz the power, 2016-09-26 23:39:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Greg K, 2016-09-27 18:06:32]

On 2016/09/26 23:02:32, waffles wrote:
>
https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
> File chrome/common/chrome_content_client.cc (right):
> 
>
https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
> chrome/common/chrome_content_client.cc:500: if (has_cli_flash)
> On 2016/09/26 22:54:14, Greg K wrote:
> > I'm confused by this: why do we want to push back the command line version
> here?
> > That check, which I coincidentally rewrote today into a more accurate check,
> > tests for file system access. In this if clause, there is no filesystem
access
> > and Chrome won't be able to load any files at this point.
> 
> I don't know what the purpose of this is, I just decided it was wisest to
retain
> the existing behavior (command-line flash is added in
> AddPepperFlashFromCommandLine before this `return`).

That was probably incorrect behavior before. Since I don't see anyway Chrome can
load the command line Flash player, once command line access is gone, we should
drop that clause (and just double check that command line flash still loads).
Thanks.

[waffles, 2016-09-27 20:59:45]

On 2016/09/27 18:06:32, Greg K wrote:
> On 2016/09/26 23:02:32, waffles wrote:
> >
>
https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
> > File chrome/common/chrome_content_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
> > chrome/common/chrome_content_client.cc:500: if (has_cli_flash)
> > On 2016/09/26 22:54:14, Greg K wrote:
> > > I'm confused by this: why do we want to push back the command line version
> > here?
> > > That check, which I coincidentally rewrote today into a more accurate
check,
> > > tests for file system access. In this if clause, there is no filesystem
> access
> > > and Chrome won't be able to load any files at this point.
> > 
> > I don't know what the purpose of this is, I just decided it was wisest to
> retain
> > the existing behavior (command-line flash is added in
> > AddPepperFlashFromCommandLine before this `return`).
> 
> That was probably incorrect behavior before. Since I don't see anyway Chrome
can
> load the command line Flash player, once command line access is gone, we
should
> drop that clause (and just double check that command line flash still loads).
> Thanks.

Done.

[Greg K, 2016-09-27 21:02:45]

On 2016/09/27 20:59:45, waffles wrote:
> On 2016/09/27 18:06:32, Greg K wrote:
> > On 2016/09/26 23:02:32, waffles wrote:
> > >
> >
>
https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
> > > File chrome/common/chrome_content_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2370683003/diff/50001/chrome/common/chrome_co...
> > > chrome/common/chrome_content_client.cc:500: if (has_cli_flash)
> > > On 2016/09/26 22:54:14, Greg K wrote:
> > > > I'm confused by this: why do we want to push back the command line
version
> > > here?
> > > > That check, which I coincidentally rewrote today into a more accurate
> check,
> > > > tests for file system access. In this if clause, there is no filesystem
> > access
> > > > and Chrome won't be able to load any files at this point.
> > > 
> > > I don't know what the purpose of this is, I just decided it was wisest to
> > retain
> > > the existing behavior (command-line flash is added in
> > > AddPepperFlashFromCommandLine before this `return`).
> > 
> > That was probably incorrect behavior before. Since I don't see anyway Chrome
> can
> > load the command line Flash player, once command line access is gone, we
> should
> > drop that clause (and just double check that command line flash still
loads).
> > Thanks.
> 
> Done.

lgtm

[Nico, 2016-09-27 21:09:15]

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:494:
std::unique_ptr<content::PepperPluginInfo> command_line_flash(
why is this on the heap?

Also, why the api changin in GetCommandLinePepperFlash()? Couldn't you just pass
flash_versions instead of plugins to the old GetCommandLinePepperFlash()?

[Greg K, 2016-09-27 21:12:30]

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:494:
std::unique_ptr<content::PepperPluginInfo> command_line_flash(
On 2016/09/27 21:09:14, Nico (mostly away until Oct 3) wrote:
> why is this on the heap?
> 
> Also, why the api changin in GetCommandLinePepperFlash()? Couldn't you just
pass
> flash_versions instead of plugins to the old GetCommandLinePepperFlash()?

FWIW, since the other functions take a PepperPluginInfo*, I like the continuity
here, unless we have them all take the flash_versions list.

[Nico, 2016-09-27 21:13:40]

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:494:
std::unique_ptr<content::PepperPluginInfo> command_line_flash(
On 2016/09/27 21:12:29, Greg K wrote:
> On 2016/09/27 21:09:14, Nico (mostly away until Oct 3) wrote:
> > why is this on the heap?
> > 
> > Also, why the api changin in GetCommandLinePepperFlash()? Couldn't you just
> pass
> > flash_versions instead of plugins to the old GetCommandLinePepperFlash()?
> 
> FWIW, since the other functions take a PepperPluginInfo*, I like the
continuity
> here, unless we have them all take the flash_versions list.

I like CLs that do one thing per CL: Either change behavior, or refactor code
and don't change behavior. Since this CL says it's a "change behavior" CL I'd
prefer if it was limited to doing the behavior change it's advertised to do in
the simplest way possible :-)

[Greg K, 2016-09-27 21:28:29]

On 2016/09/27 21:13:40, Nico (mostly away until Oct 3) wrote:
>
https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
> File chrome/common/chrome_content_client.cc (right):
> 
>
https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
> chrome/common/chrome_content_client.cc:494:
> std::unique_ptr<content::PepperPluginInfo> command_line_flash(
> On 2016/09/27 21:12:29, Greg K wrote:
> > On 2016/09/27 21:09:14, Nico (mostly away until Oct 3) wrote:
> > > why is this on the heap?
> > > 
> > > Also, why the api changin in GetCommandLinePepperFlash()? Couldn't you
just
> > pass
> > > flash_versions instead of plugins to the old GetCommandLinePepperFlash()?
> > 
> > FWIW, since the other functions take a PepperPluginInfo*, I like the
> continuity
> > here, unless we have them all take the flash_versions list.
> 
> I like CLs that do one thing per CL: Either change behavior, or refactor code
> and don't change behavior. Since this CL says it's a "change behavior" CL I'd
> prefer if it was limited to doing the behavior change it's advertised to do in
> the simplest way possible :-)

Ok, however it lands, I do think it's good to move this code to have the
functions take a uniform argument, either in this CL or in another.

Cheers,

Greg

[waffles, 2016-09-27 21:44:53]

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:494:
std::unique_ptr<content::PepperPluginInfo> command_line_flash(
On 2016/09/27 21:13:40, Nico (mostly away until Oct 3) wrote:
> On 2016/09/27 21:12:29, Greg K wrote:
> > On 2016/09/27 21:09:14, Nico (mostly away until Oct 3) wrote:
> > > why is this on the heap?
> > > 
> > > Also, why the api changin in GetCommandLinePepperFlash()? Couldn't you
just
> > pass
> > > flash_versions instead of plugins to the old GetCommandLinePepperFlash()?
> > 
> > FWIW, since the other functions take a PepperPluginInfo*, I like the
> continuity
> > here, unless we have them all take the flash_versions list.
> 
> I like CLs that do one thing per CL: Either change behavior, or refactor code
> and don't change behavior. Since this CL says it's a "change behavior" CL I'd
> prefer if it was limited to doing the behavior change it's advertised to do in
> the simplest way possible :-)

Passing flash_versions was my first thought, but since it's a ScopedVector
instead of a std::vector, I still needed to change the signature of that
function; while I was there it started to bother me that the signature of the
function differed from GetComponentUpdatedPepperFlash and GetSystemPepperFlash,
thus the alteration.

I think it's a small refactoring, but I can undo it or split it into a separate
CL if you insist.

Re: heap, it feels strange to me to use ScopedVector with objects not in the
heap, although I agree that the entire ScopedVector here could be rethought.
That seemed like a much larger refactor, so I landed on this middle ground.

[Nico, 2016-09-27 21:46:25]

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2370683003/diff/70001/chrome/common/chrome_co...
chrome/common/chrome_content_client.cc:494:
std::unique_ptr<content::PepperPluginInfo> command_line_flash(
On 2016/09/27 21:44:52, waffles wrote:
> On 2016/09/27 21:13:40, Nico (mostly away until Oct 3) wrote:
> > On 2016/09/27 21:12:29, Greg K wrote:
> > > On 2016/09/27 21:09:14, Nico (mostly away until Oct 3) wrote:
> > > > why is this on the heap?
> > > > 
> > > > Also, why the api changin in GetCommandLinePepperFlash()? Couldn't you
> just
> > > pass
> > > > flash_versions instead of plugins to the old
GetCommandLinePepperFlash()?
> > > 
> > > FWIW, since the other functions take a PepperPluginInfo*, I like the
> > continuity
> > > here, unless we have them all take the flash_versions list.
> > 
> > I like CLs that do one thing per CL: Either change behavior, or refactor
code
> > and don't change behavior. Since this CL says it's a "change behavior" CL
I'd
> > prefer if it was limited to doing the behavior change it's advertised to do
in
> > the simplest way possible :-)
> 
> Passing flash_versions was my first thought, but since it's a ScopedVector
> instead of a std::vector, I still needed to change the signature of that
> function; while I was there it started to bother me that the signature of the
> function differed from GetComponentUpdatedPepperFlash and
GetSystemPepperFlash,
> thus the alteration.
> 
> I think it's a small refactoring, but I can undo it or split it into a
separate
> CL if you insist.
> 
> Re: heap, it feels strange to me to use ScopedVector with objects not in the
> heap, although I agree that the entire ScopedVector here could be rethought.
> That seemed like a much larger refactor, so I landed on this middle ground.

Ah, I had missed that flash_versions is a ScopedVector, my bad, sorry :-/ lgtm
then.

[waffles, 2016-09-27 22:18:13]

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

[commit-bot: I haz the power, 2016-09-27 22:18:36]

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-09-27 22:57:31]

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

[commit-bot: I haz the power, 2016-09-27 22:57:32]

Dry run: This issue passed the CQ dry run.

[waffles, 2016-09-30 20:19:23]

Will, would you like me to hold off submitting until you review, or should I
just go ahead?

[Will Harris, 2016-10-01 09:04:30]

On 2016/09/30 20:19:23, waffles wrote:
> Will, would you like me to hold off submitting until you review, or should I
> just go ahead?

lgtm

[waffles, 2016-10-02 13:59:58]

The CQ bit was checked by waffles@chromium.org

[commit-bot: I haz the power, 2016-10-02 14:00:04]

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-10-02 14:45:30]

Committed patchset #2 (id:70001)

[commit-bot: I haz the power, 2016-10-02 14:47:31]

Description was changed from

==========
Don't add fake flash when command-line flash exists.

BUG=650442
==========

to

==========
Don't add fake flash when command-line flash exists.

BUG=650442

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

[commit-bot: I haz the power, 2016-10-02 14:47:32]

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