Don't set the request target type to object when navigating from a Pepper plug-in.

Don't set the request target type to object when navigating from a Pepper plug-in.

The request target type used to be object for all requests coming from a Pepper plug-in (since http://crrev.com/142370), but we only want it for the initial plug-in file, not for navigations.

BUG=283374
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221231

[Bernhard Bauer, 2013-09-02 16:23:21]

Please review.

[vabr (Chromium), 2013-09-02 16:32:17]

On 2013/09/02 16:23:21, Bernhard Bauer wrote:
> Please review.

Fix LGTM.
A test for that would be nice, but that should have already been added by me in
the referenced revision.

Cheers,
Vaclav

[Bernhard Bauer, 2013-09-03 15:55:53]

Yuzhu, can I get an OWNERS review? Thanks!

[yzshen1, 2013-09-03 18:13:39]

https://codereview.chromium.org/23548008/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/23548008/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2760:
WebURLRequest::TargetIsMainFrame,
Is it possible that this navigate happens in a subframe?

[Bernhard Bauer, 2013-09-03 20:58:02]

https://codereview.chromium.org/23548008/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/23548008/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2760:
WebURLRequest::TargetIsMainFrame,
On 2013/09/03 18:13:40, yzshen1 wrote:
> Is it possible that this navigate happens in a subframe?

Hm, good point! Fixed.

[yzshen1, 2013-09-03 21:04:25]

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2760: frame->parent() ?
(forgive me if I misunderstand this)

I thought whether to use TargetIsSubframe/TargetIsMainFrame is determined by
|target|, not |frame|. Maybe we should use |target_frame| as line 2775?

[Bernhard Bauer, 2013-09-03 21:15:08]

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2760: frame->parent() ?
On 2013/09/03 21:04:26, yzshen1 wrote:
> (forgive me if I misunderstand this)
> 
> I thought whether to use TargetIsSubframe/TargetIsMainFrame is determined by
> |target|, not |frame|. Maybe we should use |target_frame| as line 2775?

Hm, I think we don't have a target in all cases, i.e. |target_frame| could be
NULL. Would that mean that the navigation would happen in |frame|?

[yzshen1, 2013-09-03 21:19:41]

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2760: frame->parent() ?
I don't know the answer. I think we could look into the implementation of
loadFrameRequest().

On 2013/09/03 21:15:09, Bernhard Bauer wrote:
> On 2013/09/03 21:04:26, yzshen1 wrote:
> > (forgive me if I misunderstand this)
> > 
> > I thought whether to use TargetIsSubframe/TargetIsMainFrame is determined by
> > |target|, not |frame|. Maybe we should use |target_frame| as line 2775?
> 
> Hm, I think we don't have a target in all cases, i.e. |target_frame| could be
> NULL. Would that mean that the navigation would happen in |frame|?

[Bernhard Bauer, 2013-09-03 21:59:26]

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/23548008/diff/12001/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2760: frame->parent() ?
On 2013/09/03 21:19:42, yzshen1 wrote:
> I don't know the answer. I think we could look into the implementation of
> loadFrameRequest().

Hm, I think so; see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

Updated the code to that effect, PTAL?

[Bernhard Bauer, 2013-09-04 11:45:24]

I take it back!

I looked into it a bit more, and ResourceFetcher infers the target type
automatically if it's unspecified
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
This is also how this works for NPAPI plug-ins without having to explicitly
specify a target type.

[yzshen1, 2013-09-04 16:49:08]

LGTM

Thanks!

[commit-bot: I haz the power, 2013-09-04 16:49:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23548008/47001

[commit-bot: I haz the power, 2013-09-04 18:47:52]

Change committed as 221231