Narrow the interface to ResourceMessageFilter::GetContexts()

Narrow the interface to ResourceMessageFilter::GetContexts()

ResourceMessageFilter::GetContexts() accepted a ResourceHostMsg_Request
argument.

This provided great flexibility to implementors, but no guarantees to
callers of which fields were actually required.

Instead, narrow the interface to just the two fields that are currently
used, resource_type and origin_pid. This means that implementers will
have to change the interface if they want to use other fields, which
means that callers will get updated.

The origin_pid field is only used by the deprecated NPAPI plugin support
and should be removed when that support is deleted.

BUG=561549
TEST=content_unittests

Committed: https://crrev.com/cfe6e5de8faa0178c01e9aa59b5cac1c1a89480c
Cr-Commit-Position: refs/heads/master@{#362544}

[Adam Rice, 2015-11-25 16:26:46]

ricea@chromium.org changed reviewers:
+ kinuko@chromium.org

[Adam Rice, 2015-11-25 16:26:46]

kinuko, could you review this if you have time? Otherwise you or I could find
another reviewer.

[kinuko, 2015-11-26 03:52:37]

The code change itself looks good, want to make the documentation around
origin_pid clearer though.

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_message_filter.h (right):

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_message_filter.h:43: int origin_pid,
Can we link to the crbug for deprecating NPAPI?

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_message_filter.h:48: // NULL in unittests or for
requests from the (NPAPI) plugin process.
Could we add a comment here too to note that |origin_pid| argument for
|get_contexts_callback| is not used (and may be invalid) for requests that are
NOT from the plugin process.

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_message_filter.h:62: void
GetContexts(ResourceType resource_type,
Please document that origin_pid is only required for NPAPI plugin here too

[Adam Rice, 2015-11-26 07:54:36]

Description was changed from

==========
Narrow the interface to GetContexts()

ResourceMessageFilter::GetContexts() accepted a ResourceHostMsg_Request
argument.

This provided great flexibility to implementors, but no guarantees to
callers of which fields were actually required.

Instead, narrow the interface to just the two fields that are currently
used, resource_type and origin_pid. This means that implementors will
have to change the interface if they want to use other fields, which
means that callers will get updated.

The origin_pid field is only used by the deprecated NPAPI plugin support
and should be removed when that support is deleted.

BUG=561549
TEST=content_unittests
==========

to

==========
Narrow the interface to ResourceMessageFilter::GetContexts()

ResourceMessageFilter::GetContexts() accepted a ResourceHostMsg_Request
argument.

This provided great flexibility to implementors, but no guarantees to
callers of which fields were actually required.

Instead, narrow the interface to just the two fields that are currently
used, resource_type and origin_pid. This means that implementers will
have to change the interface if they want to use other fields, which
means that callers will get updated.

The origin_pid field is only used by the deprecated NPAPI plugin support
and should be removed when that support is deleted.

BUG=561549
TEST=content_unittests
==========

[Adam Rice, 2015-11-26 08:53:51]

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_message_filter.h (right):

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_message_filter.h:43: int origin_pid,
On 2015/11/26 03:52:37, kinuko wrote:
> Can we link to the crbug for deprecating NPAPI?

Done.

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_message_filter.h:48: // NULL in unittests or for
requests from the (NPAPI) plugin process.
On 2015/11/26 03:52:37, kinuko wrote:
> Could we add a comment here too to note that |origin_pid| argument for
> |get_contexts_callback| is not used (and may be invalid) for requests that are
> NOT from the plugin process.

Done.

https://codereview.chromium.org/1481583002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_message_filter.h:62: void
GetContexts(ResourceType resource_type,
On 2015/11/26 03:52:37, kinuko wrote:
> Please document that origin_pid is only required for NPAPI plugin here too

Done.

[kinuko, 2015-11-26 10:05:20]

lgtm

[Adam Rice, 2015-12-01 18:11:55]

+davidben FYI. If you have no objection, I will land this.

[davidben, 2015-12-01 19:01:57]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-12-01 19:01:57]

lgtm

https://codereview.chromium.org/1481583002/diff/20001/content/browser/loader/...
File content/browser/loader/resource_message_filter.h (right):

https://codereview.chromium.org/1481583002/diff/20001/content/browser/loader/...
content/browser/loader/resource_message_filter.h:42: // crbug.com/493212 is the
tracking bug for NPAPI removal.
It's not also used by PPAPI plugins? (I don't actually know how they work.)

[Adam Rice, 2015-12-01 19:25:46]

https://codereview.chromium.org/1481583002/diff/20001/content/browser/loader/...
File content/browser/loader/resource_message_filter.h (right):

https://codereview.chromium.org/1481583002/diff/20001/content/browser/loader/...
content/browser/loader/resource_message_filter.h:42: // crbug.com/493212 is the
tracking bug for NPAPI removal.
On 2015/12/01 19:01:57, davidben (behind on reviews) wrote:
> It's not also used by PPAPI plugins? (I don't actually know how they work.)

