Stop implementing NPN_GetAuthenticationInfo

RESOLVED FIXED in Firefox 55

Status

()

Core
Plug-ins
P2
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: Benjamin Smedberg, Assigned: Raoul D'Cunha, Mentored)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 months ago
NPN_GetAuthenticationInfo is an NPAPI API we implemented for Java and since we only support Flash we no longer need it.

Remove the following:

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/dom/plugins/base/nsNPAPIPlugin.cpp#2490

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/dom/plugins/ipc/PPluginInstance.ipdl#250

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/dom/plugins/ipc/PluginModuleChild.cpp#1650

https://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_getauthenticationinfo.html

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/dom/plugins/test/testplugin/nptest.cpp#3015

And then I'm sure there'll be some other fixups to make this all work and pass tests.

I'll mentor. I expect this to be a relatively easy bug.
(Reporter)

Updated

8 months ago
Summary: Stop impelementing NPN_GetAuthenticationInfo → Stop implementing NPN_GetAuthenticationInfo
Comment hidden (offtopic)
Comment hidden (off-topic)
(Reporter)

Comment 3

8 months ago
Other than the specific test I mentioned, it would be good to run the major plugin tests: `mach mochitest dom/plugins`
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8856462 - Flags: review?(benjamin)
(Reporter)

Updated

8 months ago
Assignee: nobody → raoul_dcunha
(Reporter)

Comment 5

8 months ago
mozreview-review
Comment on attachment 8856462 [details]
Bug 1352576 - Removed all instances of NPN_GetAuthenticationInfo from the codebase

https://reviewboard.mozilla.org/r/128420/#review130882

This is a great start! I've made one important note about not changing npfunctions.h because it's an external API layer (Flash depends on the current layout which we must not change). So I'll need to see an updated version with that fixed.

Once that's ready, do you have try server access, or shall I trigger a try build for you?

::: dom/plugins/base/npfunctions.h
(Diff revision 1)
>    NPN_EnumerateProcPtr enumerate;
>    NPN_PluginThreadAsyncCallProcPtr pluginthreadasynccall;
>    NPN_ConstructProcPtr construct;
>    NPN_GetValueForURLPtr getvalueforurl;
>    NPN_SetValueForURLPtr setvalueforurl;
> -  NPN_GetAuthenticationInfoPtr getauthenticationinfo;

NPAPI is a binary protocol, so we shouldn't change this file at all. Just keep this here and fill it in with nullptr in our implementation.

::: dom/plugins/base/nsNPAPIPlugin.cpp
(Diff revision 1)
>    _enumerate,
>    _pluginthreadasynccall,
>    _construct,
>    _getvalueforurl,
>    _setvalueforurl,
> -  _getauthenticationinfo,

nullptr here

::: dom/plugins/ipc/PluginModuleChild.cpp
(Diff revision 1)
>      mozilla::plugins::child::_enumerate,
>      mozilla::plugins::child::_pluginthreadasynccall,
>      mozilla::plugins::child::_construct,
>      mozilla::plugins::child::_getvalueforurl,
>      mozilla::plugins::child::_setvalueforurl,
> -    mozilla::plugins::child::_getauthenticationinfo,

And nullptr here
Attachment #8856462 - Flags: review?(benjamin) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 7

7 months ago
mozreview-review-reply
Comment on attachment 8856462 [details]
Bug 1352576 - Removed all instances of NPN_GetAuthenticationInfo from the codebase

https://reviewboard.mozilla.org/r/128420/#review130882

Thanks for the review, Benjamin!
(Assignee)

Updated

7 months ago
Attachment #8857909 - Flags: review?(benjamin)
(Reporter)

Comment 8

7 months ago
Raoul, this looks like the correct change but it shouldn't be a new commit, rather a modification of the first. Can you fold these two commits together? You should be able to do this using the `hg histedit` command: https://www.mercurial-scm.org/wiki/HisteditExtension
Flags: needinfo?(raoul_dcunha)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8857909 - Attachment is obsolete: true
Attachment #8857909 - Flags: review?(benjamin)
(Assignee)

Comment 10

7 months ago
(In reply to Benjamin Smedberg [:bsmedberg] from comment #8)
> Raoul, this looks like the correct change but it shouldn't be a new commit,
> rather a modification of the first. Can you fold these two commits together?
> You should be able to do this using the `hg histedit` command:
> https://www.mercurial-scm.org/wiki/HisteditExtension

I've squashed the commits :). If you are happy with the change, can you trigger a try build for me as I don't have access to the try server
Flags: needinfo?(raoul_dcunha)
(Reporter)

Comment 11

7 months ago
mozreview-review
Comment on attachment 8856462 [details]
Bug 1352576 - Removed all instances of NPN_GetAuthenticationInfo from the codebase

https://reviewboard.mozilla.org/r/128420/#review132682

Woot!
Attachment #8856462 - Flags: review?(benjamin) → review+

Comment 12

7 months ago
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f7780670900
Removed all instances of NPN_GetAuthenticationInfo from the codebase r=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/5f7780670900
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.