Last Comment Bug 767639 - Remove remaining ActiveX stub
: Remove remaining ActiveX stub
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: John Schoenick [:johns]
:
Mentors:
Depends on: 745030
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-22 17:30 PDT by John Schoenick [:johns]
Modified: 2012-08-27 05:02 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove activeX stubs and simpify classid/java handling (10.51 KB, patch)
2012-08-07 13:58 PDT, John Schoenick [:johns]
jaas: review+
john: checkin+
Details | Diff | Splinter Review
Restore activex classid hook for object tags (12.08 KB, patch)
2012-08-24 13:37 PDT, John Schoenick [:johns]
jaas: review-
Details | Diff | Splinter Review

Description John Schoenick [:johns] 2012-06-22 17:30:18 PDT
nsObjectLoadingContent has code related to trying to load an ActiveX plugin if we have classid="clsid:blah" and a helper-plugin that can actually support activeX. This should just go away as we've not had such a helper plugin in a long time
Comment 1 John Schoenick [:johns] 2012-08-07 13:58:50 PDT
Created attachment 649797 [details] [diff] [review]
Remove activeX stubs and simpify classid/java handling
Comment 2 John Schoenick [:johns] 2012-08-07 14:47:36 PDT
Comment on attachment 649797 [details] [diff] [review]
Remove activeX stubs and simpify classid/java handling

This simplifies handling java a bit, and removes the special cases for classid now that classid can only be 'java:' URIs

In particular, objects like:
> <object type="application/x-java-vm" data="my.class"></object>

Wont spuriously open data streams that will never be used and possibly generate meaningless errors
Comment 3 John Schoenick [:johns] 2012-08-17 14:29:40 PDT
Comment on attachment 649797 [details] [diff] [review]
Remove activeX stubs and simpify classid/java handling

https://hg.mozilla.org/integration/mozilla-inbound/rev/471f34ddcceb

try:
https://tbpl.mozilla.org/?tree=Try&rev=02f602af452c
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-08-17 19:22:07 PDT
https://hg.mozilla.org/mozilla-central/rev/471f34ddcceb
Comment 5 Jacek Caban 2012-08-24 10:55:25 PDT
(In reply to John Schoenick [:johns] from comment #0)
> This should just go away as we've not had such a helper plugin in a long time

We re-used this functionality in Wine. We have MSHTML implementation based on our forked (but close to upstream) Gecko fork and our own pseudo-plugin that, in combination with our MSHTML, adds support for ActiveX components. All we need is Gecko to recognize classid="clsid:..." as application/x-oleobject MIME. Thus, the patch from this bug causes big regression for us. We may deal with it in our fork, but it's always nicer to not have downstream changes. And given that we need very little from Gecko, the maintenance cost of having such code is pretty low for Mozilla.

Would you please consider r+ for a patch reverting relevant parts of the patch? I will take care of that if you'd agree.
Comment 6 John Schoenick [:johns] 2012-08-24 13:35:57 PDT
(In reply to Jacek Caban from comment #5)
> (In reply to John Schoenick [:johns] from comment #0)
> > This should just go away as we've not had such a helper plugin in a long time
> 
> We re-used this functionality in Wine. We have MSHTML implementation based
> on our forked (but close to upstream) Gecko fork and our own pseudo-plugin
> that, in combination with our MSHTML, adds support for ActiveX components.
> All we need is Gecko to recognize classid="clsid:..." as
> application/x-oleobject MIME. Thus, the patch from this bug causes big
> regression for us. We may deal with it in our fork, but it's always nicer to
> not have downstream changes. And given that we need very little from Gecko,
> the maintenance cost of having such code is pretty low for Mozilla.
> 
> Would you please consider r+ for a patch reverting relevant parts of the
> patch? I will take care of that if you'd agree.

The code necessary to support this is pretty minimal, so it comes down to whether or not we want this hook present. If it's serving a useful purpose somewhere I have no issue with it, so I'll leave it up to josh
Comment 7 John Schoenick [:johns] 2012-08-24 13:37:13 PDT
Created attachment 655132 [details] [diff] [review]
Restore activex classid hook for object tags

This restores the activex hook while maintaining the java/uri-handling cleanup I snuck into the patch.
ActiveX similarly uses 'codebase' as its actual-URI, so it can just piggyback on the java logic here
Comment 8 Josh Aas 2012-08-24 14:31:26 PDT
Comment on attachment 655132 [details] [diff] [review]
Restore activex classid hook for object tags

Review of attachment 655132 [details] [diff] [review]:
-----------------------------------------------------------------

I have a strong desire to be helpful here but maintaining code we don't need in a notoriously touchy area for the sake of one particular project that already maintains a fork of Gecko doesn't seem like the right move. I don't want to take on this added compatibility concern for Wine when Wine already has the code (John's patch here is probably good) and a delivery mechanism in place. If the maintenance cost for us is low, then it is probably also low for Wine.

I don't expect this code to change a lot after John's project to clean it up is over in a week or so, and I'd be happy to help answer questions you guys might have in the future.
Comment 9 John Schoenick [:johns] 2012-08-24 15:31:10 PDT
Remarking as fixed, in that case. I did push this patch to try though, to make sure it doesn't introduce any issues:

https://tbpl.mozilla.org/?tree=Try&rev=fbe4d149be3f

Feel free to ping josh or myself if issues with this arise
Comment 10 Jacek Caban 2012-08-27 05:02:57 PDT
Thanks a lot for the patch and feedback! It looks good on try and passes all Wine tests, so I've committed it to our repository.

Note You need to log in before you can comment on or make changes to this bug.