fix: runtime select scripting or content scripts apis #324
Conversation
… determine if scripting API can be used by webcompat injections and shims.
…to inject webcompat content scripts
|
@wisniewskit this is the initial draft for the PR we discussed earlier today, in the PR description I've added some more details about the approach (in particular the new experimental API checking the pref and synchronously providing a boolean result for which API to pick). I've marked it as a draft while I'm working with my team to determine which option for the changes on the scripting API implementation side we are going to take, but in the meantime you may take a look to this version Also, thanks again for the help!!! ❤️ |
|
Push to try including this PR + one line change to default matchAboutBlank to false for webcompat in the short term (which is one of the options being considered, in alternative to explicitly introducing matchAboutBlank option in the scripting.registerContentScripts API method) + flipping the |
…olPrefSync method
|
Given that the new experimental API was mainly synchronously returning the value of a pref in the form it was when I created this PR, and both @wisniewskit and @willdurand were wondering if we could be rolling the new API method into one of the existing APIs, I decided to go on and roll that method into the aboutConfigPrefs experimental API (which is now including a separate aboutConfigPrefsChild.js which is the part of the API running in the child extension process and providing the getBoolPrefSync method which is meant to return the value of a webcompat pref synchrously (which for consistency with the getPref method is implicitly prefixing the pref name with Besides that tweak, the updated version of this PR is functionally the same as the version submitted yesterday. |
…Scritps API based on the useScriptingAPI pref value
rpl
force-pushed
the
fix/runtime-select-scripting-or-contentScripts-apis
branch
from
53bdcd3
to
556b8ad
Compare
|
@wisniewskit this PR is now officially open for review, the change on the scripting API internals side is tracked by Bug 1853412 - Set matchAboutBlank to false in scripting.registerContentScripts for webcompat. Bug 1853412 patches are NOT including adding the "extensions.webcompat.useScriptingAPI" pref set to true, because I thought that may better fit into a separate patch landed along with Bug 1853013, does that sound good to you? The pref should be set to true in If you add me as a reviewer on the patch that sets the pref I'll be happy to sign it off. As a side note, today I applied the following changes:
Besides these couple of changes the PR is the same as it was in its initial version yesterday. |
…nature to improve readability.
|
@wisniewskit The last change applied today is purely cosmetic, nothing changes in practice besides being more immediately visible that the second parameter is optional and defaults to false. I've also pushed to try a couple of patches (vendoring these changes in m-c + opt-in on using the scripting API) and linked it here in Bug 1853013 comment 8. |
| `${extensionPrefNameBase}${prefName}`, | ||
| defaultValue | ||
| ); | ||
| } catch (_) { |
There was a problem hiding this comment.
| } catch (_) { | |
| } catch { |
} catch { can be used instead of a dummy } catch (_) {.
There was a problem hiding this comment.
I'd leave it as is given that the other getPref method is written like this, they can eventually be both changed in a separate followup, in this PR I'd prefer to keep the two method as in sync as possible (as also mentioned in my answer to another review comment).
| try { | ||
| return Services.prefs.getBoolPref( | ||
| `${extensionPrefNameBase}${prefName}`, | ||
| defaultValue |
There was a problem hiding this comment.
Does the method ever throw if you pass defaultValue here? I don't think so, right? Just one of these should work: try-catch-return or getBoolPref+defaultValue
Given this, I would prefer this to be without try-catch, so that if there is a serious issue, that it would be caught (disregard this comment if we have automated test coverage that tests the behavior with and without the flag).
There was a problem hiding this comment.
under normal conditions there shouldn't be, but to be fair from a quick look to the internal C++ implementation it seems there are technically other kind of unexpected conditions that could lead to an error being raised (e.g. if Preferences::InitStaticMembers() failed, and if I'm reading it correctly also if there is an unexpected mismatch between the value actually set and the one we expect, boolean)
And given this is a more generic method that could be used in the future not just for the check of the extensions.webcompat.useScriptingAPI pref, I thought to keep the exact same convention used by the other similar method defined in aboutConfigPrefs.
personally I prefer the two method to look the same, and if changed then both to be changed to stay as consistent with each other as possible, and so I don't see that as strictly something to be done in this particular PR.
…ment part of aboutConfigPrefs.js
…e hit if the contentScripts API was selected
This PR is a followup to the previous set of changes introduced in #320, and part of handling the issue caught when we have been vendored the new webcompat version using the scripting API in mozilla-central (Bug 1853013).
The underlying issue is due to the scripting.registerContentScripts API currently forcefully setting matchAboutBlank internally to
true(as also described in Bug 1853013 comment 6.The approach we discussed and agreed on with @wisniewskit earlier today is composed of two parts:
one part of the approach is this set of changes on the webcompat side, this changes are meant to let webcompat to select either scripting or contentScripts API at runtime (and so this PR is largely about bringing back the support for contentScripts.register and select one of the API based on a call to a new experimental API, which will be synchronously provide a boolean result to determine if scripting should be used in the current browser instance)
the second part of the issue will have to apply some change on the
scriptingAPI implementation on the mozilla-central side, tracked by Bug 1853412 - Set matchAboutBlank to false in scripting.registerContentScripts for webcompatIn this first version of the patch I've brought back the code for using contentScripts.register by also trying to contain the changes in the same area that we have touched in #320 without a too big of a refactoring, there may be some more tweaks we can still apply without expanding the impact of the changes but we may also likely want to balance between what we could consider reasonable in this PR vs. in separate followups and so this version felt like a reasonable starting point.
TODO list
aboutConfigPrefs(as a newgetBoolPrefSyncmethod running in the child extension process)apply changes to add matchAboutBlank option if that is going to be the option we will choose for the changes applied on the scripting API implementation side(dropped as not actually needed, the option we are going for in the short term in the one tracked by Bug 1853412 - Set matchAboutBlank to false in scripting.registerContentScripts for webcompat and that doesn't need further changes to this PR.