Rewrite 'Preferences' to make it async #4482

Snuffleupagus · 2014-03-18T17:37:57Z

/cc @brendandahl Please look at this, and see if I'm on the right track with the rewrite.

This PR makes a number of changes to Preferences:

Remove the class-like implementation, so that it's instead invoked once when the viewer loads.

This change is necessary if we want to support preferences for the viewer itself, and not just when opening documents. (The second commit makes use of this to add a pref that enables the hand tool by default.)
Changes all functions to be asynchronous.
Adds docs. Some of it might be unnecessary or too verbose, so feedback is welcome on this.

brendandahl · 2014-03-18T20:45:26Z

Getting better, but it can be made simpler if you use promises to their fullest and remember that .then creates a promise and is resolved with whatever is returned in that function(or if you return a promise, that promise's resolved value). For example the set function:

  set: function preferencesSet(name, value) {
    return this.initializedPromise.then(function () {
      if (DEFAULT_PREFERENCES[name] === undefined) {
        throw new Error('preferencesSet: \'' + name + '\' is undefined.');
      } else if (value === undefined) {
        throw new Error('preferencesSet: no value is specified.');
      }
      var valueType = typeof value;
      var defaultType = typeof DEFAULT_PREFERENCES[name];

      if (valueType !== defaultType) {
        if (valueType === 'number' && defaultType === 'string') {
          value = value.toString();
        } else {
          throw new Error('Preferences_set: \'' + value + '\' is a \"' +
                           valueType + '\", expected \"' + defaultType + '\".');
        }
      } else {
        if (valueType === 'number' && (value | 0) !== value) {
          throw new Error('Preferences_set: \'' + value +
                           '\' must be an \"integer\".');
        }
      }
      this.prefs[name] = value;
      return this._writeToStorage(this.prefs); // <<!! should return promise
    }.bind(this));
  },

brendandahl · 2014-03-18T20:46:43Z

The above is also nice since you don't have to worry about rejecting the promise and can use regular exceptions.

Snuffleupagus · 2014-03-19T11:54:11Z

@brendandahl Good points, thanks for the quick feedback!
I've pushed an updated version, that I hope should address your comments.

yurydelendik · 2014-03-26T15:05:34Z

web/firefoxcom.js

-  FirefoxCom.requestSync('setPreferences', prefObj);
+Preferences._writeToStorage = function (prefObj) {
+  return new Promise(function (resolve) {
+    FirefoxCom.requestSync('setPreferences', prefObj);


can you make it with async "request" call?

~~There seems to be something wrong with the request method, I'll check if it's easy to address.~~

Edit: Ignore the above, I apparently missed that you have to change the methods in PdfStreamConverter.jsm for this to work properly. Revised patch coming up.

Done, thanks for catching this!

Snuffleupagus · 2014-03-28T10:34:34Z

@brendandahl Could you please look at this again, and see if I have managed to address all of your comments.

Slightly off-topic: The main reason that prompted me to rewrite this, was actually to make it possible to easily add a pref to enable/disable WebGL (once #4286 lands).

yurydelendik · 2014-04-02T13:11:59Z

/botio-linux preview

pdfjsbot · 2014-04-02T13:11:59Z

From: Bot.io (Linux)

Received

Command cmd_preview from @yurydelendik received. Current queue size: 3

Live output at: http://107.21.233.14:8877/31d10bc354d18c5/output.txt

pdfjsbot · 2014-04-02T13:30:13Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/31d10bc354d18c5/output.txt

Total script time: 0.41 mins

Published

Viewer: http://107.21.233.14:8877/31d10bc354d18c5/web/viewer.html
B2G Viewer: http://107.21.233.14:8877/31d10bc354d18c5/extensions/b2g/content/web/viewer.html
Extension: http://107.21.233.14:8877/31d10bc354d18c5/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/31d10bc354d18c5/extensions/firefox/pdf.js.amo.xpi

yurydelendik · 2014-04-02T14:18:38Z

web/preferences.js

+ *   or every time the viewer is loaded.
+ */
+var Preferences = {
+  prefs: {},


Will prefs: Object.create(DEFAULT_PREFERENCES), be better?

I don't know, what's the advantage of doing that?

I was thinking you would want to populate prefs with default values

The way things currently are set up (this wasn't changed by this PR), is that if/when a pref doesn't exist in prefs, it's just fetched from DEFAULT_PREFERENCES instead.

Okay, I was just wondering just that, {} has meaning of the new object -- I would expect null in this case.

I would expect null in this case.

That would complicate the code more, and after thinking about this I've reached the conclusion that it's probably nicer to do what you suggested initially anyway.

yurydelendik · 2014-04-02T14:21:20Z

Looks good after the comments above addressed (and removing [wip] from the title will help a little :)

yurydelendik · 2014-04-02T15:07:48Z

/botio-linux preview

pdfjsbot · 2014-04-02T15:07:49Z

From: Bot.io (Linux)

Received

Command cmd_preview from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/86e4ded51fd7f81/output.txt

pdfjsbot · 2014-04-02T15:08:14Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/86e4ded51fd7f81/output.txt

Total script time: 0.43 mins

Published

Viewer: http://107.21.233.14:8877/86e4ded51fd7f81/web/viewer.html
B2G Viewer: http://107.21.233.14:8877/86e4ded51fd7f81/extensions/b2g/content/web/viewer.html
Extension: http://107.21.233.14:8877/86e4ded51fd7f81/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/86e4ded51fd7f81/extensions/firefox/pdf.js.amo.xpi

… loads

yurydelendik · 2014-04-03T13:23:18Z

/botio-windows preview

pdfjsbot · 2014-04-03T13:23:19Z

From: Bot.io (Windows)

Received

Command cmd_preview from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/4da7afe83358619/output.txt

pdfjsbot · 2014-04-03T13:23:49Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/4da7afe83358619/output.txt

Total script time: 0.51 mins

Published

Viewer: http://107.22.172.223:8877/4da7afe83358619/web/viewer.html
B2G Viewer: http://107.22.172.223:8877/4da7afe83358619/extensions/b2g/content/web/viewer.html
Extension: http://107.22.172.223:8877/4da7afe83358619/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.22.172.223:8877/4da7afe83358619/extensions/firefox/pdf.js.amo.xpi

yurydelendik · 2014-04-03T13:32:11Z

Looks good, thank you

Rewrite 'Preferences' to make it async

Snuffleupagus · 2014-04-03T14:30:40Z

@brendandahl and @yurydelendik Thanks to the both of you for your help with this PR!

yurydelendik reviewed Mar 26, 2014
View reviewed changes

yurydelendik reviewed Apr 2, 2014
View reviewed changes

Snuffleupagus mentioned this pull request Apr 2, 2014

Add callback to asyncStorage.setItem stub mozilla/botio-files-pdfjs#12

Merged

Snuffleupagus added 3 commits April 3, 2014 10:08

Rewrite 'Preferences' to make it async

c920372

Modify {get, set}Preferences in PdfStreamConverter.jsm to support async

4c9c43f

Add preference to automatically enable the Hand Tool when the viewer…

42f9541

… loads

Snuffleupagus changed the title ~~[WIP] Rewrite 'Preferences' to make it async~~ Rewrite 'Preferences' to make it async Apr 3, 2014

yurydelendik added a commit that referenced this pull request Apr 3, 2014

Merge pull request #4482 from Snuffleupagus/prefs-async-v2

3e17021

Rewrite 'Preferences' to make it async

yurydelendik merged commit 3e17021 into mozilla:master Apr 3, 2014

Snuffleupagus deleted the prefs-async-v2 branch April 3, 2014 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite 'Preferences' to make it async #4482

Rewrite 'Preferences' to make it async #4482

Snuffleupagus commented Mar 18, 2014

brendandahl commented Mar 18, 2014

brendandahl commented Mar 18, 2014

Snuffleupagus commented Mar 19, 2014

yurydelendik Mar 26, 2014

Snuffleupagus Mar 26, 2014

Snuffleupagus Mar 26, 2014

Snuffleupagus commented Mar 28, 2014

yurydelendik commented Apr 2, 2014

pdfjsbot commented Apr 2, 2014

pdfjsbot commented Apr 2, 2014

yurydelendik Apr 2, 2014

Snuffleupagus Apr 2, 2014

yurydelendik Apr 2, 2014

Snuffleupagus Apr 2, 2014

yurydelendik Apr 2, 2014

Snuffleupagus Apr 3, 2014

yurydelendik commented Apr 2, 2014

yurydelendik commented Apr 2, 2014

pdfjsbot commented Apr 2, 2014

pdfjsbot commented Apr 2, 2014

yurydelendik commented Apr 3, 2014

pdfjsbot commented Apr 3, 2014

pdfjsbot commented Apr 3, 2014

yurydelendik commented Apr 3, 2014

Snuffleupagus commented Apr 3, 2014

Rewrite 'Preferences' to make it async #4482

Rewrite 'Preferences' to make it async #4482

Conversation

Snuffleupagus commented Mar 18, 2014

brendandahl commented Mar 18, 2014

brendandahl commented Mar 18, 2014

Snuffleupagus commented Mar 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Mar 28, 2014

yurydelendik commented Apr 2, 2014

pdfjsbot commented Apr 2, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Apr 2, 2014

From: Bot.io (Linux)

Success

Published

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Apr 2, 2014

yurydelendik commented Apr 2, 2014

pdfjsbot commented Apr 2, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Apr 2, 2014

From: Bot.io (Linux)

Success

Published

yurydelendik commented Apr 3, 2014

pdfjsbot commented Apr 3, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Apr 3, 2014

From: Bot.io (Windows)

Success

Published

yurydelendik commented Apr 3, 2014

Snuffleupagus commented Apr 3, 2014