Emit a notice when bss.gecko_android.strict_max_version is set

[willdurand]

Fixes #4855

┆Issue is synchronized with this Jira Task

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (56ff200) 98.75% compared to head (b5e2d72) 98.75%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4856   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files          54       54           
  Lines        2883     2884    +1     
  Branches      874      875    +1     
=======================================
+ Hits         2847     2848    +1     
  Misses         36       36           

Impacted Files	Coverage Δ
src/parsers/manifestjson.js	99.24% <100.00%> (+<0.01%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rpl]

In addition to the inline review comment below, while trying this branch on a test extension, I noticed another pre-existing issue we may want to think about (not necessarily in this PR, we may as well file a separate issue and handle it in a separate PR):

with a manifest which include bss.gecko_android and bss.gecko.strict_min_version (or applications.gecko.strict_min_version) is < 113 (when bsss.gecko_android has been introduced based on the browser-compat-data) we show the following two warnings:

KEY_FIREFOX_UNSUPPORTED_BY_MIN…   Manifest key not supported   "strict_min_version" requires Firefox 111.0, which   manifest.json            
_VERSION                          by the specified minimum     was released before version 113 introduced support                            
                                  Firefox version              for "browser_specific_settings.gecko_android".                                
KEY_FIREFOX_ANDROID_UNSUPPORTE…   Manifest key not supported   "strict_min_version" requires Firefox for Android    manifest.json            
D_BY_MIN_VERSION                  by the specified minimum     111.0, which was released before version 113                                  
                                  Firefox for Android          introduced support for                                                        
                                  version                      "browser_specific_settings.gecko_android".

And I was thinking that we may handle that by introducing a new firefoxAndroidStrictMinVersion in the utils.js module, then in checkKeySupport skip the if (support.firefox) { ... } block if the key is browser_specific_settings.gecko_android and then in the if (support.firefox_android) { ... } use the minVersion returned by firefoxAndroidStrictMinVersion, something like the changes in the following diff:

diff --git a/src/parsers/manifestjson.js b/src/parsers/manifestjson.js
index 0d6fb81a..5a86b22a 100644
--- a/src/parsers/manifestjson.js
+++ b/src/parsers/manifestjson.js
@@ -38,6 +38,7 @@ import JSONParser from 'parsers/json';
 import {
   basicCompatVersionComparison,
   firefoxStrictMinVersion,
+  firefoxAndroidStrictMinVersion,
   firstStableVersion,
   isToolkitVersionString,
   isValidVersionString,
@@ -144,7 +145,7 @@ export default class ManifestJSONParser extends JSONParser {
   }
 
   checkKeySupport(support, minVersion, key, isPermission = false) {
-    if (support.firefox) {
+    if (support.firefox && key !== "browser_specific_settings.gecko_android") {
       // We don't have to support gaps in the `@mdn/browser-compat-data`
       // information for Firefox Desktop so far.
       const versionAdded = support.firefox.version_added;
@@ -182,12 +183,13 @@ export default class ManifestJSONParser extends JSONParser {
       // not warn if the minVersion is in one of the few version gaps).
       const versionAddedAndroid = firstStableVersion(support.firefox_android);
 
-      if (basicCompatVersionComparison(versionAddedAndroid, minVersion)) {
+      const minVersionAndroid = firefoxAndroidStrictMinVersion(this.parsedJSON);
+      if (basicCompatVersionComparison(versionAddedAndroid, minVersionAndroid)) {
         if (!isPermission) {
           this.collector.addWarning(
             messages.keyFirefoxAndroidUnsupportedByMinVersion(
               key,
-              this.parsedJSON.applications.gecko.strict_min_version,
+              minVersionAndroid,
               versionAddedAndroid
             )
           );
@@ -195,7 +197,7 @@ export default class ManifestJSONParser extends JSONParser {
           this.collector.addNotice(
             messages.permissionFirefoxAndroidUnsupportedByMinVersion(
               key,
-              this.parsedJSON.applications.gecko.strict_min_version,
+              minVersionAndroid,
               versionAddedAndroid
             )
           );
diff --git a/src/utils.js b/src/utils.js
index 165cd83e..0b63d204 100644
--- a/src/utils.js
+++ b/src/utils.js
@@ -385,6 +385,22 @@ export function firefoxStrictMinVersion(manifestJson) {
   return null;
 }
 
+export function firefoxAndroidStrictMinVersion(manifestJson) {
+  if (
+    manifestJson.applications &&
+    manifestJson.applications.gecko_android &&
+    manifestJson.applications.gecko_android.strict_min_version &&
+    typeof manifestJson.applications.gecko_android.strict_min_version === 'string'
+  ) {
+    return parseInt(
+      manifestJson.applications.gecko_android.strict_min_version.split('.')[0],
+      10
+    );
+  }
+  // Fallback to gecko compatibility range.
+  return firefoxStrictMinVersion(manifestJson);
+}
+
 export function basicCompatVersionComparison(versionAdded, minVersion) {
   const asNumber = parseInt(versionAdded, 10);
   return !Number.isNaN(asNumber) && asNumber > minVersion;

I'm pretty sure some of the existing test case is going to fail with these changes, and so that would be one more reason to consider this separately from what this PR is aiming for, and in this one just handle the notice message on strict_max_version when included in gecko_android.

[rpl]

I have the feeling that here we should take into account that for a MV2 extensions there could be applications.gecko and here we would overwrite it with a bss that may only include browser_specific_settings.gecko_android and we wouldn't have the version range for gecko anymore in the final value set to this.parsedJSON.applications.

Given that we would be already reporting a blocking linting error if applications is included in an MV3 manifest,
we may just merge applications and browser_specific_settings here (so that we may sure to have merged both if applications.gecko was included in an MV2 extension along with bss.gecko_android), and so I was thinking to something like the following

    if (
      this.parsedJSON.browser_specific_settings &&
      (this.parsedJSON.browser_specific_settings.gecko ||
        this.parsedJSON.browser_specific_settings.gecko_android)
    ) {
      this.parsedJSON.applications = {
        ...(this.parsedJSON.applications || {}),
        ...this.parsedJSON.browser_specific_settings,
      };
    }

and a test case which make this hit that particular scenario.

[willdurand]

Yeah, I think that makes sense. I remember I wasn't thrilled about this change but I couldn't find why... Thanks!

[willdurand]

@rpl about the other pre-existing issue, this is covered by #4781, right? If so, can that be moved out of this PR? Thanks

[rpl]

lgtm (and thanks for adding the additional test case).

[rpl]

@rpl about the other pre-existing issue, this is covered by #4781, right? If so, can that be moved out of this PR? Thanks

yes, the pre-existing issue I mentioned in #4856 (review) looks definitely part of what we have filed #4781, from my perspective it is absolutely ok (and actually better) to move that part out of this PR and consider that comment as part of a separate PR meant to fix #4781 specifically.