Stop including flash in the archive on Windows.

Stop including flash in the archive on Windows.

Also fix a stale comment or two.

TEST=https://docs.google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/edit#
BUG=624086
Committed: https://crrev.com/06247e8b22adef234f0c122db635ff607a33c360
Cr-Commit-Position: refs/heads/master@{#414019}

[grt (UTC plus 2), 2016-08-17 07:29:40]

Description was changed from

==========
Stop including flash in the archive on Windows.

BUG=624086
==========

to

==========
Stop including flash in the archive on Windows.

Also fix a stale comment or two.

BUG=624086
==========

[grt (UTC plus 2), 2016-08-17 07:29:40]

grt@chromium.org changed reviewers:
+ laforge@chromium.org

[grt (UTC plus 2), 2016-08-17 07:30:15]

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

[commit-bot: I haz the power, 2016-08-17 07:30:30]

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-08-17 08:24:10]

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

[commit-bot: I haz the power, 2016-08-17 08:24:11]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-08-17 15:29:07]

PTAL. Feel free to check the CQ box if it looks good. Thanks.

[Will Harris, 2016-08-17 16:25:59]

TEST line?

I can do some manual testing, but it would be good for a line for TE to qualify
this. Also, does anything need to be changed in the main BAT spreadsheet?

[waffles, 2016-08-17 17:09:19]

On 2016/08/17 16:25:59, Will Harris wrote:
> TEST line?
> 
> I can do some manual testing, but it would be good for a line for TE to
qualify
> this. Also, does anything need to be changed in the main BAT spreadsheet?

Do not land this change before we remove the code that registers Flash with a
compile-time version in chrome_content_client.cc; I think landing this first
will break Flash for those users (because Chrome will still think it has a
bundled version). Removing that code is blocked on crbug/628320.

[grt (UTC plus 2), 2016-08-17 18:35:27]

On 2016/08/17 17:09:19, waffles wrote:
> On 2016/08/17 16:25:59, Will Harris wrote:
> > TEST line?
> > 
> > I can do some manual testing, but it would be good for a line for TE to
> qualify
> > this. Also, does anything need to be changed in the main BAT spreadsheet?
> 
> Do not land this change before we remove the code that registers Flash with a
> compile-time version in chrome_content_client.cc; I think landing this first
> will break Flash for those users (because Chrome will still think it has a
> bundled version). Removing that code is blocked on crbug/628320.

I had no context for this change, and threw it up on the wall primarily to see
what happened. Glad you saw the thread and chimed in. :-)

[waffles, 2016-08-22 22:57:03]

On 2016/08/17 18:35:27, grt (UTC plus 2) wrote:
> On 2016/08/17 17:09:19, waffles wrote:
> > On 2016/08/17 16:25:59, Will Harris wrote:
> > > TEST line?
> > > 
> > > I can do some manual testing, but it would be good for a line for TE to
> > qualify
> > > this. Also, does anything need to be changed in the main BAT spreadsheet?
> > 
> > Do not land this change before we remove the code that registers Flash with
a
> > compile-time version in chrome_content_client.cc; I think landing this first
> > will break Flash for those users (because Chrome will still think it has a
> > bundled version). Removing that code is blocked on crbug/628320.
> 
> I had no context for this change, and threw it up on the wall primarily to see
> what happened. Glad you saw the thread and chimed in. :-)

lgtm now :)

[grt (UTC plus 2), 2016-08-23 07:11:58]

grt@chromium.org changed reviewers:
+ thakis@chromium.org, waffles@chromium.org

[grt (UTC plus 2), 2016-08-23 07:12:01]

+thakis for chrome/BUILD.gn OWNERS review

waffles: please take note that since the initial patch set, I changed BUILD.gn
so that the flapper binaries are no longer copied into the build directory. It
seems correct to me since they are no longer bundled in the installer. Please
let me know if they still need to be copied for any reason. Thanks.

[waffles, 2016-08-23 16:37:27]