Only RenderProcessHostImpl and PluginProcessHost actually create a
GetContextsCallback object. PluginProcessHost is only created in
PluginServiceImpl::FindOrStartNpapiPluginProcess().

PPAPI plugins work differently. I don't know what they do but it doesn't use
origin_pid.

[Adam Rice, 2015-12-01 19:26:30]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2015-12-01 19:27:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481583002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481583002/20001

[commit-bot: I haz the power, 2015-12-01 20:12:38]

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

[commit-bot: I haz the power, 2015-12-01 20:12:39]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Adam Rice, 2015-12-01 21:34:37]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2015-12-01 21:39:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1481583002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1481583002/20001

[commit-bot: I haz the power, 2015-12-01 23:24:55]

Description was changed from

==========
Narrow the interface to ResourceMessageFilter::GetContexts()

ResourceMessageFilter::GetContexts() accepted a ResourceHostMsg_Request
argument.

This provided great flexibility to implementors, but no guarantees to
callers of which fields were actually required.

Instead, narrow the interface to just the two fields that are currently
used, resource_type and origin_pid. This means that implementers will
have to change the interface if they want to use other fields, which
means that callers will get updated.

The origin_pid field is only used by the deprecated NPAPI plugin support
and should be removed when that support is deleted.

BUG=561549
TEST=content_unittests
==========

to

==========
Narrow the interface to ResourceMessageFilter::GetContexts()

ResourceMessageFilter::GetContexts() accepted a ResourceHostMsg_Request
argument.

This provided great flexibility to implementors, but no guarantees to
callers of which fields were actually required.

Instead, narrow the interface to just the two fields that are currently
used, resource_type and origin_pid. This means that implementers will
have to change the interface if they want to use other fields, which
means that callers will get updated.

The origin_pid field is only used by the deprecated NPAPI plugin support
and should be removed when that support is deleted.

BUG=561549
TEST=content_unittests
==========

[commit-bot: I haz the power, 2015-12-01 23:24:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-01 23:27:25]

Description was changed from

==========
Narrow the interface to ResourceMessageFilter::GetContexts()

ResourceMessageFilter::GetContexts() accepted a ResourceHostMsg_Request
argument.

This provided great flexibility to implementors, but no guarantees to
callers of which fields were actually required.

Instead, narrow the interface to just the two fields that are currently
used, resource_type and origin_pid. This means that implementers will
have to change the interface if they want to use other fields, which
means that callers will get updated.

The origin_pid field is only used by the deprecated NPAPI plugin support
and should be removed when that support is deleted.

BUG=561549
TEST=content_unittests
==========

to

==========
Narrow the interface to ResourceMessageFilter::GetContexts()

ResourceMessageFilter::GetContexts() accepted a ResourceHostMsg_Request
argument.

This provided great flexibility to implementors, but no guarantees to
callers of which fields were actually required.

Instead, narrow the interface to just the two fields that are currently
used, resource_type and origin_pid. This means that implementers will
have to change the interface if they want to use other fields, which
means that callers will get updated.

The origin_pid field is only used by the deprecated NPAPI plugin support
and should be removed when that support is deleted.

BUG=561549
TEST=content_unittests

Committed: https://crrev.com/cfe6e5de8faa0178c01e9aa59b5cac1c1a89480c
Cr-Commit-Position: refs/heads/master@{#362544}
==========

[commit-bot: I haz the power, 2015-12-01 23:27:26]

Patchset 2 (id:??) landed as
https://crrev.com/cfe6e5de8faa0178c01e9aa59b5cac1c1a89480c
Cr-Commit-Position: refs/heads/master@{#362544}