|
|
DescriptionRestrict EventRouter::Get/Set-RegisteredEvents.
The code is going to be changed in a subsequent CL to allow extension
service worker events, it's better if we restrict GetRegisteredEvents and
SetRegisteredEvents as we don't really need them outside of EventRouter.
Expose simpler HasRegisteredEvents and test specific
ClearRegisteredEventsForTest instead.
BUG=721147
Test=No visible change expected.
Review-Url: https://codereview.chromium.org/2879673002
Cr-Commit-Position: refs/heads/master@{#472151}
Committed: https://chromium.googlesource.com/chromium/src/+/ac968918d5b2b6baf9fe987cb0094aa9a73177c0
Patch Set 1 #
Total comments: 5
Patch Set 2 : address comments #
Messages
Total messages: 29 (21 generated)
The CQ bit was checked by lazyboy@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 ========== Restrict EventRouter::Get/Set-RegisteredEvents. I'm going to change the code a bit in a subsequent CL, it's better if we retrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific SetRegisteredEvents instead. BUG=None ========== to ========== Restrict EventRouter::Get/Set-RegisteredEvents. I'm going to change the code a bit in a subsequent CL, it's better if we retrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific SetRegisteredEvents instead. BUG=721147 Test=No visible change expected. ==========
lazyboy@chromium.org changed reviewers: + karandeepb@chromium.org
https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... File extensions/browser/event_router.cc (right): https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... extensions/browser/event_router.cc:736: void EventRouter::SetRegisteredEvents(const std::string& extension_id, note: This block is moved from line 345.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am not familiar with EventRouters but seems reasonable. LGTM. Also, the CL description needs updating. https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... File extensions/browser/event_router.h (right): https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... extensions/browser/event_router.h:185: bool HasRegisteredEvents(const ExtensionId& extension_id) const; The other functions don't use the ExtensionId typedef. Would be better if we remain consistent with surrounding code by changing this to string, or the rest to ExtensionId. https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... extensions/browser/event_router.h:188: void ClearRegisteredEventsForTest(const ExtensionId& extension_id); Optional nit: Both of these can be inlined.
Description was changed from ========== Restrict EventRouter::Get/Set-RegisteredEvents. I'm going to change the code a bit in a subsequent CL, it's better if we retrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific SetRegisteredEvents instead. BUG=721147 Test=No visible change expected. ========== to ========== Restrict EventRouter::Get/Set-RegisteredEvents. The code is going to be changed in a subsequent CL to allow extension service worker events, it's better if we retrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific ClearRegisteredEventsForTest instead. BUG=721147 Test=No visible change expected. ==========
Description was changed from ========== Restrict EventRouter::Get/Set-RegisteredEvents. The code is going to be changed in a subsequent CL to allow extension service worker events, it's better if we retrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific ClearRegisteredEventsForTest instead. BUG=721147 Test=No visible change expected. ========== to ========== Restrict EventRouter::Get/Set-RegisteredEvents. The code is going to be changed in a subsequent CL to allow extension service worker events, it's better if we restrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific ClearRegisteredEventsForTest instead. BUG=721147 Test=No visible change expected. ==========
Description was changed from ========== Restrict EventRouter::Get/Set-RegisteredEvents. The code is going to be changed in a subsequent CL to allow extension service worker events, it's better if we restrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific ClearRegisteredEventsForTest instead. BUG=721147 Test=No visible change expected. ========== to ========== Restrict EventRouter::Get/Set-RegisteredEvents. The code is going to be changed in a subsequent CL to allow extension service worker events, it's better if we restrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific ClearRegisteredEventsForTest instead. BUG=721147 Test=No visible change expected. ==========
CL description updated, thanks! https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... File extensions/browser/event_router.h (right): https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... extensions/browser/event_router.h:185: bool HasRegisteredEvents(const ExtensionId& extension_id) const; On 2017/05/12 00:34:25, karandeepb wrote: > The other functions don't use the ExtensionId typedef. Would be better if we > remain consistent with surrounding code by changing this to string, or the rest > to ExtensionId. We started preferring ExtensionId for new code to specify extension id. https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... extensions/browser/event_router.h:188: void ClearRegisteredEventsForTest(const ExtensionId& extension_id); On 2017/05/12 00:34:25, karandeepb wrote: > Optional nit: Both of these can be inlined. Done.
On 2017/05/12 00:56:25, lazyboy wrote: > CL description updated, thanks! > > https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... > File extensions/browser/event_router.h (right): > > https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... > extensions/browser/event_router.h:185: bool HasRegisteredEvents(const > ExtensionId& extension_id) const; > On 2017/05/12 00:34:25, karandeepb wrote: > > The other functions don't use the ExtensionId typedef. Would be better if we > > remain consistent with surrounding code by changing this to string, or the > rest > > to ExtensionId. > > We started preferring ExtensionId for new code to specify extension id. > > https://codereview.chromium.org/2879673002/diff/1/extensions/browser/event_ro... > extensions/browser/event_router.h:188: void ClearRegisteredEventsForTest(const > ExtensionId& extension_id); > On 2017/05/12 00:34:25, karandeepb wrote: > > Optional nit: Both of these can be inlined. > > Done. Still LGTM.
The CQ bit was checked by lazyboy@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.
lazyboy@chromium.org changed reviewers: + benwells@chromium.org
+benwells for chrome/browser/apps/*
c/b/apps lgtm
The CQ bit was checked by lazyboy@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.
The CQ bit was checked by lazyboy@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": 1494956347388870, "parent_rev": "b4d70c2b28f321a94a52615c95692b6da8738783", "commit_rev": "ac968918d5b2b6baf9fe987cb0094aa9a73177c0"}
Message was sent while issue was closed.
Description was changed from ========== Restrict EventRouter::Get/Set-RegisteredEvents. The code is going to be changed in a subsequent CL to allow extension service worker events, it's better if we restrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific ClearRegisteredEventsForTest instead. BUG=721147 Test=No visible change expected. ========== to ========== Restrict EventRouter::Get/Set-RegisteredEvents. The code is going to be changed in a subsequent CL to allow extension service worker events, it's better if we restrict GetRegisteredEvents and SetRegisteredEvents as we don't really need them outside of EventRouter. Expose simpler HasRegisteredEvents and test specific ClearRegisteredEventsForTest instead. BUG=721147 Test=No visible change expected. Review-Url: https://codereview.chromium.org/2879673002 Cr-Commit-Position: refs/heads/master@{#472151} Committed: https://chromium.googlesource.com/chromium/src/+/ac968918d5b2b6baf9fe987cb009... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ac968918d5b2b6baf9fe987cb009... |