Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2594193002:
Only use host() for chrome:// urls in EventListener (Closed)
DescriptionOnly use host() for chrome:// urls in EventListener
See bug for details.
BUG=536858
Committed: https://crrev.com/327c18b2e5d3125d15f2afc288a070c2ca772ac5
Cr-Commit-Position: refs/heads/master@{#440770}
Patch Set 1 : Use GetOrigin for all listener_urls #
Total comments: 5
Patch Set 2 : Add comment #
Total comments: 2
Patch Set 3 : Use url::Origin #Messages
Total messages: 24 (13 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
stevenjb@chromium.org changed reviewers: + asargent@chromium.org, rdevlin.cronin@chromium.org
I think this is actually the Right Thing To Do, but I will leave it to the experts to decide :)
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
lgtm https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... File extensions/browser/event_listener_map.cc (right): https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... extensions/browser/event_listener_map.cc:40: return base::WrapUnique(new EventListener( nit: MakeUnique<EventListener>(...)
https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... File extensions/browser/event_listener_map.cc (right): https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... extensions/browser/event_listener_map.cc:40: return base::WrapUnique(new EventListener( On 2016/12/22 01:51:19, Dan Beam wrote: > nit: MakeUnique<EventListener>(...) WrapUnique is used here because the constructor is private, and I'm not going to change that here.
https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... File extensions/browser/event_listener_map.cc (right): https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... extensions/browser/event_listener_map.cc:40: return base::WrapUnique(new EventListener( On 2016/12/22 01:55:29, stevenjb wrote: > On 2016/12/22 01:51:19, Dan Beam wrote: > > nit: MakeUnique<EventListener>(...) > > WrapUnique is used here because the constructor is private, and I'm not going to > change that here. ah, fair
https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... File extensions/browser/event_listener_map.cc (right): https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... extensions/browser/event_listener_map.cc:41: event_name, "", listener_url.GetOrigin(), process, std::move(filter))); let's add a comment saying why we do this.
https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... File extensions/browser/event_listener_map.cc (right): https://codereview.chromium.org/2594193002/diff/40001/extensions/browser/even... extensions/browser/event_listener_map.cc:41: event_name, "", listener_url.GetOrigin(), process, std::move(filter))); On 2016/12/22 16:01:09, Devlin wrote: > let's add a comment saying why we do this. Done.
The CQ bit was checked by stevenjb@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.
lgtm https://codereview.chromium.org/2594193002/diff/60001/extensions/browser/even... File extensions/browser/event_listener_map.cc (right): https://codereview.chromium.org/2594193002/diff/60001/extensions/browser/even... extensions/browser/event_listener_map.cc:44: event_name, "", listener_url.GetOrigin(), process, std::move(filter))); for annoying reasons, let's actually make this url::Origin(listener_url).GetURL(). Also, please add a // TODO(devlin): If we dispatched events to processes more intelligently, this could be avoided.
https://codereview.chromium.org/2594193002/diff/60001/extensions/browser/even... File extensions/browser/event_listener_map.cc (right): https://codereview.chromium.org/2594193002/diff/60001/extensions/browser/even... extensions/browser/event_listener_map.cc:44: event_name, "", listener_url.GetOrigin(), process, std::move(filter))); On 2016/12/27 17:12:09, Devlin wrote: > for annoying reasons, let's actually make this > url::Origin(listener_url).GetURL(). Also, please add a > // TODO(devlin): If we dispatched events to processes more intelligently, this > could be avoided. Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2594193002/#ps80001 (title: "Use url::Origin")
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": 1482861460865110,
"parent_rev": "2b049e602ec3cc00fbe5eb0b40b0038e5ebedc6a", "commit_rev":
"391a07b7716df0491e2a2b0c9a2b264c811863d2"}
Message was sent while issue was closed.
Description was changed from ========== Only use host() for chrome:// urls in EventListener See bug for details. BUG=536858 ========== to ========== Only use host() for chrome:// urls in EventListener See bug for details. BUG=536858 Review-Url: https://codereview.chromium.org/2594193002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Only use host() for chrome:// urls in EventListener See bug for details. BUG=536858 Review-Url: https://codereview.chromium.org/2594193002 ========== to ========== Only use host() for chrome:// urls in EventListener See bug for details. BUG=536858 Committed: https://crrev.com/327c18b2e5d3125d15f2afc288a070c2ca772ac5 Cr-Commit-Position: refs/heads/master@{#440770} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/327c18b2e5d3125d15f2afc288a070c2ca772ac5 Cr-Commit-Position: refs/heads/master@{#440770} |
