Get rid of TVector glue in Default Plugin on Mac/Intel

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
Plug-ins
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Mark Mentovai, Assigned: Mark Mentovai)

Tracking

({fixed1.8.0.1, fixed1.8.1, fixed1.8.1.12})

1.8 Branch
mozilla1.8.1
PowerPC
Mac OS X
fixed1.8.0.1, fixed1.8.1, fixed1.8.1.12
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

1.25 KB, patch
Josh Aas
: review+
Benjamin Smedberg
: approval1.8.0.1+
Benjamin Smedberg
: approval1.8.1+
Details | Diff | Splinter Review
17.45 KB, patch
Mark Mentovai
: review+
Mark Mentovai
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.8.1.12+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
The Default Plugin has ugly TVector glue for the CFM interface.  This is appropriate on ppc, but it's now wrong on x86.  On x86, this needs to change to use raw native Mach-O the whole way.

http://lxr.mozilla.org/mozilla/source/modules/plugin/samples/default/mac/npmac.cpp#92

Note: Default Plugin is now disabled on Firefox.  (Why is it still packaged and shipped?)
(Assignee)

Comment 1

12 years ago
Created attachment 202676 [details] [diff] [review]
Patch

This is Mac-only code, so there's no problem getting rid of XP_MACOSX.
Attachment #202676 - Flags: review?(joshmoz)

Updated

12 years ago
Attachment #202676 - Flags: review?(joshmoz) → review+

Comment 2

12 years ago
On checkin, please expand up the following comment to explain the intel/ppc function pointer situation:

// glue for mapping outgoing Macho function pointers to TVectors

Lots of people use that plugin as a reference.
(Assignee)

Comment 3

12 years ago
Fix checked in.  Leaving open for subsequent TARGET_CARBON cleanup.
(Assignee)

Comment 4

12 years ago
Created attachment 203446 [details] [diff] [review]
Miscellaneous dead-code cleanup
Attachment #203446 - Flags: review?(joshmoz)

Comment 5

12 years ago
Comment on attachment 203446 [details] [diff] [review]
Miscellaneous dead-code cleanup

Yeah, I think we can probably get rid of the 68k mac code now. I didn't compile this myself, I trust you on this one. Looks good.
Attachment #203446 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

12 years ago
Attachment #203446 - Flags: superreview?(mikepinkerton)
Comment on attachment 203446 [details] [diff] [review]
Miscellaneous dead-code cleanup

sr=pink
Attachment #203446 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 7

12 years ago
Created attachment 207005 [details] [diff] [review]
Miscellaneous dead-code cleanup, as checked in

Checked in on the trunk.
Attachment #203446 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.8.0.1?
Resolution: --- → FIXED
(Assignee)

Comment 8

12 years ago
Comment on attachment 202676 [details] [diff] [review]
Patch

Need this patch for DefaultPlugin to be compatible with x86 Macs.  DefaultPlugin is disabled in the standard Firefox configuration, but is shipped and can be enabled.  DefaultPlugin IS used by other products.

Only requesting approval for the first patch here, the clean-up patch isn't necessary.
Attachment #202676 - Flags: approval1.8.0.1?

Comment 9

12 years ago
Comment on attachment 202676 [details] [diff] [review]
Patch

Add fixed1.8.1 and fixed1.8.0.1 when checked in.
Attachment #202676 - Flags: approval1.8.1+
Attachment #202676 - Flags: approval1.8.0.1?
Attachment #202676 - Flags: approval1.8.0.1+

Updated

12 years ago
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
(Assignee)

Comment 10

12 years ago
The first patch on this bug was checked in to the 1_8 and 1_8_0 branches.
Keywords: fixed1.8.0.1, fixed1.8.1
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 11

10 years ago
Comment on attachment 207005 [details] [diff] [review]
Miscellaneous dead-code cleanup, as checked in

This patch was r=josh sr=pink, and was checked into the trunk on 2005-12-28.  Since it's just cleanup, it was never checked in on the branch, although the code is dead on the branch too.  I'd like to take this patch on the branch now because the #include changes fix a problem that occurs when building the 1.8 branch using the native SDK on Mac OS X 10.5 ("Leopard").  I'd like to take this change on that branch because active development still occurs on that branch for Camino: Camino 1.6, just now approaching alpha, will be based on MOZILLA_1_8_BRANCH.
Attachment #207005 - Flags: approval1.8.1.10?
(Assignee)

Comment 12

10 years ago
Comment on attachment 207005 [details] [diff] [review]
Miscellaneous dead-code cleanup, as checked in

(carrying forward reviews from previous version - this is the version that was checked in)
Attachment #207005 - Flags: superreview+
Attachment #207005 - Flags: review+
Attachment #207005 - Flags: approval1.8.1.10? → approval1.8.1.11?
Does a Firefox build include any of this code? It'll be easier to approve if the answer is "NPOTB"
(Assignee)

Comment 14

10 years ago
Comment on attachment 207005 [details] [diff] [review]
Miscellaneous dead-code cleanup, as checked in

Everything that's removed is #if 0 on Firefox.  The compiled (preprocessed, actually) version of this source is the same before and after this patch.
Comment on attachment 207005 [details] [diff] [review]
Miscellaneous dead-code cleanup, as checked in

Thanks for the comment Mark.

Approved for 1.8.1.12; a=ss for release-drivers.
Attachment #207005 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Assignee)

Comment 16

10 years ago
Attachment 207005 [details] [diff] checked in on MOZILLA_1_8_BRANCH for 1.8.1.12.
Keywords: fixed1.8.1.12
You need to log in before you can comment on or make changes to this bug.