Remove PluginLoadObserver and related logic, it was only used for NPAPI.

Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL, NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=493212
Committed: https://crrev.com/15bd8a880b4999e376a2cecf53fcea2e7e1838d7
Cr-Commit-Position: refs/heads/master@{#363119}

[Nate Chapin, 2015-11-18 00:02:25]

Description was changed from

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

BUG=
==========

to

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURLNotify and NPN_PostURLNotify.

BUG=
==========

[Nate Chapin, 2015-11-18 00:03:57]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2015-11-18 00:03:58]

dcheng, care to review the blink bits, and any others you feel comfortable with?

Is there a bug I should reference for this?

The changes to the npapi-specific chromium code are kind of hacky, and I'm not
sure I really got rid of all of the remnants of these functions, but I did my
best.

https://codereview.chromium.org/1426923007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/PluginDocument.cpp (left):

https://codereview.chromium.org/1426923007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/PluginDocument.cpp:139: const ResourceError&
error = document()->loader()->mainDocumentError();
It seems really weird that this if() block is here, instead of with the other
finish/fail code. Also, it's the only reason we stash the main document error on
DocumentLoader.

[dcheng, 2015-11-18 00:13:00]

I haven't reviewed the patch yet, but 474535 or the bug it's blocking is
probably the best bug for tracking purposes.

