Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc

Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc
For Windows component builds

BUG=686608
Review-Url: https://codereview.chromium.org/2887033003
Cr-Original-Commit-Position: refs/heads/master@{#473494}
Committed: https://chromium.googlesource.com/chromium/src/+/5cca5ec7822a8d5594a6711b6040c90430b1772e
Review-Url: https://codereview.chromium.org/2887033003
Cr-Commit-Position: refs/heads/master@{#473517}
Committed: https://chromium.googlesource.com/chromium/src/+/722473f6b681bad4dd6dd5897d281339c81efe93

[dvallet, 2017-05-17 08:02:45]

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

[commit-bot: I haz the power, 2017-05-17 08:03:34]

Dry run: CQ is trying da patch.

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

[dvallet, 2017-05-17 08:03:57]

dvallet@chromium.org changed reviewers:
+ alexclarke@chromium.org, skyostil@chromium.org

[dvallet, 2017-05-17 08:03:58]

WDYT?
This is one way I thought of cleaning this up a bit.

[commit-bot: I haz the power, 2017-05-17 08:24:37]

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

[commit-bot: I haz the power, 2017-05-17 08:24:38]

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

[alex clarke (OOO till 29th), 2017-05-17 08:45:02]

I think I prefer this, Sami WDYT?  Should we go further and do this to other
files too?

https://codereview.chromium.org/2887033003/diff/1/headless/app/headless_shell.h
File headless/app/headless_shell.h (right):

https://codereview.chromium.org/2887033003/diff/1/headless/app/headless_shell...
headless/app/headless_shell.h:43: virtual void DevToolsTargetReady() override;
You shouldn't need both virtual and override. In fact it looks like this is
breaking the compile for some platforms.

[Sami, 2017-05-17 13:11:05]

Yeah this seems like a clearer way to do things. Not sure if we could split the
rest of the ifdefs out in a similar way, but this is a good start.

https://codereview.chromium.org/2887033003/diff/1/headless/app/headless_shell...
File headless/app/headless_shell_win.cc (right):

https://codereview.chromium.org/2887033003/diff/1/headless/app/headless_shell...
headless/app/headless_shell_win.cc:5: #include <memory>
Do you need all these includes?

[dvallet, 2017-05-19 04:59:16]

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

[commit-bot: I haz the power, 2017-05-19 04:59:56]

Dry run: CQ is trying da patch.

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

[dvallet, 2017-05-19 05:17:58]

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

[commit-bot: I haz the power, 2017-05-19 05:18:23]

Dry run: CQ is trying da patch.

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

[dvallet, 2017-05-19 05:20:53]

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

[commit-bot: I haz the power, 2017-05-19 05:21:03]

Dry run: CQ is trying da patch.

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

[dvallet, 2017-05-19 05:26:41]

Description was changed from

==========
Add specific headless_shell_win.cc file for Windows component build

BUG=686608
==========

to

==========
Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc
For Windows component builds

BUG=686608
==========

[commit-bot: I haz the power, 2017-05-19 05:42:44]

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

[commit-bot: I haz the power, 2017-05-19 05:42:45]

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

[dvallet, 2017-05-19 05:48:58]

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

[commit-bot: I haz the power, 2017-05-19 05:49:20]

Dry run: CQ is trying da patch.

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

[dvallet, 2017-05-19 05:52:53]

PTAL. Added also headless_content_main_delegate, although this one is not as
clean.

[commit-bot: I haz the power, 2017-05-19 08:13:40]

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

[commit-bot: I haz the power, 2017-05-19 08:13:41]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-05-19 08:55:07]

lgtm

[Sami, 2017-05-19 13:59:48]

lgtm, thanks!

[dvallet, 2017-05-22 00:03:19]

The CQ bit was checked by dvallet@chromium.org

[commit-bot: I haz the power, 2017-05-22 00:03: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, 2017-05-22 00:05:14]

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

[commit-bot: I haz the power, 2017-05-22 00:05:15]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dvallet, 2017-05-22 00:32:29]

The CQ bit was checked by dvallet@chromium.org

[dvallet, 2017-05-22 00:32:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2887033003/#ps100001
(title: "Updated upstream")

[commit-bot: I haz the power, 2017-05-22 00:32:35]

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, 2017-05-22 02:36:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495413149100200,
"parent_rev": "d39dae4faddc9927038ccccb4d7ec37a3fd87937", "commit_rev":
"5cca5ec7822a8d5594a6711b6040c90430b1772e"}

