Remove support for NPObjects.

Remove support for NPObjects.

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

[dcheng, 2015-11-29 06:27:07]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, wfh@chromium.org

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

Mind taking an initial look? I think I've removed everything related to making
window/plugins scriptable via NPObjects. I've removed quite a few layout tests
(I believe these tests only exercise paths that are reachable via NPAPI). The
remaining layout test failures are tests that I'm hoping to port over to the new
Blink PPAPI test plugin in ppapi/tests/blink_test_plugin.cc.

[haraken, 2015-11-30 01:09:06]

Amazing!!!

The change to WebKit/* looks good to me.

[Will Harris, 2015-12-04 20:54:38]

On 2015/11/30 01:09:06, haraken wrote:
> Amazing!!!
> 
> The change to WebKit/* looks good to me.

Holy big cl, Batman!

I only looked closely at the files that had + lines on them, but if the bots say
"yes" then that's an lgtm from me.

[Wez, 2016-01-05 23:05:05]

tl;dr: So. Many. -Lines. Should have sent a poet.

Looks pretty good; as noted I think there are some deprecated scripting
interfaces in PPAPI that can also go.

[Wez, 2016-01-14 21:57:06]

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

[Wez, 2016-01-14 21:57:08]

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/np...
File content/child/npapi/npobject_util.h (left):

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/np...
content/child/npapi/npobject_util.h:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
nit: Codereview shows this file as Modified, not Deleted for some reason..?

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

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:520: //    NPNVOfflineBool: tells whether
offline-mode is enabled
nit: While you're here... this comment seems almost completely out of date - can
we just remove it?

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:527: return NPERR_INVALID_INSTANCE_ERROR;
You're not failing here because the plugin instance is invalid, but because the
browser no longer supports these NPNVariable values - shouldn't you just remove
the cases entirely, so that we'll return NPERR_GENERIC_ERROR?

https://codereview.chromium.org/1483733002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/pepper_webplugin_impl.cc (left):

https://codereview.chromium.org/1483733002/diff/320001/content/renderer/peppe...
content/renderer/pepper/pepper_webplugin_impl.cc:101:
container->allowScriptObjects();
We should also remove pp::deprecated::ScriptableObject, pp::deprecated::Var and
friends, which are the PPAPI-side counterparts to this glue.

https://codereview.chromium.org/1483733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebBindings.cpp (right):

https://codereview.chromium.org/1483733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebBindings.cpp:33: namespace blink {
What do we still need these for?

[dcheng, 2016-01-14 22:05:50]

As an update, I'm still hacking away on layout tests. If anyone wants to help
out, I've already got the PPAPI plugin checked in and a few tests converted.
There's just a lot of other tests, and it's not always easy to determine what to
do.

Another particular issue revolves around detecting plugin loads and failed
loads: there are a fair number of sandboxing/csp tests that test this, but
there's no reliable signal to determine success/failure atm.

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/np...
File content/child/npapi/npobject_util.h (left):

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/np...
content/child/npapi/npobject_util.h:1: // Copyright 2013 The Chromium Authors.
All rights reserved.
On 2016/01/14 at 21:57:08, Wez wrote:
> nit: Codereview shows this file as Modified, not Deleted for some reason..?

Yeah, I don't know why. I tried to fix it and gave up.

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

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:520: //    NPNVOfflineBool: tells whether
offline-mode is enabled
On 2016/01/14 at 21:57:08, Wez wrote:
> nit: While you're here... this comment seems almost completely out of date -
can we just remove it?

It's probably wrong, but given the size of this patch, I wanted to scope it to
just stuff directly related to NPObject (since Blink stuff is what's blocking
the majority of NPAPI removal).

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:527: return NPERR_INVALID_INSTANCE_ERROR;
On 2016/01/14 at 21:57:08, Wez wrote:
> You're not failing here because the plugin instance is invalid, but because
the browser no longer supports these NPNVariable values - shouldn't you just
remove the cases entirely, so that we'll return NPERR_GENERIC_ERROR?

I'll update this in the next rebasing + update patch.

https://codereview.chromium.org/1483733002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/pepper_webplugin_impl.cc (left):

https://codereview.chromium.org/1483733002/diff/320001/content/renderer/peppe...
content/renderer/pepper/pepper_webplugin_impl.cc:101:
container->allowScriptObjects();
On 2016/01/14 at 21:57:08, Wez wrote:
> We should also remove pp::deprecated::ScriptableObject, pp::deprecated::Var
and friends, which are the PPAPI-side counterparts to this glue.

I thought Flash used it. I could be wrong.

https://codereview.chromium.org/1483733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebBindings.cpp (right):

https://codereview.chromium.org/1483733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebBindings.cpp:33: namespace blink {
On 2016/01/14 at 21:57:08, Wez wrote:
> What do we still need these for?

I left these around for the moment, since we pass function pointers to them to
the plugin host:
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/npap...

The reason for leaving this around is I believe there are some layout tests that
depend on being able to instantiate a NPAPI plugin but none of the NPObject
capability. So leaving these in allows things to hopefully not crash while we
incrementally update tests.

[Wez, 2016-01-19 17:58:28]

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

https://codereview.chromium.org/1483733002/diff/320001/content/child/npapi/pl...
content/child/npapi/plugin_host.cc:527: return NPERR_INVALID_INSTANCE_ERROR;
On 2016/01/14 at 22:05:50, dcheng wrote:
> On 2016/01/14 at 21:57:08, Wez wrote:
> > You're not failing here because the plugin instance is invalid, but because
the browser no longer supports these NPNVariable values - shouldn't you just
remove the cases entirely, so that we'll return NPERR_GENERIC_ERROR?
> 
> I'll update this in the next rebasing + update patch.

ACK

[dcheng, 2016-03-23 06:26:23]

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

[dcheng, 2016-03-23 06:26:25]

+piman since the list of blocking tests is getting pretty short. This CL needs
to be rebaselined after some of the PPAPI test fixes land as well.

[piman, 2016-03-23 17:28:02]

Very cool. Couple of questions.

https://codereview.chromium.org/1483733002/diff/540001/content/child/npapi/np...
File content/child/npapi/np_channel_base.h (right):

https://codereview.chromium.org/1483733002/diff/540001/content/child/npapi/np...
content/child/npapi/np_channel_base.h:45: static int Count();
Can we remove this too?

https://codereview.chromium.org/1483733002/diff/540001/content/renderer/peppe...
File content/renderer/pepper/pepper_webplugin_impl.cc (left):

https://codereview.chromium.org/1483733002/diff/540001/content/renderer/peppe...
content/renderer/pepper/pepper_webplugin_impl.cc:101:
container->allowScriptObjects();
To confirm, this doesn't affect PPB_Var_Deprecated, right?
It's probably leftover from when we used to implement it through NPObject
instead of v8 directly.

https://codereview.chromium.org/1483733002/diff/540001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (left):

https://codereview.chromium.org/1483733002/diff/540001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:581: crbug.com/318978 [ Linux ]
plugins/override-node-method.html [ Skip ]
Some of these (will) have been converted to ppapi (and are still meaningful), so
we'll want to keep them.

[dcheng, 2016-03-24 03:14:08]

https://codereview.chromium.org/1483733002/diff/540001/content/child/npapi/np...
File content/child/npapi/np_channel_base.h (right):

https://codereview.chromium.org/1483733002/diff/540001/content/child/npapi/np...
content/child/npapi/np_channel_base.h:45: static int Count();
On 2016/03/23 at 17:28:02, piman wrote:
> Can we remove this too?

Done.

https://codereview.chromium.org/1483733002/diff/540001/content/renderer/peppe...
File content/renderer/pepper/pepper_webplugin_impl.cc (left):

https://codereview.chromium.org/1483733002/diff/540001/content/renderer/peppe...
content/renderer/pepper/pepper_webplugin_impl.cc:101:
container->allowScriptObjects();
On 2016/03/23 at 17:28:02, piman wrote:
> To confirm, this doesn't affect PPB_Var_Deprecated, right?
> It's probably leftover from when we used to implement it through NPObject
instead of v8 directly.

I don't think so: in any case, every implementation of this method is empty, so
removing it should be a no-op.

https://codereview.chromium.org/1483733002/diff/540001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (left):

https://codereview.chromium.org/1483733002/diff/540001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:581: crbug.com/318978 [ Linux ]
plugins/override-node-method.html [ Skip ]
On 2016/03/23 at 17:28:02, piman wrote:
> Some of these (will) have been converted to ppapi (and are still meaningful),
so we'll want to keep them.

I've reverted all layout test changes just to see how the bots look.

[dcheng, 2016-03-29 05:52:31]

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

[dcheng, 2016-03-29 05:52:33]

OK, PTAL. I rebased and reran the tests on this CL today, and it looks like
we're very close to being able to land this: there were 6 failing tests in the
last CQ run. 4 have already been fixed, and the remaining 2 are probably going
to be removed since we believe they test conditions that are not possible to
trigger with PPAPI.

+tkent for //components/test_runner
+piman for //content
+haraken for //third_party/WebKit

[tkent, 2016-03-29 06:01:04]

components/test_runner lgtm

https://codereview.chromium.org/1483733002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebBindings.cpp (right):

https://codereview.chromium.org/1483733002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebBindings.cpp:35: bool
WebBindings::construct(NPP npp, NPObject* object, const NPVariant* args,
uint32_t argCount, NPVariant* result)
Don't you remove WebBindings entirely?

[haraken, 2016-03-29 06:02:03]

WebKit LGTM

[dcheng, 2016-03-29 07:04:15]

https://codereview.chromium.org/1483733002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebBindings.cpp (right):

https://codereview.chromium.org/1483733002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebBindings.cpp:35: bool
WebBindings::construct(NPP npp, NPObject* object, const NPVariant* args,
uint32_t argCount, NPVariant* result)
On 2016/03/29 at 06:01:04, tkent wrote:
> Don't you remove WebBindings entirely?

I'm planning on trying that in a follow up since this patch is already quite
large. Right now, function pointers to WebBindings methods are set as the
implementation for various NPN functions:
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/npap....

When I looked through the test plugin, it seems like it's probably safe to
delete WebBindings and just set the function pointers to null, but it's also
hard to tell for sure. So for now, I would prefer to defer it to another patch,
if that's OK.

[tkent, 2016-03-29 07:38:57]

https://codereview.chromium.org/1483733002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebBindings.cpp (right):

https://codereview.chromium.org/1483733002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebBindings.cpp:35: bool
WebBindings::construct(NPP npp, NPObject* object, const NPVariant* args,
uint32_t argCount, NPVariant* result)
On 2016/03/29 at 07:04:15, dcheng wrote:
> On 2016/03/29 at 06:01:04, tkent wrote:
> > Don't you remove WebBindings entirely?
> 
> I'm planning on trying that in a follow up since this patch is already quite
large. Right now, function pointers to WebBindings methods are set as the
implementation for various NPN functions:
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/npap....
> 
> When I looked through the test plugin, it seems like it's probably safe to
delete WebBindings and just set the function pointers to null, but it's also
hard to tell for sure. So for now, I would prefer to defer it to another patch,
if that's OK.

Ok, that's fine.

[piman, 2016-03-29 18:35:44]

LGTM, uncomfortably excited.

[dcheng, 2016-03-31 06:46:36]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-03-31 06:46:37]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org,
tkent@chromium.org, haraken@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1483733002/#ps640001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-31 06:47:01]

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

[commit-bot: I haz the power, 2016-03-31 08:03:49]

Committed patchset #33 (id:640001)

[commit-bot: I haz the power, 2016-03-31 08:04:55]

Description was changed from

==========
Remove support for NPObjects.

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

to

==========
Remove support for NPObjects.

BUG=493212

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

[commit-bot: I haz the power, 2016-03-31 08:04:57]

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

[Will Harris, 2016-03-31 14:11:20]

On 2016/03/31 08:04:57, commit-bot: I haz the power wrote:
> Patchset 33 (id:??) landed as
> https://crrev.com/e46cb3d6d59966409e1f2b679c38056e5fadde3d
> Cr-Commit-Position: refs/heads/master@{#384217}

🍾!