Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538263005:
Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver (Closed)
|
Created:
4 years ago by Yusuke Sato Modified:
4 years ago Reviewers:
hidehiko CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver
It is slightly weird for ArcServiceManager to handle (only) one of the
ArcService instances it has in a special way with inheritance, and this
CL fixes that. ArcServiceManager looks more neutral now and no longer
has to #include arc_intent_helper_observer.h on the header side. This
removes all virtual member functions from ArcServiceManager too.
Instead, do the same thing with a pair of member variable and its getter
function, which is more consistent with the existing ArcServiceManager
code. All the implementation details go to the .cc side.
This is a follow-up CL for
https://codereview.chromium.org/2487623002/.
BUG=672840
TEST=try
Committed: https://crrev.com/7daca36ac601b5c720b71c1bd894d30d8b565393
Cr-Commit-Position: refs/heads/master@{#438718}
Patch Set 1 #Patch Set 2 : review #
Total comments: 5
Patch Set 3 : rebase, no code change #Patch Set 4 : Add back DCHECK #
Total comments: 2
Patch Set 5 : Address comment #
Messages
Total messages: 36 (26 generated)
The CQ bit was checked by yusukes@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 ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ==========
The CQ bit was checked by yusukes@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes this. ArcServiceManager looks more neutral now. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ==========
Description was changed from ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes this. ArcServiceManager looks more neutral now. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ==========
Description was changed from ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ==========
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
pre-drive-by comments. https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.cc:46: for (auto& observer : manager_->observer_list_) In C++, it is not allowed to access a private member of the enclosing class, IIUC? So, maybe it is necessary to expose the observer_list_ via public member, or to add "friend"? https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:85: class IntentHelperObserverImpl; // implemented in arc_service_manager.cc. This can be just a standalone class in anonymous namespace in .cc file, IIUC. Could you also read my comment in .cc file for member accessibility?
https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.cc:46: for (auto& observer : manager_->observer_list_) On 2016/12/14 09:06:08, hidehiko wrote: > In C++, it is not allowed to access a private member of the enclosing class, > IIUC? > So, maybe it is necessary to expose the observer_list_ via public member, or to > add "friend"? On, wait. My knowledge was old. I've just learnt C++11 allows it. http://en.cppreference.com/w/cpp/language/nested_types Sorry for wrong comment.
The CQ bit was checked by yusukes@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...
The CQ bit was checked by yusukes@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 ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ==========
Thanks for the review. PTAL. https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:85: class IntentHelperObserverImpl; // implemented in arc_service_manager.cc. On 2016/12/14 09:06:08, hidehiko wrote: > This can be just a standalone class in anonymous namespace in .cc file, IIUC. > Could you also read my comment in .cc file for member accessibility? Based on your second comment on the .cc side, I guess I can keep this as-is to keep intent_helper_observer_ private. Please let me know if I'm missing something.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
LGTM! https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/2538263005/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:85: class IntentHelperObserverImpl; // implemented in arc_service_manager.cc. On 2016/12/15 00:52:33, Yusuke Sato (ooo Dec 16 to 31) wrote: > On 2016/12/14 09:06:08, hidehiko wrote: > > This can be just a standalone class in anonymous namespace in .cc file, IIUC. > > Could you also read my comment in .cc file for member accessibility? > > Based on your second comment on the .cc side, I guess I can keep this as-is to > keep intent_helper_observer_ private. Please let me know if I'm missing > something. Yes, I'm fine to skip this as well. https://codereview.chromium.org/2538263005/diff/60001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2538263005/diff/60001/components/arc/arc_serv... components/arc/arc_service_manager.cc:9: #include "base/memory/ptr_util.h" As you're here, could you add #include "base/logging.h" ?
Description was changed from ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=None TEST=try ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=672840 TEST=try ==========
https://codereview.chromium.org/2538263005/diff/60001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/2538263005/diff/60001/components/arc/arc_serv... components/arc/arc_service_manager.cc:9: #include "base/memory/ptr_util.h" On 2016/12/15 00:58:49, hidehiko wrote: > As you're here, could you add #include "base/logging.h" ? Done.
The CQ bit was checked by yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/2538263005/#ps80001 (title: "Address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by yusukes@chromium.org
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": 80001, "attempt_start_ts": 1481769777980840,
"parent_rev": "50150357c8f0f30f50b53a97c35df8363554b0b3", "commit_rev":
"7d6d2d6a4fa0f7f4b05eca55606e64417aecb98d"}
Message was sent while issue was closed.
Description was changed from ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=672840 TEST=try ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=672840 TEST=try Review-Url: https://codereview.chromium.org/2538263005 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=672840 TEST=try Review-Url: https://codereview.chromium.org/2538263005 ========== to ========== Simplify ArcServiceManager by stop deriving it from ArcIntentHelperObserver It is slightly weird for ArcServiceManager to handle (only) one of the ArcService instances it has in a special way with inheritance, and this CL fixes that. ArcServiceManager looks more neutral now and no longer has to #include arc_intent_helper_observer.h on the header side. This removes all virtual member functions from ArcServiceManager too. Instead, do the same thing with a pair of member variable and its getter function, which is more consistent with the existing ArcServiceManager code. All the implementation details go to the .cc side. This is a follow-up CL for https://codereview.chromium.org/2487623002/. BUG=672840 TEST=try Committed: https://crrev.com/7daca36ac601b5c720b71c1bd894d30d8b565393 Cr-Commit-Position: refs/heads/master@{#438718} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7daca36ac601b5c720b71c1bd894d30d8b565393 Cr-Commit-Position: refs/heads/master@{#438718} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
