The preferred property of CAFrameRateRange is a Float and is not optional. The nil-coalescing operator ?? 0 is unnecessary and should be removed to improve code clarity.

That's incorrect. This property is optional and eliminating the null-coalescing operator causes a compile error:

./flutter/shell/platform/darwin/ios/framework/Source/VSyncClientTest.swift:46:58: error: value of optional type 'Float?' must be unwrapped to a value of type 'Float'
 44 |     if #available(iOS 15.0, *) {
 45 |       XCTAssertEqual(Double(link.preferredFrameRateRange.maximum), maxFrameRate, accuracy: 0.1)
 46 |       XCTAssertEqual(Double(link.preferredFrameRateRange.preferred), maxFrameRate, accuracy: 0.1)
    |                                                          |- error: value of optional type 'Float?' must be unwrapped to a value of type 'Float'
    |                                                          |- note: coalesce using '??' to provide a default when the optional value contains 'nil'
    |                                                          `- note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
 47 |       XCTAssertEqual(Double(link.preferredFrameRateRange.minimum), maxFrameRate / 2, accuracy: 0.1)
 48 |     } else {

This initialiser used to hardcode class method calls on FlutterDisplayLinkManager, which necessitated mocking them using OCMock in tests.

I've added the new designated initialiser above with additional parameters that we can inject directly in tests and avoid OCMock and kept this one for backward compatibility for now.

I have a followup that deletes this and injects the values explicitly at each of the call sites, which is clearer/more readable.

See the note in the implementation file on the rationale for the new initialiser.

this part is a bit confusing that it passes FlutterDisplayLinkManager.displayRefreshRate into maxRefreshRate?

I've named it that because that's effectively what this parameter does: it sets the maximum frame rate.

If you take a look at the implementation, it serves two purposes:

• We set the initial refreshRate.
• We immediately call setMaxRefreshRate to set the maximum refresh rate on the CADisplayLink under the hood in FlutterDisplayLinkManager.

Separate issue is that we're making a self call in the initializer but that's an existing issue; this just makes it possible to inject the param.

does _isVariableRefreshRateEnabled mean it will use 60hz by default, but may increase to 120hz if needed? or does it mean the opposite (use 120hz by default, but decrease to 60hz if needed?)

This is a future refactoring leaking through. FlutterDisplayLink.maxRefreshRateEnabledOnIPhone is a badly named API. If you take a look at the implementation, it checks if CADisableMinimumFrameDurationOnPhone is set. If it is, the frame rate is locked to 60 Hz. If not, ProMotion is enabled (i.e. variable refresh rate up to 120 Hz).

There's a followup patch that renames FlutterDisplayLinkManager.maxRefreshRateEnabledOnIPhone to FlutterDisplayManager.isVariableRefreshRateEnabled since that's the explicitly documented behaviour.

From Apple's docs:

A Boolean value that allows your app to access frame rates higher than the system’s default.

Devices with ProMotion displays allow apps to dynamically request a frame rate they prefer. If you set this key to YES, your app can request any frame rate the display supports. If you set this key to NO, frame rates higher than the system default are unavailable.

For more information on refresh rates, see Optimizing iPhone and iPad apps to support ProMotion displays.

There's a deep dive on variable refresh rate support at: WWDC 2021: Optimize for variable refresh rate displays

sanity check - these being 0 doesn't mean it's not refreshing right? it simply means it will refresh at 120hz always? (or 60hz always?)

Correct, from Apple's docs, this preferredFramesPerSecond/preferredFrameRateRange.preferred being 0 just means max display rate:

The property defaults to 0, which is equivalent to the display’s maximum refresh rate, such as a UIScreen instance’s maximumFramesPerSecond property.

These are just a rewrite of the existing tests with no change.

is this test identical to previous one?

Yep this is identical. I assume you mean the setup of the client at the top?

Here's the old code:

  void (^callback)(CFTimeInterval, CFTimeInterval) =
      ^(CFTimeInterval startTime, CFTimeInterval targetTime) {
      };
  id mockDisplayLinkManager = [OCMockObject mockForClass:[FlutterDisplayLinkManager class]];
  double maxFrameRate = 120;
  [[[mockDisplayLinkManager stub] andReturnValue:@(maxFrameRate)] displayRefreshRate];

  // VSyncClient with empty block and max frame rate of 120 Hz (via the OCMock).
  FlutterVSyncClient* vsyncClient =
      [[FlutterVSyncClient alloc] initWithTaskRunner:self.threadTaskRunner callback:callback];

And the new code:

  // VSyncClient with empty block and max frame rate of 120 Hz.
  let maxFrameRate: Double = 120.0
  let vsyncClient = VSyncClient(
    taskRunner: threadTaskRunner,
    isVariableRefreshRateEnabled: false,
    maxRefreshRate: maxFrameRate
  ) { _, _ in }!

The old code relied on the fact that we hardcoded a static FlutterDisplayLinkManager.displayRefreshRate call in it. Our fallback initialiser still does that, but the point of adding this new one was so we can just inject directly (I have a followup that eliminates the old one altogether because it's pretty confusing.

The tests at the bottom are identical as well.

Sorry i meant are these tests identical:

• testDoNotSetVariableRefreshRatesIfCADisableMinimumFrameDurationOnPhoneIsNotOn
• testDoNotSetVariableRefreshRatesIfCADisableMinimumFrameDurationOnPhoneIsNotSet

Ah good catch they are now.

With the previous version of the code that hardcoded the lookup in the initializer there were two ways it could be set to NO:

• the IsNotOn test: mocked the NSBundle to return NO for the CADisableMinimumFrameDurationOnPhone key.
• the IsNotSet test: simulated the case where the key is completely missing from the info.plist

Since we now inject the bool we only need one. Removed the other.