On 2016/08/23 07:12:01, grt (UTC plus 2) wrote:
> +thakis for chrome/BUILD.gn OWNERS review
> 
> waffles: please take note that since the initial patch set, I changed BUILD.gn
> so that the flapper binaries are no longer copied into the build directory. It
> seems correct to me since they are no longer bundled in the installer. Please
> let me know if they still need to be copied for any reason. Thanks.

I think this is OK, since catapult will pick up the binaries from
src/third_party for perf testing.

[Nico, 2016-08-23 17:19:47]

rs-lgtm, I don't know this code well though. (But waffles probably does :-) )

https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn#newcode264
chrome/BUILD.gn:264: # dependencies are on chrome.dll. On Windows, these are not
bundled.
Aren't they not bundled now on Mac as well?

[laforge, 2016-08-23 17:40:40]

On 2016/08/23 17:19:47, Nico wrote:
> rs-lgtm, I don't know this code well though. (But waffles probably does :-) )
> 
> https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn
> File chrome/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn#newcode264
> chrome/BUILD.gn:264: # dependencies are on chrome.dll. On Windows, these are
not
> bundled.
> Aren't they not bundled now on Mac as well?

We aren't going to be bundling for Mac or Linux either, not sure we know what to
edit to remove them properly from those bundles.  Ideally we'd do that in one
CL.

[waffles, 2016-08-23 17:49:10]

On 2016/08/23 17:40:40, laforge wrote:
> On 2016/08/23 17:19:47, Nico wrote:
> > rs-lgtm, I don't know this code well though. (But waffles probably does :-)
)
> > 
> > https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn
> > File chrome/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn#newcode264
> > chrome/BUILD.gn:264: # dependencies are on chrome.dll. On Windows, these are
> not
> > bundled.
> > Aren't they not bundled now on Mac as well?
> 
> We aren't going to be bundling for Mac or Linux either, not sure we know what
to
> edit to remove them properly from those bundles.  Ideally we'd do that in one
> CL.

I recommend leaving Linux to a second CL, as I think un-bundling there might
have higher risk (we have different code paths to register it with Chrome, due
to the different sandboxing model). If unbundling on Linux does cause unforeseen
issues (most likely with tests), it would be nice to handle reverts, fixes, and
merges without requiring us to rerun tedious manual tests on Windows and Mac.
And if not, it's not too painful to have it in two CLs.

I'm all for changing this on Mac in this CL, though.

Re: wfh's comments about a TEST= line. I can clone & update the Flash test case
doc he and I have been using for the post-unbundled world, but I am not sure
about the BAT spreadsheet.

[grt (UTC plus 2), 2016-08-23 20:28:38]

On 2016/08/23 17:49:10, waffles wrote:
> On 2016/08/23 17:40:40, laforge wrote:
> > On 2016/08/23 17:19:47, Nico wrote:
> > > rs-lgtm, I don't know this code well though. (But waffles probably does
:-)
> )
> > > 
> > > https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn
> > > File chrome/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn#newcode264
> > > chrome/BUILD.gn:264: # dependencies are on chrome.dll. On Windows, these
are
> > not
> > > bundled.
> > > Aren't they not bundled now on Mac as well?
> > 
> > We aren't going to be bundling for Mac or Linux either, not sure we know
what
> to
> > edit to remove them properly from those bundles.  Ideally we'd do that in
one
> > CL.
> 
> I recommend leaving Linux to a second CL, as I think un-bundling there might
> have higher risk (we have different code paths to register it with Chrome, due
> to the different sandboxing model). If unbundling on Linux does cause
unforeseen
> issues (most likely with tests), it would be nice to handle reverts, fixes,
and
> merges without requiring us to rerun tedious manual tests on Windows and Mac.
> And if not, it's not too painful to have it in two CLs.
> 
> I'm all for changing this on Mac in this CL, though.

I'm not the folk to change it on Mac in this CL; I have no idea how the bundle
on Mac is created. How about we keep this focused on Winderz and have a separate
CL from a Mac folk for the Mac side of the house?

