Do not reference non-existant bundled flash on Chrome OS.

Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795, 644400
Committed: https://crrev.com/cf804457306e0facdb59124fa38b5315493e1da7
Cr-Commit-Position: refs/heads/master@{#419513}

[Greg K, 2016-09-13 22:21:10]

kerrnel@chromium.org changed reviewers:
+ ihf@chromium.org, wfh@chromium.org

[Greg K, 2016-09-13 22:21:10]

Please review this CL to fix how Flash is loaded on Chrome OS.

[Will Harris, 2016-09-13 23:30:17]

I am 100% sure that waffles needs to review this, as he has very recently
changed this code, and he is back in the office on 15 Sep so maybe add him then?
Is it more urgent than this?

[ilja, 2016-09-14 03:55:50]

Description was changed from

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795
==========

to

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795
==========

[ilja, 2016-09-14 03:55:50]

ihf@chromium.org changed reviewers:
+ hshi@chromium.org, waffles@chromium.org

[ilja, 2016-09-14 03:58:31]

Adding waffles and hshi. I agree we should coordinate.

I am fine with this change,as it is dead code right now. But I assume it was
written to unify CrOS and Linux, so we should make sure to be all on the same
page.

[hshi1, 2016-09-14 04:05:53]

lgtm

[Greg K, 2016-09-14 16:47:19]

On 2016/09/13 23:30:17, Will Harris wrote:
> I am 100% sure that waffles needs to review this, as he has very recently
> changed this code, and he is back in the office on 15 Sep so maybe add him
then?
> Is it more urgent than this?

waffles actually asked me to take care of this in his absence, but if he's
coming back tomorrow, I will just wait and have him take a look. Thanks for
pointing that out.

- Greg

[ilja, 2016-09-15 01:53:18]

lgtm

I tried to understand the history of this code better. I think the "if
defined(OS_CHROMEOS)" sneaked in over time. But there really never was a bundle
of Flash with Chrome, as Flash on ChromeOS is distributed with the OS.

[waffles, 2016-09-15 18:05:13]

Thanks for doing this!

lgtm to fix the ChromeOS issues. This points out an issue on desktop when the
flag is specified; I have an idea how to fix it below. Either I can do it in a
follow-up CL or you can do it in this one; up to you.

https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:489:
AddPepperFlashFromCommandLine(plugins);
I would like to refactor AddPepperFlashFromCommandLine so that it pushes into
flash_versions below; otherwise, the flag will remain be broken on desktop
(because we'll register both flag flash and fake flash).

[waffles, 2016-09-15 18:07:53]

Description was changed from

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795
==========

to

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795,644400
==========

[Greg K, 2016-09-16 16:58:45]

kerrnel@chromium.org changed reviewers:
+ thakis@chromium.org

[Greg K, 2016-09-16 16:58:45]

thakis@chromium.org: Please review changes in

[Will Harris, 2016-09-16 17:03:35]

I have a small Q

https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:489:
AddPepperFlashFromCommandLine(plugins);
On 2016/09/15 18:05:13, waffles wrote:
> I would like to refactor AddPepperFlashFromCommandLine so that it pushes into
> flash_versions below; otherwise, the flag will remain be broken on desktop
> (because we'll register both flag flash and fake flash).

Fake Flash is only registered on Chrome builds, right? So chromium builds who
use --ppapi-flash-path  to pick up Adobe PPAPI system installers (e.g. on Linux)
will still work?

[waffles, 2016-09-16 17:05:14]

On 2016/09/16 17:03:35, Will Harris wrote:
> I have a small Q
> 
>
https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
> File chrome/common/chrome_content_client.cc (right):
> 
>
https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
> chrome/common/chrome_content_client.cc:489:
> AddPepperFlashFromCommandLine(plugins);
> On 2016/09/15 18:05:13, waffles wrote:
> > I would like to refactor AddPepperFlashFromCommandLine so that it pushes
into
> > flash_versions below; otherwise, the flag will remain be broken on desktop
> > (because we'll register both flag flash and fake flash).
> 
> Fake Flash is only registered on Chrome builds, right? So chromium builds who
> use --ppapi-flash-path  to pick up Adobe PPAPI system installers (e.g. on
Linux)
> will still work?

Right.

[Greg K, 2016-09-19 17:10:19]

On 2016/09/16 17:05:14, waffles wrote:
> On 2016/09/16 17:03:35, Will Harris wrote:
> > I have a small Q
> > 
> >
>
https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
> > File chrome/common/chrome_content_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2340643002/diff/1/chrome/common/chrome_conten...
> > chrome/common/chrome_content_client.cc:489:
> > AddPepperFlashFromCommandLine(plugins);
> > On 2016/09/15 18:05:13, waffles wrote:
> > > I would like to refactor AddPepperFlashFromCommandLine so that it pushes
> into
> > > flash_versions below; otherwise, the flag will remain be broken on desktop
> > > (because we'll register both flag flash and fake flash).
> > 
> > Fake Flash is only registered on Chrome builds, right? So chromium builds
who
> > use --ppapi-flash-path  to pick up Adobe PPAPI system installers (e.g. on
> Linux)
> > will still work?
> 
> Right.

Nice,

Can I please get an OWNERS review for this CL?

Thank you,

- Greg

[Nico, 2016-09-19 17:35:30]

On Mon, Sep 19, 2016 at 1:10 PM, <kerrnel@chromium.org> wrote:

> On 2016/09/16 17:05:14, waffles wrote:
> > On 2016/09/16 17:03:35, Will Harris wrote:
> > > I have a small Q
> > >
> > >
> >
> https://codereview.chromium.org/2340643002/diff/1/chrome/
> common/chrome_content_client.cc
> > > File chrome/common/chrome_content_client.cc (right):
> > >
> > >
> >
> https://codereview.chromium.org/2340643002/diff/1/chrome/
> common/chrome_content_client.cc#newcode489
> > > chrome/common/chrome_content_client.cc:489:
> > > AddPepperFlashFromCommandLine(plugins);
> > > On 2016/09/15 18:05:13, waffles wrote:
> > > > I would like to refactor AddPepperFlashFromCommandLine so that it
> pushes
> > into
> > > > flash_versions below; otherwise, the flag will remain be broken on
> desktop
> > > > (because we'll register both flag flash and fake flash).
> > >
> > > Fake Flash is only registered on Chrome builds, right? So chromium
> builds
> who
> > > use --ppapi-flash-path to pick up Adobe PPAPI system installers (e.g.
> on
> > Linux)
> > > will still work?
> >
> > Right.
>
> Nice,
>
> Can I please get an OWNERS review for this CL?
>

You wrote "thakis@chromium.org: Please review changes in" followed by
nothing.

I guess you want me to look at more than the empty set after all? :-P


>
> Thank you,
>
> - Greg
>
> https://codereview.chromium.org/2340643002/
>

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

[Nico, 2016-09-19 17:36:43]

rs-lgtm based on waffles's review and ihf's notes on the bug

[Greg K, 2016-09-19 17:37:39]

On 2016/09/19 17:35:30, Nico wrote:
> On Mon, Sep 19, 2016 at 1:10 PM, <mailto:kerrnel@chromium.org> wrote:
> 
> > On 2016/09/16 17:05:14, waffles wrote:
> > > On 2016/09/16 17:03:35, Will Harris wrote:
> > > > I have a small Q
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2340643002/diff/1/chrome/
> > common/chrome_content_client.cc
> > > > File chrome/common/chrome_content_client.cc (right):
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2340643002/diff/1/chrome/
> > common/chrome_content_client.cc#newcode489
> > > > chrome/common/chrome_content_client.cc:489:
> > > > AddPepperFlashFromCommandLine(plugins);
> > > > On 2016/09/15 18:05:13, waffles wrote:
> > > > > I would like to refactor AddPepperFlashFromCommandLine so that it
> > pushes
> > > into
> > > > > flash_versions below; otherwise, the flag will remain be broken on
> > desktop
> > > > > (because we'll register both flag flash and fake flash).
> > > >
> > > > Fake Flash is only registered on Chrome builds, right? So chromium
> > builds
> > who
> > > > use --ppapi-flash-path to pick up Adobe PPAPI system installers (e.g.
> > on
> > > Linux)
> > > > will still work?
> > >
> > > Right.
> >
> > Nice,
> >
> > Can I please get an OWNERS review for this CL?
> >
> 
> You wrote mailto:"thakis@chromium.org: Please review changes in" followed by
> nothing.
> 
> I guess you want me to look at more than the empty set after all? :-P
> 

Hah, I messed up because I didn't see the auto-inserted comments. Yes, I would
like a review of both files, thank you. :-)

- Greg

> 
> >
> > Thank you,
> >
> > - Greg
> >
> > https://codereview.chromium.org/2340643002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[Greg K, 2016-09-19 17:37:48]

The CQ bit was checked by kerrnel@chromium.org

[commit-bot: I haz the power, 2016-09-19 17:38:11]

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-19 18:38:33]

Description was changed from

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795,644400
==========

to

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795,644400
==========

[commit-bot: I haz the power, 2016-09-19 18:38:35]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-19 18:40:08]

Description was changed from

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795,644400
==========

to

==========
Do not reference non-existant bundled flash on Chrome OS.

Chrome on Chrome OS is incorrectly building against the Flash player
bundled in third_party/. However, on Chrome OS, flash player is loaded
from the root partition and passed to Chrome via command line. So Chrome
should not be referencing the versions in flapper_version.h. In
addition, this means the bundled flash player loading code is unused.

BUG=644795,644400

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

[commit-bot: I haz the power, 2016-09-19 18:40:09]

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