|
|
Chromium Code Reviews
DescriptionIn aura mus, WindowManagerClient::AddAccelerator() should take an array.
This is a follow up for https://codereview.chromium.org/2520093003/
BUG=671229, 632049
Committed: https://crrev.com/68c719cc1ac847685d1a781ea00ef2b50761ad55
Cr-Commit-Position: refs/heads/master@{#436424}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove unused include. #
Messages
Total messages: 32 (22 generated)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Change AddAccelerator -> AddAccelerators for Aura mus. BUG=671229 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. BUG=671229 ==========
Description was changed from ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. BUG=671229 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is the follow up change on https://bugs.chromium.org/p/chromium/issues/detail?id=632049. BUG=671229 ==========
Description was changed from ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is the follow up change on https://bugs.chromium.org/p/chromium/issues/detail?id=632049. BUG=671229 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is the follow up change based on https://bugs.chromium.org/p/chromium/issues/detail?id=632049. BUG=671229 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanhph@chromium.org changed reviewers: + mfomitchev@google.com, sky@chromium.org
mfomitchev@chromium.org changed reviewers: + mfomitchev@chromium.org
LGTM with nits I think it's better to reference the CL, not the bug in the CL description, i.e.: "This is a follow for https://codereview.chromium.org/2520093003" If you want to reference the bug as well, you can add it under "BUG=". You can have multiple bug numbers there, separated by commas. https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... File ui/aura/mus/window_manager_delegate.h (right): https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... ui/aura/mus/window_manager_delegate.h:17: #include "services/ui/public/interfaces/event_matcher.mojom.h" no longer needed I think?
Description was changed from ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is the follow up change based on https://bugs.chromium.org/p/chromium/issues/detail?id=632049. BUG=671229 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow for https://bugs.chromium.org/p/chromium/issues/detail?id=632049. BUG=671229,632049 ==========
On 2016/12/05 20:08:32, mfomitchev wrote: > LGTM with nits > > I think it's better to reference the CL, not the bug in the CL description, > i.e.: > > "This is a follow for https://codereview.chromium.org/2520093003%22 > > If you want to reference the bug as well, you can add it under "BUG=". You can > have multiple bug numbers there, separated by commas. Thanks Mikhail. I added another bug number and update text in the description.
thanhph@chromium.org changed reviewers: - mfomitchev@google.com
https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... File ui/aura/mus/window_manager_delegate.h (right): https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... ui/aura/mus/window_manager_delegate.h:17: #include "services/ui/public/interfaces/event_matcher.mojom.h" On 2016/12/05 20:08:32, mfomitchev wrote: > no longer needed I think? Done, thanks!
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/05 20:19:45, thanhph wrote: > https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... > File ui/aura/mus/window_manager_delegate.h (right): > > https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... > ui/aura/mus/window_manager_delegate.h:17: #include > "services/ui/public/interfaces/event_matcher.mojom.h" > On 2016/12/05 20:08:32, mfomitchev wrote: > > no longer needed I think? > > Done, thanks! You may have missed my comment regarding CL description.
Description was changed from ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow for https://bugs.chromium.org/p/chromium/issues/detail?id=632049. BUG=671229,632049 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow for https://codereview.chromium.org/2520093003/ BUG=671229,632049 ==========
Description was changed from ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow for https://codereview.chromium.org/2520093003/ BUG=671229,632049 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow up for https://codereview.chromium.org/2520093003/ BUG=671229,632049 ==========
On 2016/12/05 20:20:43, mfomitchev wrote: > On 2016/12/05 20:19:45, thanhph wrote: > > > https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... > > File ui/aura/mus/window_manager_delegate.h (right): > > > > > https://codereview.chromium.org/2552783002/diff/1/ui/aura/mus/window_manager_... > > ui/aura/mus/window_manager_delegate.h:17: #include > > "services/ui/public/interfaces/event_matcher.mojom.h" > > On 2016/12/05 20:08:32, mfomitchev wrote: > > > no longer needed I think? > > > > Done, thanks! > > You may have missed my comment regarding CL description. Done, I updated the description to add the cl link https://codereview.chromium.org/2520093003/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by thanhph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org Link to the patchset: https://codereview.chromium.org/2552783002/#ps20001 (title: "Remove unused include.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480976192542370,
"parent_rev": "7ff9b542eb8c5a1dfcb32d0a9adbe8796a57757e", "commit_rev":
"923cd9144e9d1c859729f575abbe45061abed380"}
Message was sent while issue was closed.
Description was changed from ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow up for https://codereview.chromium.org/2520093003/ BUG=671229,632049 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow up for https://codereview.chromium.org/2520093003/ BUG=671229,632049 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow up for https://codereview.chromium.org/2520093003/ BUG=671229,632049 ========== to ========== In aura mus, WindowManagerClient::AddAccelerator() should take an array. This is a follow up for https://codereview.chromium.org/2520093003/ BUG=671229,632049 Committed: https://crrev.com/68c719cc1ac847685d1a781ea00ef2b50761ad55 Cr-Commit-Position: refs/heads/master@{#436424} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/68c719cc1ac847685d1a781ea00ef2b50761ad55 Cr-Commit-Position: refs/heads/master@{#436424} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
