Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(220)

Issue 2937623002: [Extensions] Simplify EventFilteringInfo (Closed)

Created:
3 years, 6 months ago by Devlin
Modified:
3 years, 6 months ago
Reviewers:
karandeepb
CC:
chromium-reviews, chromim-apps-reviews_chromium.org, extensions-reviews_chromium.org, lazyboy
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Extensions] Simplify EventFilteringInfo EventFilteringInfo currently has a bunch of parameters and boolean flags indicating whether or not it "has" those parameters. This can be greatly simplified with base::Optional. Use base::Optional for all the members in EventFilteringInfo and, since it only exposed getters/setters, make it a struct. This cleanup will be very useful in passing the filtering info directly over IPC. BUG=732564 Review-Url: https://codereview.chromium.org/2937623002 Cr-Commit-Position: refs/heads/master@{#479412} Committed: https://chromium.googlesource.com/chromium/src/+/22da9e8bfbd02c8052f076e59e203bd2dd74c4eb

Patch Set 1 #

Patch Set 2 : . #

Total comments: 4

Patch Set 3 : karan's #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -132 lines) Patch
M chrome/browser/extensions/api/mdns/mdns_api.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/tabs/windows_event_router.cc View 2 chunks +12 lines, -10 lines 0 comments Download
M chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/menu_manager.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/api/web_request/web_request_api.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/event_listener_map_unittest.cc View 8 chunks +9 lines, -9 lines 0 comments Download
M extensions/browser/guest_view/extensions_guest_view_manager_delegate.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/common/event_filter.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M extensions/common/event_filter_unittest.cc View 3 chunks +6 lines, -6 lines 0 comments Download
M extensions/common/event_filtering_info.h View 2 chunks +8 lines, -39 lines 0 comments Download
M extensions/common/event_filtering_info.cc View 1 2 1 chunk +41 lines, -48 lines 0 comments Download
M extensions/common/event_matcher.h View 1 chunk +1 line, -2 lines 0 comments Download
M extensions/common/event_matcher.cc View 1 chunk +8 lines, -7 lines 0 comments Download
M extensions/renderer/api_event_handler.h View 1 1 chunk +1 line, -1 line 0 comments Download
M extensions/renderer/api_event_listeners.h View 1 chunk +1 line, -1 line 0 comments Download
M extensions/renderer/event_emitter.h View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 22 (17 generated)
Devlin
karandeepb@, mind taking a look? lazyboy@, FYI since we're both touching event-y code.
3 years, 6 months ago (2017-06-14 00:10:04 UTC) #9
karandeepb
LGTM with nits. https://codereview.chromium.org/2937623002/diff/20001/extensions/common/event_filtering_info.cc File extensions/common/event_filtering_info.cc (right): https://codereview.chromium.org/2937623002/diff/20001/extensions/common/event_filtering_info.cc#newcode32 extensions/common/event_filtering_info.cc:32: url = maybe_url; std::move this? https://codereview.chromium.org/2937623002/diff/20001/extensions/common/event_filtering_info.cc#newcode36 ...
3 years, 6 months ago (2017-06-14 01:48:20 UTC) #12
Devlin
https://codereview.chromium.org/2937623002/diff/20001/extensions/common/event_filtering_info.cc File extensions/common/event_filtering_info.cc (right): https://codereview.chromium.org/2937623002/diff/20001/extensions/common/event_filtering_info.cc#newcode32 extensions/common/event_filtering_info.cc:32: url = maybe_url; On 2017/06/14 01:48:19, karandeepb wrote: > ...
3 years, 6 months ago (2017-06-14 16:02:36 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2937623002/40001
3 years, 6 months ago (2017-06-14 16:03:08 UTC) #19
commit-bot: I haz the power
3 years, 6 months ago (2017-06-14 16:28:48 UTC) #22
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/22da9e8bfbd02c8052f076e59e20...

Powered by Google App Engine
This is Rietveld 408576698