Bug 29054 - Remove dead ActiveX code in RenderPartObject
Summary: Remove dead ActiveX code in RenderPartObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-08 14:17 PDT by John Abd-El-Malek
Modified: 2009-09-11 10:29 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch (5.44 KB, patch)
2009-09-08 14:21 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Proposed patch (5.45 KB, patch)
2009-09-08 14:23 PDT, John Abd-El-Malek
dglazkov: review+
dglazkov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2009-09-08 14:17:38 PDT
There's some code in that file that was added when Chromium was hosting some ActiveX controls.  That's not the case anymore, so that code can be removed from WebKit to clean things up.
Comment 1 John Abd-El-Malek 2009-09-08 14:21:06 PDT
Created attachment 39213 [details]
Proposed patch
Comment 2 John Abd-El-Malek 2009-09-08 14:23:02 PDT
Created attachment 39215 [details]
Proposed patch

Previous patch didn't have an updated ChangeLog.
Comment 3 Eric Seidel (no email) 2009-09-09 08:12:22 PDT
It's difficult for me to tell at first glance if all of this code is only used by Chromium or not.  I guess it would be slightly easier if we referenced the original revision where this all was added, as then I would compare this removal with the additions in the original patch.
Comment 4 John Abd-El-Malek 2009-09-09 09:43:35 PDT
(In reply to comment #3)
> It's difficult for me to tell at first glance if all of this code is only used
> by Chromium or not.  I guess it would be slightly easier if we referenced the
> original revision where this all was added, as then I would compare this
> removal with the additions in the original patch.

Here's the original patch: http://trac.webkit.org/changeset/39115

The code has changed a bit since then through refactoring/cleaning up by others.

Thanks
Comment 5 Dimitri Glazkov (Google) 2009-09-10 16:07:03 PDT
Comment on attachment 39215 [details]
Proposed patch

sounds good.
Comment 6 Dimitri Glazkov (Google) 2009-09-10 16:07:22 PDT
Comment on attachment 39215 [details]
Proposed patch

jam will commit by hand.
Comment 7 Darin Adler 2009-09-10 16:54:13 PDT
I'm pretty sure this patch changed the behavior of Safari on Windows. Was that intentional? Was it correct?
Comment 8 John Abd-El-Malek 2009-09-10 17:20:22 PDT
(In reply to comment #7)
> I'm pretty sure this patch changed the behavior of Safari on Windows. Was that
> intentional? Was it correct?

This would only be the case if Safari provides a generic ActiveX host control, i.e. one that supports the "application/x-oleobject" mime type.  I don't think that's the case?  Chrome used to do that as an experiment in web compat, but it hurt more than it helped, so we removed it.
Comment 9 Darin Adler 2009-09-10 17:23:26 PDT
(In reply to comment #8)
> This would only be the case if Safari provides a generic ActiveX host control,
> i.e. one that supports the "application/x-oleobject" mime type.  I don't think
> that's the case?  Chrome used to do that as an experiment in web compat, but it
> hurt more than it helped, so we removed it.

Of course Safari does not provide such a thing, but I had the impression that it could be a plug-in that provides it, not Safari itself.

I guess I didn't realize this was a Chrome-specific experiment. If so, then I think the ifdefs were done wrong, because the hook was included in all WebKit browsers, not just Chrome. I guess it was included in the non-Chrome versions of WebKit for no good reason, and there's a good chance nobody tried to use this hook on other platforms, so it's probably OK to take it out.
Comment 10 John Abd-El-Malek 2009-09-10 17:41:39 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > This would only be the case if Safari provides a generic ActiveX host control,
> > i.e. one that supports the "application/x-oleobject" mime type.  I don't think
> > that's the case?  Chrome used to do that as an experiment in web compat, but it
> > hurt more than it helped, so we removed it.
> 
> Of course Safari does not provide such a thing, but I had the impression that
> it could be a plug-in that provides it, not Safari itself.
> 
> I guess I didn't realize this was a Chrome-specific experiment. If so, then I
> think the ifdefs were done wrong, because the hook was included in all WebKit
> browsers, not just Chrome. I guess it was included in the non-Chrome versions
> of WebKit for no good reason, and there's a good chance nobody tried to use
> this hook on other platforms, so it's probably OK to take it out.

I'm not aware of any NPAPI plugin that registers for that mime type, since it's really a generic type used for ActiveX.  I'm quite certain no other WebKit port is crazy enough to try this, but if there are any regressions we can revisit this.
Comment 11 John Abd-El-Malek 2009-09-11 10:29:32 PDT
Committed in r48274.