> Re: wfh's comments about a TEST= line. I can clone & update the Flash test
case
> doc he and I have been using for the post-unbundled world, but I am not sure
> about the BAT spreadsheet.

It will suffice to either have a brief test case, or a pointer to a crbug
issue/doc with more test details.

[laforge, 2016-08-23 21:29:12]

On 2016/08/23 20:28:38, grt (UTC plus 2) wrote:
> On 2016/08/23 17:49:10, waffles wrote:
> > On 2016/08/23 17:40:40, laforge wrote:
> > > On 2016/08/23 17:19:47, Nico wrote:
> > > > rs-lgtm, I don't know this code well though. (But waffles probably does
> :-)
> > )
> > > > 
> > > > https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn
> > > > File chrome/BUILD.gn (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2251923002/diff/20001/chrome/BUILD.gn#newcode264
> > > > chrome/BUILD.gn:264: # dependencies are on chrome.dll. On Windows, these
> are
> > > not
> > > > bundled.
> > > > Aren't they not bundled now on Mac as well?
> > > 
> > > We aren't going to be bundling for Mac or Linux either, not sure we know
> what
> > to
> > > edit to remove them properly from those bundles.  Ideally we'd do that in
> one
> > > CL.
> > 
> > I recommend leaving Linux to a second CL, as I think un-bundling there might
> > have higher risk (we have different code paths to register it with Chrome,
due
> > to the different sandboxing model). If unbundling on Linux does cause
> unforeseen
> > issues (most likely with tests), it would be nice to handle reverts, fixes,
> and
> > merges without requiring us to rerun tedious manual tests on Windows and
Mac.
> > And if not, it's not too painful to have it in two CLs.
> > 
> > I'm all for changing this on Mac in this CL, though.
> 
> I'm not the folk to change it on Mac in this CL; I have no idea how the bundle
> on Mac is created. How about we keep this focused on Winderz and have a
separate
> CL from a Mac folk for the Mac side of the house?

OK, we can do that in a separate CL.

> 
> > Re: wfh's comments about a TEST= line. I can clone & update the Flash test
> case
> > doc he and I have been using for the post-unbundled world, but I am not sure
> > about the BAT spreadsheet.
> 
> It will suffice to either have a brief test case, or a pointer to a crbug
> issue/doc with more test details.

[waffles, 2016-08-23 22:30:26]

Description was changed from

==========
Stop including flash in the archive on Windows.

Also fix a stale comment or two.

BUG=624086
==========

to

==========
Stop including flash in the archive on Windows.

Also fix a stale comment or two.

TEST=https://docs.google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/edit#
BUG=624086
==========

[waffles, 2016-08-23 22:34:58]

> It will suffice to either have a brief test case, or a pointer to a crbug
> issue/doc with more test details.

Updated TEST= line with a test plan.

[grt (UTC plus 2), 2016-08-24 07:13:58]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2016-08-24 07:14:23]

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-08-24 08:33:41]

Description was changed from

==========
Stop including flash in the archive on Windows.

Also fix a stale comment or two.

TEST=https://docs.google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/edit#
BUG=624086
==========

to

==========
Stop including flash in the archive on Windows.

Also fix a stale comment or two.

TEST=https://docs.google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/edit#
BUG=624086
==========

[commit-bot: I haz the power, 2016-08-24 08:33:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-24 08:35:38]

Description was changed from

==========
Stop including flash in the archive on Windows.

Also fix a stale comment or two.

TEST=https://docs.google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/edit#
BUG=624086
==========

to

==========
Stop including flash in the archive on Windows.

Also fix a stale comment or two.

TEST=https://docs.google.com/document/d/1P0WslUXMUO9azLmWZ0AtQ-sGExWbHttr_2gQWDXf3MY/edit#
BUG=624086

Committed: https://crrev.com/06247e8b22adef234f0c122db635ff607a33c360
Cr-Commit-Position: refs/heads/master@{#414019}
==========

[commit-bot: I haz the power, 2016-08-24 08:35:39]

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