Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2887033003: Add specific headless_shell_win.cc and headless_content_main_delegate_win.cc (Closed)

Created:
1 year, 5 months ago by dvallet
Modified:
1 year, 1 month ago
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Added headless_content_main_delegate_win #

Patch Set 3 : Fixed weird Windows formatting #

Patch Set 4 : fixed nit #

Patch Set 5 : Remove virtual (was added back by mistake) #

Patch Set 6 : Updated upstream #

Total comments: 1

Patch Set 7 : fix comment typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M headless/lib/headless_content_main_delegate.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 47 (29 generated)
dvallet
WDYT? This is one way I thought of cleaning this up a bit.
1 year, 5 months ago (2017-05-17 08:03:58 UTC) #4
alex clarke (OOO till 29th)
I think I prefer this, Sami WDYT? Should we go further and do this to ...
1 year, 5 months ago (2017-05-17 08:45:02 UTC) #7
Sami
Yeah this seems like a clearer way to do things. Not sure if we could ...
1 year, 5 months ago (2017-05-17 13:11:05 UTC) #8
dvallet
PTAL. Added also headless_content_main_delegate, although this one is not as clean.
1 year, 5 months ago (2017-05-19 05:52:53 UTC) #20
alex clarke (OOO till 29th)
lgtm
1 year, 5 months ago (2017-05-19 08:55:07 UTC) #23
Sami
lgtm, thanks!
1 year, 5 months ago (2017-05-19 13:59:48 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2887033003/80001
1 year, 5 months ago (2017-05-22 00:03:23 UTC) #26
commit-bot: I haz the power
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/215671) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
1 year, 5 months ago (2017-05-22 00:05:15 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2887033003/100001
1 year, 5 months ago (2017-05-22 00:32:35 UTC) #31
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5cca5ec7822a8d5594a6711b6040c90430b1772e
1 year, 5 months ago (2017-05-22 02:36:28 UTC) #34
PhistucK
https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_content_main_delegate.cc File headless/lib/headless_content_main_delegate.cc (left): https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_content_main_delegate.cc#oldcode184 headless/lib/headless_content_main_delegate.cc:184: // chrashpad is already enabled. s/chrashpad/crashpad
1 year, 5 months ago (2017-05-22 06:28:39 UTC) #36
dvallet
On 2017/05/22 at 06:28:39, phistuck wrote: > https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_content_main_delegate.cc > File headless/lib/headless_content_main_delegate.cc (left): > > https://codereview.chromium.org/2887033003/diff/100001/headless/lib/headless_content_main_delegate.cc#oldcode184 ...
1 year, 5 months ago (2017-05-22 06:33:44 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2887033003/120001
1 year, 5 months ago (2017-05-22 06:35:00 UTC) #40
PhistucK
I am not sure it is fine to re-use a change list for multiple commits...
1 year, 5 months ago (2017-05-22 06:37:44 UTC) #41
dvallet
I've never done it, since it's a comment typo I thought of giving it a ...
1 year, 5 months ago (2017-05-22 06:40:35 UTC) #42
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/722473f6b681bad4dd6dd5897d281339c81efe93
1 year, 5 months ago (2017-05-22 07:13:22 UTC) #45
Lei Zhang
FTR, patch set 6 landed as r473494 and patch set 7 landed as r473517. This ...
1 year, 1 month ago (2017-08-21 18:30:39 UTC) #46
dvallet
1 year, 1 month ago (2017-08-21 22:43:10 UTC) #47
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698