Remove obsolete Mac quirk code

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: stefanh, Assigned: phil_bystrican, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla50
x86
macOS
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
QUIRK_FLASH_AVOID_CGMODE_CRASHES was only used for older versions of Mac OS X and can be removed.

Comment 1

3 years ago
Short guide to this bug: Use this link to find all the places in the code which mention this quirk:

http://searchfox.org/mozilla-central/search?q=QUIRK_FLASH_AVOID_CGMODE_CRASHES&case=false&regexp=false&path=

Remove each reference and any conditional blocks that still branch on this quirk.
Mentor: spohl.mozilla.bugs
Keywords: good-first-bug

Updated

3 years ago
Priority: -- → P3
Assignee

Comment 2

3 years ago
I would be willing to take this as my first bug if someone could assign it to me.
Hi Phil, thanks for your help! I've assigned the bug to you. I'm the assigned mentor for this bug, so please feel free to ask me if you have any questions. You can also find me on IRC as spohl.
Assignee: nobody → phil_bystrican
Assignee

Updated

3 years ago
Attachment #8773147 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8773147 [details] [diff] [review]
Removed definition and usage of QUIRK_FLASH_AVOID_CGMODE_CRASHES. Adjusted the bit shifts for quirk values that fell below it to keep the increment between them consistent

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

This all looks good so far, thanks! We should use this opportunity to clean up PluginUtilsOSX::GetCGLayer[1] and CGBridgeLayer[2] since they no longer need the |aAvoidCGCrashes| parameter. I noticed that we've already been passing |false| unconditionally[3], so there's really no need to keep this around. Could you upload a new patch with this addressed? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginUtilsOSX.mm#136
[2] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginUtilsOSX.mm#25
[3] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceChild.cpp#3531
Attachment #8773147 - Flags: review?(spohl.mozilla.bugs)
Assignee

Comment 6

3 years ago
Thanks Stephen! I'll upload another patch addressing those issues soon.
I forgot to mention: Please add "r=spohl" (without the quotes) at the end of the commit message in your patch. This will allow us to set a "checkin-needed" flag on your patch when it's ready to be committed to our source repository and our Sheriffs will be able to commit your patch without having to edit the commit message. Thanks!
Assignee

Comment 8

3 years ago
Okay I'll make sure to include that in the next one. On another note, this second set of changes should be a completely new patch that doesn't include the changes of that last one correct? I'm thinking this because this change doesn't appear to rely on any of my last changes so it seems like it can stand alone.
This bug calls for the removal of obsolete code and the changes are still small enough that it makes sense to just include it all in one patch. It also makes it easier to review. If you strongly prefer having two patches, that's fine. But I would typically reserve this for larger changes.
Assignee

Comment 10

3 years ago
CGBridgeLayer was only ever constructed with aAvoidCGCrashes as false so its property mAvoidCGCrashes was removed and all conditionals that relied on it were removed. This also resulted in protectLastCGContext never doing any work so it was removed as well. r=spohl
Assignee

Updated

3 years ago
Attachment #8773147 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8773369 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8773369 [details] [diff] [review]
Removed definition and usage of QUIRK_FLASH_AVOID_CGMODE_CRASHES. Adjusted the bit shifts for quirk values that fell below it to keep the increment between them consistent

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

Almost there. Once these two last comments have been addressed, I'll send this to our try server to compile and test on all platforms. Thanks!

::: dom/plugins/ipc/PluginUtilsOSX.mm
@@ +29,1 @@
>    CGContextRef mLastCGContext;

|mLastCGContext| can be removed throughout.

@@ +88,5 @@
>  }
>  
>  @end
>  
> +void* mozilla::plugins::PluginUtilsOSX::GetCGLayer(DrawPluginFunc aFunc, void* aPluginInstance, double aContentsScaleFactor) {

nit: Since we're touching this line, please change this to 80 characters or less per line. The following would work (with the params aligned vertically. bugzilla may not show this correctly):

void*
mozilla::plugins::PluginUtilsOSX::GetCGLayer(DrawPluginFunc aFunc,
                                             void* aPluginInstance,
                                             double aContentsScaleFactor) {
Attachment #8773369 - Flags: review?(spohl.mozilla.bugs)
Assignee

Comment 12

3 years ago
CGBridgeLayer was only ever constructed with aAvoidCGCrashes as false so its property mAvoidCGCrashes was removed and all conditionals that relied on it were removed. This also resulted in protectLastCGContext never doing any work so it was removed as well. r=spohl
Assignee

Updated

3 years ago
Attachment #8773369 - Attachment is obsolete: true
Comment on attachment 8773882 [details] [diff] [review]
Removed definition and usage of QUIRK_FLASH_AVOID_CGMODE_CRASHES. Adjusted the bit shifts for quirk values that fell below it to keep the increment between them consistent

Thanks!
Attachment #8773882 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 15

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3943bab61628
Removed definition and usage of QUIRK_FLASH_AVOID_CGMODE_CRASHES. Adjusted the bit shifts for quirk values that fell below it to keep the increment between them consistent. r=spohl
Keywords: checkin-needed

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3943bab61628
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.