[dcheng, 2015-11-18 18:38:18]

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
File content/child/npapi/plugin_host.cc (right):

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:354: NPError NPN_GetURLNotify(NPP id,
I feel like we should just nuke this one too.

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:386: NPError NPN_PostURLNotify(NPP id,
Ditto: why not nuke?

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
File content/child/npapi/plugin_instance.h (right):

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_instance.h:127: PluginStreamUrl*
CreateStream(unsigned long resource_id,
Then we could probably nuke this too.

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
File content/child/npapi/plugin_stream.h (right):

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_stream.h:23: class PluginStream : public
base::RefCounted<PluginStream> {
And probably this thing too. KILL IT ALL.

https://codereview.chromium.org/1426923007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/PluginDocument.cpp (left):

https://codereview.chromium.org/1426923007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/PluginDocument.cpp:141:
view->didFinishLoading();
I think (but it's hard to tell) that these are the only calls to
didFinishLoading / didFailLoading via PluginView. Can we remove the virtuals
from PluginView?

[Nate Chapin, 2015-11-18 23:28:50]

This got big.

It's not clear to me whether deleting some of those layout tests (specifically,
document-open.html, embed-inside-object.html, and
xss-DENIED-plugin-navigation.html) drops coverage for things that have a PPAPI
equivalent.

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
File content/child/npapi/plugin_host.cc (right):

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:354: NPError NPN_GetURLNotify(NPP id,
On 2015/11/18 18:38:18, dcheng wrote:
> I feel like we should just nuke this one too.


These functions are referenced by the _NPPluginFuncs struct that is actually
exposed by the plugin API, so it's not clear to me it's right to drop them
entirely. We have several similarly stubbed functions in this file (e.g.,
NPN_HandleEvent and NPN_UnfocusInstance).

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:386: NPError NPN_PostURLNotify(NPP id,
On 2015/11/18 18:38:18, dcheng wrote:
> Ditto: why not nuke?

See above.

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
File content/child/npapi/plugin_instance.h (right):

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_instance.h:127: PluginStreamUrl*
CreateStream(unsigned long resource_id,
On 2015/11/18 18:38:18, dcheng wrote:
> Then we could probably nuke this too.

Done.

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
File content/child/npapi/plugin_stream.h (right):

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_stream.h:23: class PluginStream : public
base::RefCounted<PluginStream> {
On 2015/11/18 18:38:18, dcheng wrote:
> And probably this thing too. KILL IT ALL.

Done.

https://codereview.chromium.org/1426923007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/PluginDocument.cpp (left):

https://codereview.chromium.org/1426923007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/PluginDocument.cpp:141:
view->didFinishLoading();
On 2015/11/18 18:38:18, dcheng wrote:
> I think (but it's hard to tell) that these are the only calls to
> didFinishLoading / didFailLoading via PluginView. Can we remove the virtuals
> from PluginView?

Done.

[dcheng, 2015-11-19 03:41:40]

lgtm, let's update the CL description to mention that this kills NPN_GetUrl,
NPN_PostUrl, and NPN_URLRedirectResponse too.

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
File content/child/npapi/plugin_host.cc (right):

https://codereview.chromium.org/1426923007/diff/120001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:354: NPError NPN_GetURLNotify(NPP id,
On 2015/11/18 at 23:28:50, Nate Chapin wrote:
> On 2015/11/18 18:38:18, dcheng wrote:
> > I feel like we should just nuke this one too.
> 
> 
> These functions are referenced by the _NPPluginFuncs struct that is actually
exposed by the plugin API, so it's not clear to me it's right to drop them
entirely. We have several similarly stubbed functions in this file (e.g.,
NPN_HandleEvent and NPN_UnfocusInstance).

Oops, sorry. I posted the comment on the wrong lines (I meant to nuke NPN_GetUrl
instead of copying part of the former implementation of NPN_GetUrlNotify over,
and similar for NPN_PostURL). Happily, that's exactly what happened anyway

https://codereview.chromium.org/1426923007/diff/220001/content/child/npapi/pl...
File content/child/npapi/plugin_stream.cc (left):

https://codereview.chromium.org/1426923007/diff/220001/content/child/npapi/pl...
content/child/npapi/plugin_stream.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Do you know why this is considered 'modify' instead of 'delete? It looks awfully
like 'delete' to me.

https://codereview.chromium.org/1426923007/diff/220001/content/renderer/npapi...
File content/renderer/npapi/webplugin_impl.cc (left):

https://codereview.chromium.org/1426923007/diff/220001/content/renderer/npapi...
content/renderer/npapi/webplugin_impl.cc:377: weak_factory_.GetWeakPtr()));
Looks like you can delete weak_factory_ too.

https://codereview.chromium.org/1426923007/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1426923007/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:998: //
LocalFrame::detach() can be called multiple times which
\(^_^)/

[Nate Chapin, 2015-11-19 19:28:50]

https://codereview.chromium.org/1426923007/diff/220001/content/child/npapi/pl...
File content/child/npapi/plugin_stream.cc (left):

https://codereview.chromium.org/1426923007/diff/220001/content/child/npapi/pl...
content/child/npapi/plugin_stream.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2015/11/19 03:41:40, dcheng wrote:
> Do you know why this is considered 'modify' instead of 'delete? It looks
awfully
> like 'delete' to me.

They're definitely deleted locally, git rm fails to re-delete. IDK what
codereview is talking about.

https://codereview.chromium.org/1426923007/diff/220001/content/renderer/npapi...
File content/renderer/npapi/webplugin_impl.cc (left):

https://codereview.chromium.org/1426923007/diff/220001/content/renderer/npapi...
content/renderer/npapi/webplugin_impl.cc:377: weak_factory_.GetWeakPtr()));
On 2015/11/19 03:41:40, dcheng wrote:
> Looks like you can delete weak_factory_ too.

Done.

[Nate Chapin, 2015-11-19 19:29:39]

Description was changed from

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURLNotify and NPN_PostURLNotify.

BUG=
==========

to

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL,
NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=
==========

[Nate Chapin, 2015-11-19 19:31:49]

japhet@chromium.org changed reviewers:
+ wfh@chromium.org

[Nate Chapin, 2015-11-19 19:31:50]

wfh, would you mind double-checking the test deletions and making sure you're OK
with them?

[Will Harris, 2015-11-20 01:00:30]

strange, I thought I already replied, sorry.

this lgtm - it seems that all the interfaces being removed were not being used
by pepper, so looks safe to remove :gulp:

[Nate Chapin, 2015-11-20 18:15:25]

Description was changed from

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL,
NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=
==========

to

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL,
NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=493212
==========

[Nate Chapin, 2015-11-20 18:16:13]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-11-20 18:16:13]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1426923007/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-11-20 18:17:29]

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

[commit-bot: I haz the power, 2015-11-20 18:37:11]

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

[commit-bot: I haz the power, 2015-11-20 18:37:12]

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

[Nate Chapin, 2015-11-20 19:08:26]

japhet@chromium.org changed reviewers:
+ jochen@chromium.org

[Nate Chapin, 2015-11-20 19:08:27]

jochen, is this something you're comfortable reviewing? content/ and components/
are what I still need OWNERS for.

[jochen (gone - plz use gerrit), 2015-11-24 10:38:31]

LGTM++

[sof, 2015-11-26 13:31:24]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-11-26 13:31:25]

Does http/tests/plugins/geturlnotify-from-npp-destroystream.html still serve a
purpose after this CL? (It isn't deleted here)

[dcheng, 2015-11-27 20:37:50]

On 2015/11/26 at 13:31:25, sigbjornf wrote:
> Does http/tests/plugins/geturlnotify-from-npp-destroystream.html still serve a
purpose after this CL? (It isn't deleted here)

We can probably go ahead and nuke that as well.

[Nate Chapin, 2015-12-03 22:34:36]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-12-03 22:34:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dcheng@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1426923007/#ps260001
(title: "Rebase, delete
http/tests/plugins/geturlnotify-from-npp-destroystream.html")

[commit-bot: I haz the power, 2015-12-03 22:37:01]

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

[commit-bot: I haz the power, 2015-12-03 22:57:17]

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

[commit-bot: I haz the power, 2015-12-03 22:57:19]

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

[Nate Chapin, 2015-12-03 23:02:09]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-12-03 23:02:09]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
dcheng@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1426923007/#ps280001
(title: " ")

[commit-bot: I haz the power, 2015-12-03 23:04:16]

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

[commit-bot: I haz the power, 2015-12-04 01:10:09]

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

[commit-bot: I haz the power, 2015-12-04 01:10:10]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[dcheng, 2015-12-04 02:51:55]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-12-04 02:55:18]

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

[commit-bot: I haz the power, 2015-12-04 04:01:43]

Description was changed from

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL,
NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=493212
==========

to

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL,
NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=493212
==========

[commit-bot: I haz the power, 2015-12-04 04:01:44]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-12-04 04:02:31]

Description was changed from

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL,
NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=493212
==========

to

==========
Remove PluginLoadObserver and related logic, it was only used for NPAPI.

This really, truly kills NPN_GetURL, NPN_GetURLNotify, NPN_PostURL,
NPN_PostURLNotify, and NPN_URLRedirectResponse.

BUG=493212

Committed: https://crrev.com/15bd8a880b4999e376a2cecf53fcea2e7e1838d7
Cr-Commit-Position: refs/heads/master@{#363119}
==========

[commit-bot: I haz the power, 2015-12-04 04:02:32]

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