[Extensions] Make Event::restrict_to_browser_context const.

[Extensions] Make Event::restrict_to_browser_context const.

In all of the places where clients were setting
Event::restrict_to_browser_context, all of them were setting it right
after creating an Event instance. There's already a version of constructor
for that. Use it.

TBR=dmazzoni@chromium.org for chrome/browser/speech/extension_api/*
TBR=derat@chromium.org for chrome/browser/ui/ash/*
BUG=723754
Test=None

Review-Url: https://codereview.chromium.org/2898383002
Cr-Commit-Position: refs/heads/master@{#474448}
Committed: https://chromium.googlesource.com/chromium/src/+/59155a46351362462cecf6ed4b6c584f2519cc34

[lazyboy, 2017-05-24 01:24:14]

Description was changed from

==========
[Extensions] Make Event::restrict_to_browser_context const.

In all of the places where clients were setting
Event::restrict_to_browser_context were setting it right after creating
an Event instance. There's already a version of constructor for that.
Use it.

BUG=723754
Test=None
==========

to

==========
[Extensions] Make Event::restrict_to_browser_context const.

In all of the places where clients were setting
Event::restrict_to_browser_context, all of them were setting it right
after creating an Event instance. There's already a version of constructor
for that. Use it.

BUG=723754
Test=None
==========

[lazyboy, 2017-05-24 01:36:37]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-05-24 01:36:38]

I wonder how many Events that are constructed without browser_context are
correct... (through the first Event constructor in event_router.h)? Either way,
this CL doesn't touch those.

[lazyboy, 2017-05-24 01:38:34]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 01:39:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 03:15:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 03:15:56]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-24 16:08:19]

On 2017/05/24 01:36:38, lazyboy wrote:
> I wonder how many Events that are constructed without browser_context are
> correct... (through the first Event constructor in event_router.h)? Either
way,
> this CL doesn't touch those.

+1; my guess is most of these are probably wrong.

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/downloads/downloads_api.cc:1893: // events with
on-record extension renderers.
This makes sense (at least, somewhat), but it's still the wrong granularity. 
Instead, we should introduce something like "include incognito" in the event
structure to handle this case, because otherwise this will broadcast to
unrelated profiles.  Can we add a TODO?

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(left):

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:58:
event->restrict_to_browser_context = profile;
maybe substitute this with DCHECK_EQ(profile,
event->restrict_to_browser_context)?

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:159:
web_navigation::OnDOMContentLoaded::Create(details)));
We need to add restrict_to_browser_context here if we remove it from
DispatchEvent()

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:177:
web_navigation::OnCompleted::Create(details)));
ditto

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:236:
DispatchEvent(web_contents->GetBrowserContext(), std::move(event), url);
nit: cache context

https://codereview.chromium.org/2898383002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/2898383002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:366: // If non-NULL, then the event will not
be sent to other BrowserContexts
nit: s/NULL/null

[lazyboy, 2017-05-24 19:19:33]

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/downloads/downloads_api.cc:1893: // events with
on-record extension renderers.
On 2017/05/24 16:08:18, Devlin (catching up) wrote:
> This makes sense (at least, somewhat), but it's still the wrong granularity. 
> Instead, we should introduce something like "include incognito" in the event
> structure to handle this case, because otherwise this will broadcast to
> unrelated profiles.  Can we add a TODO?

Added a crbug. Added TODO here and in Event constructor.

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(left):

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:58:
event->restrict_to_browser_context = profile;
On 2017/05/24 16:08:18, Devlin (catching up) wrote:
> maybe substitute this with DCHECK_EQ(profile,
> event->restrict_to_browser_context)?

Done.

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
(right):

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:159:
web_navigation::OnDOMContentLoaded::Create(details)));
On 2017/05/24 16:08:18, Devlin (catching up) wrote:
> We need to add restrict_to_browser_context here if we remove it from
> DispatchEvent()

Thanks for catching this!
Done.

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:177:
web_navigation::OnCompleted::Create(details)));
On 2017/05/24 16:08:18, Devlin (catching up) wrote:
> ditto

Done.

https://codereview.chromium.org/2898383002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:236:
DispatchEvent(web_contents->GetBrowserContext(), std::move(event), url);
On 2017/05/24 16:08:19, Devlin (catching up) wrote:
> nit: cache context

Done.

https://codereview.chromium.org/2898383002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/2898383002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:366: // If non-NULL, then the event will not
be sent to other BrowserContexts
On 2017/05/24 16:08:19, Devlin (catching up) wrote:
> nit: s/NULL/null

Done.

[Devlin, 2017-05-24 20:18:23]

lgtm; thanks for the cleanup!

[lazyboy, 2017-05-24 20:49:42]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 20:50:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-05-24 20:54:37]

lazyboy@chromium.org changed reviewers:
+ xiyuan@chromium.org

[lazyboy, 2017-05-24 20:54:38]

+Xiyuan for chrome/browser/chromeos

[xiyuan, 2017-05-24 21:01:38]

chromeos lgtm

[commit-bot: I haz the power, 2017-05-24 21:43:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 21:43:18]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-05-24 21:49:48]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-05-24 21:49:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2898383002/#ps30036
(title: "sync @tott")

[commit-bot: I haz the power, 2017-05-24 21:50:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 21:59:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 21:59:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lazyboy, 2017-05-24 22:09:50]

Description was changed from

==========
[Extensions] Make Event::restrict_to_browser_context const.

In all of the places where clients were setting
Event::restrict_to_browser_context, all of them were setting it right
after creating an Event instance. There's already a version of constructor
for that. Use it.

BUG=723754
Test=None
==========

to

==========
[Extensions] Make Event::restrict_to_browser_context const.

In all of the places where clients were setting
Event::restrict_to_browser_context, all of them were setting it right
after creating an Event instance. There's already a version of constructor
for that. Use it.

TBR=dmazzoni@chromium.org for chrome/browser/speech/extension_api/*
TBR=derat@chromium.org for chrome/browser/ui/ash/*
BUG=723754
Test=None
==========

[lazyboy, 2017-05-24 22:14:49]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2017-05-24 22:16:21]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 22:23:39]

CQ is committing da patch.

Bot data: {"patchset_id": 30036, "attempt_start_ts": 1495664089895100,
"parent_rev": "6552fc43089472109de62975dbb91553d7dc19df", "commit_rev":
"59155a46351362462cecf6ed4b6c584f2519cc34"}

[commit-bot: I haz the power, 2017-05-24 22:23:52]

Description was changed from

==========
[Extensions] Make Event::restrict_to_browser_context const.

In all of the places where clients were setting
Event::restrict_to_browser_context, all of them were setting it right
after creating an Event instance. There's already a version of constructor
for that. Use it.

TBR=dmazzoni@chromium.org for chrome/browser/speech/extension_api/*
TBR=derat@chromium.org for chrome/browser/ui/ash/*
BUG=723754
Test=None
==========

to

==========
[Extensions] Make Event::restrict_to_browser_context const.

In all of the places where clients were setting
Event::restrict_to_browser_context, all of them were setting it right
after creating an Event instance. There's already a version of constructor
for that. Use it.

TBR=dmazzoni@chromium.org for chrome/browser/speech/extension_api/*
TBR=derat@chromium.org for chrome/browser/ui/ash/*
BUG=723754
Test=None

Review-Url: https://codereview.chromium.org/2898383002
Cr-Commit-Position: refs/heads/master@{#474448}
Committed:
https://chromium.googlesource.com/chromium/src/+/59155a46351362462cecf6ed4b6c...
==========

[commit-bot: I haz the power, 2017-05-24 22:23:53]

Committed patchset #4 (id:30036) as
https://chromium.googlesource.com/chromium/src/+/59155a46351362462cecf6ed4b6c...