Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Define a protocol for scheme-handling plugins #1479

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Aug 26, 2024

Platforms affected

iOS

Motivation and Context

Existing behaviour for plugins to opt-in to handling custom scheme resource loads was not documented and there were some unsafe assumptions around only a single plugin handling all requests.

Description

Add a CDVPluginSchemeHandler protocol to allow plugins to declare that they want to intercept WebKit resource loads.
We don't rely on the protocol itself being implemented by the plugins (we continue to check with -respondsToSelector:) but this allows us to avoid objc_msgSend and provides a way to document some of this plugin behaviour that is not otherwise explained.

Also, use a NSMapTable to associate plugin instances with the request being handled. Since we loop over the plugins, there's a chance that multiple plugins might want to handle different requests (which sounds like a logistical nightmare, but it's definitely possible). Now we keep a map of which plugin is handling each request so that we can make sure the right plugin gets called when the resource load is cancelled. NSMapTable uses weak associations, so when the task is done and released, the key and value will be dropped from the table automatically.

This should also resolve the unsafe plugin iteration issue that was mentioned in GH-1272 and GH-1030 by always iterating over an array of plugin objects that is a copy (due to calling -allValues).

Testing

Existing tests pass. Tested manually in simulator with custom scheme.

Checklist

  • I've run the tests to see all new and existing tests pass

We don't rely on the protocol itself being implemented by the plugins
(we continue to check with `-respondsToSelector:`) but this allows us to
avoid `objc_msgSend` and provides a way to document some of this plugin
behaviour that is not otherwise explained.

This should also resolve the unsafe plugin iteration issue that was
mentioned in apacheGH-1272 and apacheGH-1030 by always iterating over an array of
plugin objects that is a copy (due to calling `-allValues`).
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.22%. Comparing base (458f5f3) to head (6ba4aca).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1479   +/-   ##
=======================================
  Coverage   80.22%   80.22%           
=======================================
  Files          16       16           
  Lines        1871     1871           
=======================================
  Hits         1501     1501           
  Misses        370      370           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dpogue dpogue added this to the 8.0.0 milestone Aug 27, 2024
@dpogue dpogue marked this pull request as ready for review August 27, 2024 10:51
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Tested and confirmed that a plugin with defined overrideSchemeTask is still executed.

@dpogue dpogue merged commit c3d5949 into apache:master Aug 28, 2024
10 checks passed
@dpogue dpogue deleted the plugin-protocols branch August 28, 2024 09:23
@dpogue dpogue mentioned this pull request Aug 28, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants