Closed Bug 1284939 Opened 5 years ago Closed 4 years ago

Remove obsolete Mac quirk code

Categories

(Core :: Plug-ins, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: stefanh, Assigned: phil_bystrican, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

QUIRK_FLASH_AVOID_CGMODE_CRASHES was only used for older versions of Mac OS X and can be removed.
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
Priority: -- → P3
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
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)
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!
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.
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
Attachment #8773147 - Attachment is obsolete: true
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)
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
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/3943bab61628
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.