1284939 - Remove obsolete Mac quirk code

Status: RESOLVED FIXED

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

Description

[Stefan [:stefanh]]

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

Comment 1

[Benjamin Smedberg]

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.

Comment 2

[Phil Bystrican]

I would be willing to take this as my first bug if someone could assign it to me.

Comment 3

[Stephen A Pohl [:spohl]]

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.

Comment 4

[Phil Bystrican]

Attachment: 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

Comment 5

[Stephen A Pohl [:spohl]]

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

Comment 6

[Phil Bystrican]

Thanks Stephen! I'll upload another patch addressing those issues soon.

Comment 7

[Stephen A Pohl [:spohl]]

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!

Comment 8

[Phil Bystrican]

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.

Comment 9

[Stephen A Pohl [:spohl]]

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.

Comment 10

[Phil Bystrican]

Attachment: 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

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

Comment 11

[Stephen A Pohl [:spohl]]

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) {

Comment 12

[Phil Bystrican]

Attachment: 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

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

Comment 13

[Stephen A Pohl [:spohl]]

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!

Comment 14

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69797141683d

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3943bab61628