[commit-bot: I haz the power, 2017-05-22 02:36:27]

Description was changed from

==========
Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc
For Windows component builds

BUG=686608
==========

to

==========
Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc
For Windows component builds

BUG=686608

Review-Url: https://codereview.chromium.org/2887033003
Cr-Commit-Position: refs/heads/master@{#473494}
Committed:
https://chromium.googlesource.com/chromium/src/+/5cca5ec7822a8d5594a6711b6040...
==========

[commit-bot: I haz the power, 2017-05-22 02:36:28]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/5cca5ec7822a8d5594a6711b6040...

[PhistucK, 2017-05-22 06:28:38]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[PhistucK, 2017-05-22 06:28:39]

https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_...
File headless/lib/headless_content_main_delegate.cc (left):

https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_...
headless/lib/headless_content_main_delegate.cc:184: // chrashpad is already
enabled.
s/chrashpad/crashpad

[dvallet, 2017-05-22 06:33:44]

On 2017/05/22 at 06:28:39, phistuck wrote:
>
https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_...
> File headless/lib/headless_content_main_delegate.cc (left):
> 
>
https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_...
> headless/lib/headless_content_main_delegate.cc:184: // chrashpad is already
enabled.
> s/chrashpad/crashpad

Thanks! Sending fix

[dvallet, 2017-05-22 06:34:46]

The CQ bit was checked by dvallet@chromium.org

[dvallet, 2017-05-22 06:34:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2887033003/#ps120001
(title: "fix comment typo")

[commit-bot: I haz the power, 2017-05-22 06:35:00]

CQ is trying da patch.

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

[PhistucK, 2017-05-22 06:37:44]

I am not sure it is fine to re-use a change list for multiple commits...

[dvallet, 2017-05-22 06:40:35]

I've never done it, since it's a comment typo I thought of giving it a shot
:)

On Mon, 22 May 2017 at 16:37 <phistuck@gmail.com> wrote:

> I am not sure it is fine to re-use a change list for multiple commits...
>
> https://codereview.chromium.org/2887033003/
>

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

[commit-bot: I haz the power, 2017-05-22 07:13:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495434886249570,
"parent_rev": "7553e311e032564e696c962c1896b07cc19b226f", "commit_rev":
"722473f6b681bad4dd6dd5897d281339c81efe93"}

[commit-bot: I haz the power, 2017-05-22 07:13:21]

Description was changed from

==========
Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc
For Windows component builds

BUG=686608

Review-Url: https://codereview.chromium.org/2887033003
Cr-Commit-Position: refs/heads/master@{#473494}
Committed:
https://chromium.googlesource.com/chromium/src/+/5cca5ec7822a8d5594a6711b6040...
==========

to

==========
Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc
For Windows component builds

BUG=686608

Review-Url: https://codereview.chromium.org/2887033003
Cr-Original-Commit-Position: refs/heads/master@{#473494}
Committed:
https://chromium.googlesource.com/chromium/src/+/5cca5ec7822a8d5594a6711b6040...
Review-Url: https://codereview.chromium.org/2887033003
Cr-Commit-Position: refs/heads/master@{#473517}
Committed:
https://chromium.googlesource.com/chromium/src/+/722473f6b681bad4dd6dd5897d28...
==========

[commit-bot: I haz the power, 2017-05-22 07:13:22]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/722473f6b681bad4dd6dd5897d28...

[Lei Zhang, 2017-08-21 18:30:39]

FTR, patch set 6 landed as r473494 and patch set 7 landed as r473517.

This CL is really confusing because it got reused to fix a typo. I was expecting
to see something the size of patch set 6 when I opened this CL, but instead I
saw a 1 line change. I would tell you not to do this, but we have moved on to
PolyGerrit and I imagine it is more strict about not re-using CLs.

[dvallet, 2017-08-21 22:43:10]

Yup, shouldn't have done that.
To be honest, I just did it to try if it was possible, hopefully gerrit
doesn't allow it :)

On Tue, 22 Aug 2017 at 04:30 <thestig@chromium.org> wrote:

> FTR, patch set 6 landed as r473494 and patch set 7 landed as r473517.
>
> This CL is really confusing because it got reused to fix a typo. I was
> expecting
> to see something the size of patch set 6 when I opened this CL, but
> instead I
> saw a 1 line change. I would tell you not to do this, but we have moved on
> to
> PolyGerrit and I imagine it is more strict about not re-using CLs.
>
> https://codereview.chromium.org/2887033003/
>

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