Fix regressions in preferences in Chrome

[Rob--W]

Fixes #9489

The two commits can be reviewed in isolation, see their commit messages.

I've tested this PR by opening the preferences page and observing that the preferences are back. Also, I will shortly open a new bug report to prevent such regressions from happening again.

[Snuffleupagus]

It might be a good idea to mention here that this mode is still somewhat experimental, and might not be fully stable yet; as noted in https://github.com/mozilla/pdf.js/pull/9490/files#diff-a0d82fd09027b92c6e7bed99dc735626R90.

[Rob--W]

I decided against appending (experimental) because then the dropdown item would become too wide. In #7584 I do not see any open issues that significantly degrade the user experience, so I think that it's safe to expose the enhanced text layer option without declaring it experimental.

Why do we feel the need to explicitly add "experimental" to this option? If it's good enough to use in production, then there is no need for the label. If there are known issues, then they should be reported in an issue and linked to the meta bug.

[Snuffleupagus]

In #7584 I do not see any open issues that significantly degrade the user experience, so I think that it's safe to expose the enhanced text layer option without declaring it experimental.

I'm not sure that I agree with that assessment, since the open TODOs are related to performance.
More generally the performance of the "enhanced" text-selection mode might not be that good in some edge-cases, in particular it might be quite bad in the "one div per character" case (i.e. for badly generated files where we're not able to coalesce the text).
Also, we've got fairly limited test-coverage currently, so it's a bit difficult to tell what the real state of the enhanced text-selection mode really is in my opinion.

However if you're not concerned too concerned about any of the above points, I suppose that you can keep the existing wording! Do as you wish here, I don't care too much :-)

[Rob--W]

Fair points there. I have reworded it to "Enable enhanced mode (experimental)" (and omitted "text selection" for brevity).

[Snuffleupagus]

Shouldn't the condition be items.textLayerMode === 1 here, given that otherwise a non-default textLayerMode (as set by the user) will always be overwritten which seem strange!?

[Rob--W]

The code here is only concerned with reading and parsing managed preferences. When getPreferences(items) is called, the given items specify the values to retrieve, along with the default values if no explicit value is set by the user. Lines 308 - 310 briefly state the purpose of the code (without explicitly stating the implementation details that I've written here).

[Snuffleupagus]

How about using an all-caps DEPRECATED string instead, here and elsewhere, to make it stand out a bit more?

[Rob--W]

Sounds good to me.

[Rob--W]

@Snuffleupagus Thanks for the review. I have amended all commits with the requested changes, this is the summary of changes:

diff --git a/extensions/chromium/preferences_schema.json b/extensions/chromium/preferences_schema.json
index e0e69b5d..46d2d0c6 100644
--- a/extensions/chromium/preferences_schema.json
+++ b/extensions/chromium/preferences_schema.json
@@ -27,7 +27,7 @@
       "default": 0
     },
     "enableHandToolOnLoad": {
-      "description": "Deprecated. Set cursorToolOnLoad to 1 to enable the hand tool by default.",
+      "description": "DEPRECATED. Set cursorToolOnLoad to 1 to enable the hand tool by default.",
       "type": "boolean",
       "default": false
     },
@@ -76,12 +76,12 @@
       "default": false
     },
     "disableTextLayer": {
-      "description": "Deprecated. Set textLayerMode to 0 to disable the text selection layer by default.",
+      "description": "DEPRECATED. Set textLayerMode to 0 to disable the text selection layer by default.",
       "type": "boolean",
       "default": false
     },
     "enhanceTextSelection": {
-      "description": "Deprecated. Set textLayerMode to 2 to use the enhanced text selection layer by default.",
+      "description": "DEPRECATED. Set textLayerMode to 2 to use the enhanced text selection layer by default.",
       "type": "boolean",
       "default": false
     },
diff --git a/gulpfile.js b/gulpfile.js
index b1262cfa..3568da31 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -194,10 +194,10 @@ function checkChromePreferencesFile(chromePrefsPath, webPrefsPath) {
   var chromePrefsKeys = Object.keys(chromePrefs.properties);
   chromePrefsKeys = chromePrefsKeys.filter(function (key) {
     var description = chromePrefs.properties[key].description;
-    // Deprecated keys are allowed. The code maintained is responsible for
-    // adding migration logic to
+    // Deprecated keys are allowed in the managed preferences file.
+    // The code maintained is responsible for adding migration logic to
     // extensions/chromium/options/migration.js and web/chromecom.js .
-    return !description || !description.startsWith('Deprecated.');
+    return !description || !description.startsWith('DEPRECATED.');
   });
   chromePrefsKeys.sort();
   var webPrefs = JSON.parse(fs.readFileSync(webPrefsPath).toString());
diff --git a/web/chromecom.js b/web/chromecom.js
index 238018cc..a4605b35 100644
--- a/web/chromecom.js
+++ b/web/chromecom.js
@@ -320,15 +320,21 @@ class ChromePreferences extends BasePreferences {
 
         chrome.storage.managed.get(defaultManagedPrefs, function(items) {
           items = items || defaultManagedPrefs;
-          // Migration code for https://github.com/mozilla/pdf.js/pull/7635.
+          // Migration logic for deprecated preferences: If the new preference
+          // is not defined by an administrator (i.e. the value is the same as
+          // the default value), and a deprecated preference is set with a
+          // non-default value, migrate the deprecated preference value to the
+          // new preference value.
           // Never remove this, because we have no means of modifying managed
           // preferences.
+
+          // Migration code for https://github.com/mozilla/pdf.js/pull/7635.
           if (items.enableHandToolOnLoad && !items.cursorToolOnLoad) {
-            // if the old enableHandToolOnLoad has a non-default value,
-            // and cursorToolOnLoad has a default value, migrate.
             items.cursorToolOnLoad = 1;
           }
           delete items.enableHandToolOnLoad;
+
+          // Migration code for https://github.com/mozilla/pdf.js/pull/9479.
           if (items.textLayerMode !== 1) {
             if (items.disableTextLayer) {
               items.textLayerMode = 0;
@@ -338,6 +344,7 @@ class ChromePreferences extends BasePreferences {
           }
           delete items.disableTextLayer;
           delete items.enhanceTextSelection;
+
           getPreferences(items);
         });
       } else {

[Snuffleupagus]

Let's fold the Clearly mark "enhanced text selection" as "experimental" commit into the base one instead, i,e. [CRX] Make textLayerMode pref visible and add migration logic.

r=me with that fixed; thanks for the patch!