Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1476143002:
Fix Clang-plugin style error in ChildBrokerHost (Closed)
|
Created:
5 years ago by hans Modified:
5 years ago Reviewers:
jam CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Clang-plugin style error in ChildBrokerHost
BUG=82385
R=jam@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/d1c5b560af04db599b4b2a62a32366f8b2a80154
Patch Set 1 #
Total comments: 1
Patch Set 2 : Drop virtual #Messages
Total messages: 23 (12 generated)
Description was changed from ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 TBR=jam ========== to ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 ==========
Please take a look.
Description was changed from ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 ========== to ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 TBR=jam ==========
Since this is a build fix, I'll go ahead and TBR it. I'll follow up if you have any comments.
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1476143002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1476143002/1
lgtm https://codereview.chromium.org/1476143002/diff/1/mojo/edk/system/child_broke... File mojo/edk/system/child_broker_host.h (right): https://codereview.chromium.org/1476143002/diff/1/mojo/edk/system/child_broke... mojo/edk/system/child_broker_host.h:40: virtual ~ChildBrokerHost(); no need for virtual actually
Description was changed from ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 TBR=jam ========== to ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 TBR=jam NOTRY=true ==========
The CQ bit was checked by hans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org Link to the patchset: https://codereview.chromium.org/1476143002/#ps20001 (title: "Drop virtual")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1476143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1476143002/20001
The CQ bit was unchecked by jam@chromium.org
Description was changed from ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 TBR=jam NOTRY=true ========== to ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 TBR=jam ==========
Description was changed from ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 TBR=jam ========== to ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 ==========
removing NOTRY=true. please don't use that again. see https://www.chromium.org/developers/testing/commit-queue for when it's ok (i.e. reverts). anything else should go through the CQ, even if clang bot is red (i.e. since it's not on the cq, it's ok for it to have fixes like this be delayed while they go through the cq).
The CQ bit was checked by jam@chromium.org
On 2015/11/25 23:43:45, jam wrote: > removing NOTRY=true. please don't use that again. see > https://www.chromium.org/developers/testing/commit-queue for when it's ok (i.e. > reverts). anything else should go through the CQ, even if clang bot is red (i.e. > since it's not on the cq, it's ok for it to have fixes like this be delayed > while they go through the cq). Our whole waterfall has been red for 4 hours due to your change. I figured this is a build fix, and doing this was nicer than reverting your change.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1476143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1476143002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 ========== to ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 R=jam@chromium.org Committed: https://crrev.com/d1c5b560af04db599b4b2a62a32366f8b2a80154 Cr-Commit-Position: refs/heads/master@{#361763} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d1c5b560af04db599b4b2a62a32366f8b2a80154 Cr-Commit-Position: refs/heads/master@{#361763}
Message was sent while issue was closed.
Description was changed from ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 R=jam@chromium.org Committed: https://crrev.com/d1c5b560af04db599b4b2a62a32366f8b2a80154 Cr-Commit-Position: refs/heads/master@{#361763} ========== to ========== Fix Clang-plugin style error in ChildBrokerHost BUG=82385 R=jam@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/d1c5b560af04db599b4b2a62a323... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as d1c5b560af04db599b4b2a62a32366f8b2a80154 (presubmit successful).
Message was sent while issue was closed.
On 2015/11/25 23:46:14, hans wrote: > On 2015/11/25 23:43:45, jam wrote: > > removing NOTRY=true. please don't use that again. see > > https://www.chromium.org/developers/testing/commit-queue for when it's ok > (i.e. > > reverts). anything else should go through the CQ, even if clang bot is red > (i.e. > > since it's not on the cq, it's ok for it to have fixes like this be delayed > > while they go through the cq). > > Our whole waterfall has been red for 4 hours due to your change. Yeah, sorry about that. That's the cost of having a builder that's not on the CQ. > > I figured this is a build fix, and doing this was nicer than reverting your > change. Note: we don't revert changes for builders that aren't on the CQ immediately: i.e. we reach out to devs to fix async. see https://www.chromium.org/developers/tree-sheriffs |
