|
|
Created:
3 years, 8 months ago by lpromero Modified:
3 years, 8 months ago Reviewers:
marq (ping after 24h) CC:
chromium-reviews, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd protocol APIs to CommandDispatcher.
BUG=none
R=marq@chromium.org
Review-Url: https://codereview.chromium.org/2785153002
Cr-Commit-Position: refs/heads/master@{#461730}
Committed: https://chromium.googlesource.com/chromium/src/+/1a91ddcc16f99456e543c79d0406d5f91fdce53d
Patch Set 1 #
Total comments: 8
Patch Set 2 : Feedback #
Messages
Total messages: 19 (11 generated)
The CQ bit was checked by lpromero@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...
https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/commands/command_dispatcher.mm (right): https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:30: unsigned int outCount; Prefer method_count, here and below. https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:37: } What about protocols the protocol adheres to? (with the wrinkle that we don't want to start dispatching <NSObject>, for example).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/commands/command_dispatcher.mm (right): https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:30: unsigned int outCount; On 2017/03/30 14:03:55, marq wrote: > Prefer method_count, here and below. Would methodCount work? https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:37: } On 2017/03/30 14:03:55, marq wrote: > What about protocols the protocol adheres to? (with the wrinkle that we don't > want to start dispatching <NSObject>, for example). I started this CL to basically raise all these questions. Do we support: - the properties - the class methods - the optional methods - the parent protocol methods? Instead, shouldn't we try to be explicit everywhere we register for method dispatch? The protocol API makes a lot of things implicit and hard to follow. Where do you see us using this CL?
https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/commands/command_dispatcher.mm (right): https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:30: unsigned int outCount; On 2017/03/30 15:00:39, lpromero wrote: > On 2017/03/30 14:03:55, marq wrote: > > Prefer method_count, here and below. > > Would methodCount work? Yes, sorry; I'm working on similar code inside a C function, so I had snake_style in my head. https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:37: } On 2017/03/30 15:00:39, lpromero wrote: > On 2017/03/30 14:03:55, marq wrote: > > What about protocols the protocol adheres to? (with the wrinkle that we don't > > want to start dispatching <NSObject>, for example). > > I started this CL to basically raise all these questions. Do we support: > - the properties > - the class methods > - the optional methods > - the parent protocol methods? > > Instead, shouldn't we try to be explicit everywhere we register for method > dispatch? The protocol API makes a lot of things implicit and hard to follow. > Where do you see us using this CL? Clearly either of these calls are just conveniences to reduce boilerplate, so it's possible that they open a can of worms that's not worth figuring out. On the other hand, we may be able to say that all command protocols are just required methods with no inheritance or anything else. So maybe we start with this as-is and see if it does useful work, and then we can assess if making them handle all of the edge cases is worth the convenience they provide.
The CQ bit was checked by lpromero@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...
Updated. Sounds good to see where this will take us. https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... File ios/shared/chrome/browser/ui/commands/command_dispatcher.mm (right): https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:30: unsigned int outCount; On 2017/03/31 07:43:01, marq wrote: > On 2017/03/30 15:00:39, lpromero wrote: > > On 2017/03/30 14:03:55, marq wrote: > > > Prefer method_count, here and below. > > > > Would methodCount work? > > Yes, sorry; I'm working on similar code inside a C function, so I had > snake_style in my head. Done. https://codereview.chromium.org/2785153002/diff/1/ios/shared/chrome/browser/u... ios/shared/chrome/browser/ui/commands/command_dispatcher.mm:37: } On 2017/03/31 07:43:01, marq wrote: > On 2017/03/30 15:00:39, lpromero wrote: > > On 2017/03/30 14:03:55, marq wrote: > > > What about protocols the protocol adheres to? (with the wrinkle that we > don't > > > want to start dispatching <NSObject>, for example). > > > > I started this CL to basically raise all these questions. Do we support: > > - the properties > > - the class methods > > - the optional methods > > - the parent protocol methods? > > > > Instead, shouldn't we try to be explicit everywhere we register for method > > dispatch? The protocol API makes a lot of things implicit and hard to follow. > > Where do you see us using this CL? > > Clearly either of these calls are just conveniences to reduce boilerplate, so > it's possible that they open a can of worms that's not worth figuring out. On > the other hand, we may be able to say that all command protocols are just > required methods with no inheritance or anything else. So maybe we start with > this as-is and see if it does useful work, and then we can assess if making them > handle all of the edge cases is worth the convenience they provide. Done.
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 lpromero@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": 20001, "attempt_start_ts": 1491320161154070, "parent_rev": "cd324eb7d820806adc5a2830c613d0976ebb73ad", "commit_rev": "1a91ddcc16f99456e543c79d0406d5f91fdce53d"}
Message was sent while issue was closed.
Description was changed from ========== Add protocol APIs to CommandDispatcher. BUG=none R=marq@chromium.org ========== to ========== Add protocol APIs to CommandDispatcher. BUG=none R=marq@chromium.org Review-Url: https://codereview.chromium.org/2785153002 Cr-Commit-Position: refs/heads/master@{#461730} Committed: https://chromium.googlesource.com/chromium/src/+/1a91ddcc16f99456e543c79d0406... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1a91ddcc16f99456e543c79d0406... |