We split off bug 744257 into three patches (client, storage were the first two); and this is the third and last piece which contains "main.js" and "manager.js" from the original patch.

Patch for AITC manager (handles polling, client interactions and BrowserID logins).

I'm trying to think what the best way to test this module is, ideas welcome!

Anant was just taking a look at the patch + self._pending.dequeue(function(err, done) { + if (err) { + self._log.error("Dequeue failed " + err); + } + _reschedule(); + return; I am thinking the reschedule would go one line up ? also some formatting issues right around these parts.

(In reply to dclarke@mozilla.com from comment #3) > Anant was just taking a look at the patch > > + self._pending.dequeue(function(err, done) { > + if (err) { > + self._log.error("Dequeue failed " + err); > + } > + _reschedule(); > + return; > General formatting issues, not so sure about the reschedule

(In reply to dclarke@mozilla.com from comment #3) > Anant was just taking a look at the patch > > + self._pending.dequeue(function(err, done) { > + if (err) { > + self._log.error("Dequeue failed " + err); > + } > + _reschedule(); > + return; > > I am thinking the reschedule would go one line up ? also some formatting > issues right around these parts. No, we have to reschedule for the next item in the queue, even if the dequeue failed. If a failure like this does occur, it's most likely it was intermittent. There is the possibility of getting into an infinite loop if the dequeue fails every single time, so there is some room for making this code more robust by not trying the same operation more than a few times. Then again, if a dequeue is failing all the time, enqueue is probably broken as well; and only a client restart can fix that. What are the formatting issues? I can't find any.

I take it back i got confused by the purpose of the enqueue / dequeue.

Comment on attachment 626034 [details] [diff] [review] AITC Manager RC1 Initial comments from first pass: * in addition to "page is loaded/not loaded" the client should also pay attention to the idle timer and not waste cycles/battery when inactive. How we do this in Sync is a good example: http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/policies.js#207 * as discussed on IRC, we should probably still check periodically in the background, if the user is authed (if not, obviously we won't do that), for better UX when launching the dashboard * given the above, userOnDashboard/userOffDashboard and dashboardLoaded/dashboardUnloaded feel a little abstract. these calls are effectively "start/stop checking for updates" now and if the second bullet is part of this it'll become "change update frequency" so I'd like to see that reflected in naming. it could be one call "changeUpdateFrequency(active/passive)" * there's a lot of duplication of the "enable/disable" polling logic. Seems like we could just have an "isDashboardActive" helper that checks to see if the frontmost tab has the dashboard loaded. ** The loop in setup will enable polling even if the dashboard isn't in the frontmost window * DASHBOARD should be DASHBOARD_URI for comprehension. main.js can just call the AitcManager one, rather than duplicating it. * At some point we should review the logging I/O question. Probably a release blocker, but needs a comprehensive solution for AITC, and it's relatively simple to do so afterwards. more in a bit, haven't really read manager.js all the way through

Comment on attachment 626034 [details] [diff] [review] AITC Manager RC1 >diff --git a/services/aitc/service.js b/services/aitc/service.js >--- a/services/aitc/service.js >+++ b/services/aitc/service.js > CommonUtils.namedTimer(function() { >- // Kick-off later! >+ // Start it up! >+ Cu.import("resource://services-aitc/main.js"); >+ new Aitc(); Why not just invoke this as a side-effect of the import? >+ /* Obtain a token from Sagrada token server, given a BrowserID assertion >+ * cb(err, token) will be invoked on success or failure. >+ */ >+ _getToken: function _getToken(assertion, cb) { >+ let url = this.TOKEN_SERVER + "/1.0/aitc/1.0"; >+ let client = new TokenServerClient(); >+ >+ let self = this; >+ this._log.info("Obtaining token from " + url); >+ client.getTokenFromBrowserIDAssertion(url, assertion, function(err, tok) { >+ if (!err) { >+ self._log.info("Got token from server: " + JSON.stringify(tok)); >+ cb(null, tok); >+ return; >+ } >+ >+ if (!err.response) { >+ self._log.error(err); >+ cb(err, null); >+ return; >+ } >+ if (!err.response.success) { >+ self._log.error(err); >+ cb(err, null); >+ return; >+ } >+ >+ let msg = "Unknown error in _getToken " + err.message; >+ self._log.error(msg); >+ cb(new Error(msg), null); >+ }); >+ }, * getTokenFromBrowserIDAssertion will throw on invalid input, make sure you catch this case, as cb will not be called here if a caller messes up * err is a TokenServerClientServerError, there is no .response on that object. I think the last three code blocks should just be: self._log.error(err.name + ": " + err.error); cb(err, null);

Nominating for k9o - this is part of the AITC client implementation

Can we just mark the tracker bug 744257 for k9o instead of every individual bug?

(In reply to Anant Narayanan [:anant] from comment #10) > Can we just mark the tracker bug 744257 for k9o instead of every individual > bug? Yeah, that's probably better, as the top tracking bug is the key bug to track.

Comment on attachment 626034 [details] [diff] [review] AITC Manager RC1 Review of attachment 626034 [details] [diff] [review]: ----------------------------------------------------------------- Overall nit: Lots of sentences in comments missing period. Also, review is incomplete. ::: services/aitc/modules/main.js @@ +25,5 @@ > + > + let self = this; > + this._manager = new AitcManager(function _managerDone() { > + self._init(); > + }); Any time you have a closure with a single function call, you can always just bind the function. this._manager = new AitcManager(this._init.bind(this)). @@ +92,5 @@ > + } > + > + // Add listeners for all windows opened in the future. > + function winWatcher(subject, topic) { > + if (topic != "domwindowopened") return; style nit: always include { } ::: services/aitc/modules/manager.js @@ +167,5 @@ > + notify: function _getTimerNotify() { > + self._checkServer(); > + }, > + }, getFreq, Ci.nsITimer.TYPE_REPEATING_SLACK > + ); How about this style? Slightly less ugly, IMO. this._getTimer.initWithCallback({notify: this._checkServer.bind(this)}, getFreq, Ci.nsITimer.TYPE_REPEATING_SLACK); @@ +182,5 @@ > + return; > + } > + this._getTimer.cancel(); > + this._getTimer = null; > + }, The APIs here feel a little clunky. How about a public startPoll and endPoll which either do the right thing or no-op? Maybe I'm not groking the interaction between windows, however. @@ +228,5 @@ > + /** > + * Go through list of apps to PUT and attempt each one. If we fail, try > + * again in PUT_FREQ. > + */ > + _processQueue: function _processPutQueue() { Nit: name mismatch. @@ +314,5 @@ > + > + // Haven't tried this PUT yet, do it immediately. > + self._log.info("Queue non-empty, processing next PUT"); > + self._processQueue(); > + } style: consider pulling out _reschedule and _clientCallback into module-level functions. ::: services/aitc/service.js @@ +34,5 @@ > Cu.import("resource://services-common/utils.js"); > CommonUtils.namedTimer(function() { > + // Start it up! > + Cu.import("resource://services-aitc/main.js"); > + new Aitc(); mconnor didn't like this on the grounds that it should be a side-effect of module load. I disagree. I don't like side-effects when loading modules. It breaks one of my best practices of minimizing the number of global/static variables. Globals and statics enforce a particular usage convention, hindering flexibility. While I agree we will probably never have more than 1 AITC instance, I still think it is best to explicitly create instance under your terms. Also, what's to prevent the Aitc instance from being GC'd? Should it be stored in a module-level variable just in case?

Should address most of mconnor's concerns, and some restructuring per discussion on IRC.

Comment on attachment 628157 [details] [diff] [review] AITC Manager RC2 main.js! looking good now, just mostly cleanup and speed tweaks. Next up manager.js! >diff --git a/services/aitc/modules/main.js b/services/aitc/modules/main.js >+Aitc.prototype = { >+ get IDLE_TIMER() { >+ return Preferences.get("services.aitc.main.idleTime"); >+ }, IDLE_INTERVAL or IDLE_TIMEOUT, since this is fetching a value, not the timer. I'm actually not sure why we need a getter for this, since it's only used in one function, as far as I can tell. I'd just inline it when it's used in _init, and not keep the value around forever. Probably doesn't even need to be a pref, just a const somewhere. >+ get DASHBOARD_URL() { >+ return Preferences.get("services.aitc.dashboard.url"); >+ }, Unless we need this pref to be live-changeable, we can just do this once, rather than calling across xpcom every time we touch this getter. Doesn't even need to be a getter, just something like this (we're going to fetch this pref during init anyway, so we might as well just do it here). If we need to support "live" changes of this pref, we should just use a pref observer, not constantly re-fetch this, especially since we're changing this for now to be an origin, so it's only going to get more expensive. To wit: DASHBOARD_ORIGIN: CommonUtils.makeURI(Preferences.get("services.aitc.dashboard.url")).prePath (the CommonUtils one catches exceptions, Services.io doesn't.) >+ // Add listeners for all windows opened in the future. >+ function winWatcher(subject, topic) { >+ if (topic != "domwindowopened") return; same style comment as Greg: {} I'm fine with it as a single line, but leave whitespace after for readability, please! >+ // Also check the currently open URI. >+ let uri = Services.io.newURI( >+ browser.contentDocument.location.href, null, null >+ ); >+ if (uri.prePath == this.DASHBOARD_URL) { >+ dashboardLoaded(browser); >+ } Just use currentURI here: if (browser.currentURI.prePath == this.DASHBOARD_URL) {} >+ observe: function(aSubject, aTopic, aData) { >+ case "idle": >+ this._log.info("User went idle"); >+ if (this._manager) { >+ this._manager.userIdle(); >+ } >+ break; >+ case "back": >+ this._log.info("User is no longer idle"); >+ let win = Services.wm.getMostRecentWindow("navigator:browser"); >+ if (win && this._manager) { >+ if (win.gBrowser.currentURI.prePath == this.DASHBOARD_URL) { no reason to nest, all three can be the same if statement. I also suspect that the "this._manager" check is least likely to fail, so I'd put that last. (uOptimizations ftw!)

Comment on attachment 628157 [details] [diff] [review] AITC Manager RC2 Almost there! >diff --git a/services/aitc/modules/manager.js b/services/aitc/modules/manager.js >+/** >+ * The constructor for the manager takes a callback, which will be invoked when >+ * the manager is ready (construction is asynchronous). *DO NOT* call any >+ * methods on this object until the callback has been invoked, doing so will >+ * lead to undefined behaviour. >+ */ Should we guard against this somehow? How bad could this be? >+function AitcManager(cb) { >+ this._client = null; >+ this._getTimer = null; >+ this._putTimer = null; >+ >+ this._log = Log4Moz.repository.getLogger("Service.AITC.Manager"); >+ this._log.level = Log4Moz.Level[Preferences.get("manager.log.level")]; >+ this._log.info("Loading AitC manager module"); At some point we should really just have a helper for this, so much duplication. >+ // Check if we have pending PUTs from last time. >+ let self = this; >+ this._pending = new AitcQueue("webapps-pending.json", function _queueDone() { >+ // Inform the AitC service that we're good to go! >+ self._log.info("AitC manager has finished loading"); >+ cb(true); >+ >+ // Schedule them, but only if we can get a silent assertion. >+ self._makeClient(function(err, client) { >+ if (!err && client) { >+ self._client = client; >+ self._processQueue(); >+ } >+ }, false); I sort of wonder if we should be more aggressive in this case. Shouldn't we actively be driving towards getting these records current on the server? >+AitcManager.prototype = { >+ get MARKETPLACE() { >+ return PREFS.get("marketplace.url"); >+ }, >+ >+ get DASHBOARD_URL() { >+ return PREFS.get("dashboard.url"); >+ }, >+ >+ get TOKEN_SERVER() { >+ return PREFS.get("tokenServer.url"); >+ }, Same as main.js, these don't need to be live at this point, IMO. Also, add some consts here, and use them instead of strings for args. Still readable, but magic strings aren't much better than magic numbers. _ACTIVE: 1, _PASSIVE: 2, >+ /** >+ * Local app was just installed or uninstalled, ask client to PUT if user >+ * is logged in. >+ */ >+ appEvent: function appEvent(type, app) { >+ // Add this to the equeue. >+ let self = this; >+ let obj = {type: type, app: app, retries: 0, lastTime: 0}; >+ this._pending.enqueue(obj, function _enqueued(err, rec) { >+ if (err) { >+ self._log.error("Could not add " + type + " " + app + " to queue"); >+ return; >+ } >+ >+ // If we already have a client (i.e. user is logged in), attempt to PUT. >+ if (self._client) { >+ self._processQueue(); >+ return; >+ } >+ >+ // If not, try a silent client creation. >+ self._makeClient(function(err, client) { >+ if (!err && client) { >+ self._client = client; >+ self._processQueue(); >+ } >+ // If user is not logged in, we'll just have to try later. how would we hit this point? I guess from a non-BID marketplace? Regardless, I really think at this point we should be more aggressive about getting this data onto the service. >+ /** >+ * Poll the AITC server for any changes and process them. It is safe to call uber-nit: extra space! >+ * this function multiple times. Last caller wins. >+ * >+ * Call this whenever the following occurs: >+ * ACTIVE: User visits dashboard (immediate fetch + timer on ACTIVE_FREQ). >+ * PASSIVE: User is no longer on the dashboard (timer on IDLE_FREQ). >+ * PASSIVE: User goes idle (timer on IDLE_FREQ) even if user is on dashboard. >+ */ also note that this throws on invalid input >+ _setPoll: function _setPoll(type) { >+ if (type == "ACTIVE" && !this._client) { >+ throw new Error("_setPoll(ACTIVE) called without client"); >+ } >+ if (type != "ACTIVE" && type != "PASSIVE") { >+ throw new Error("_setPoll called with invalid argument " + type); >+ } >+ >+ if (!this._client) { >+ // User is not logged in, we cannot do anything. >+ self._log.warn("_setPoll called but user not logged in, ignoring"); >+ return; >+ } >+ >+ // Check if there are any PUTs pending first. >+ if (this._pending.length() && !(this._putTimer)) { >+ // There are pending PUTs and no timer, so let's process them. >+ this._processQueue(); >+ } else { >+ // Do one GET soon. >+ CommonUtils.nextTick(this._checkServer, this); >+ } As noted, this means the dashboard will be out of date until PUTs are completed, so we should make sure we close that loop. Distilling the IRC conversation, we're going to save idle/active state on AitcManager as a property and only call setPoll if that value changes. setPoll will use the state to make the right calls for timers, and will only call nextTick when active. We'll also return in the pending == true case and let processQueue kick the polling off once those are done, rather than uselessly spin timers in the meantime. >+ /** >+ * Go through list of apps to PUT and attempt each one. If we fail, try >+ * again in PUT_FREQ. >+ */ >+ _processQueue: function _processPutQueue() { >+ if (!this._client) { >+ throw new Error("_processQueue called without a client"); >+ } >+ >+ if (!this._pending.length()) { >+ this._log.info("There are no pending items, _processQueue closing"); >+ if (this._putTimer) { >+ this._putTimer.clear(); >+ } In the revised model, we'll make this trigger _setPoll so we can actually create timers. + /* Obtain a token from Sagrada token server, given a BrowserID assertion >+ * cb(err, token) will be invoked on success or failure. >+ */ >+ _getToken: function _getToken(assertion, cb) { >+ let url = this.TOKEN_SERVER + "/1.0/aitc/1.0"; Magic strings! Probably fine for now, but we'll want a better solution to this at some point.

Addressed all the above concerns (except one style nit about inline functions). Some notes: (In reply to Mike Connor [:mconnor] from comment #15) > >+/** > >+ * The constructor for the manager takes a callback, which will be invoked when > >+ * the manager is ready (construction is asynchronous). *DO NOT* call any > >+ * methods on this object until the callback has been invoked, doing so will > >+ * lead to undefined behaviour. > >+ */ > > Should we guard against this somehow? How bad could this be? Well, the reason for this async chain goes all the way down to AitcQueue which has to load the PUT queue from disk asynchronously. I fiddled with several different ways of making this cleaner, and finally settled on callbacks to constructors as the most "checkin-able" approach. yield, runloops and other such tricks (which we used aplenty in Weave!) usually makes somebody's eyes twitch. I don't think we absolutely need a guard, but it won't hurt to add one. It's a chain of 4 constructors all of which interact with each other in only one way, so it's rather easy to ensure nothing is awry by manual review. > >+ // Check if we have pending PUTs from last time. > >+ let self = this; > >+ this._pending = new AitcQueue("webapps-pending.json", function _queueDone() { > >+ // Inform the AitC service that we're good to go! > >+ self._log.info("AitC manager has finished loading"); > >+ cb(true); > >+ > >+ // Schedule them, but only if we can get a silent assertion. > >+ self._makeClient(function(err, client) { > >+ if (!err && client) { > >+ self._client = client; > >+ self._processQueue(); > >+ } > >+ }, false); > > I sort of wonder if we should be more aggressive in this case. Shouldn't we > actively be driving towards getting these records current on the server? Being aggressive means designing UI for getting the user to sign in when they're not on the dashboard. When we get Sign in to the Browser, this problem will go away. > >+ /** > >+ * Local app was just installed or uninstalled, ask client to PUT if user > >+ * is logged in. > >+ */ > >+ appEvent: function appEvent(type, app) { > >+ // Add this to the equeue. > >+ let self = this; > >+ let obj = {type: type, app: app, retries: 0, lastTime: 0}; > >+ this._pending.enqueue(obj, function _enqueued(err, rec) { > >+ if (err) { > >+ self._log.error("Could not add " + type + " " + app + " to queue"); > >+ return; > >+ } > >+ > >+ // If we already have a client (i.e. user is logged in), attempt to PUT. > >+ if (self._client) { > >+ self._processQueue(); > >+ return; > >+ } > >+ > >+ // If not, try a silent client creation. > >+ self._makeClient(function(err, client) { > >+ if (!err && client) { > >+ self._client = client; > >+ self._processQueue(); > >+ } > >+ // If user is not logged in, we'll just have to try later. > > how would we hit this point? I guess from a non-BID marketplace? > Regardless, I really think at this point we should be more aggressive about > getting this data onto the service. Same as above.

Comment on attachment 628471 [details] [diff] [review] AITC Manager RC3 Found a bug, fix coming up shortly.

This one is for real!

Comment on attachment 628487 [details] [diff] [review] AITC Manager RC3 >+ this._log.info("User is no longer idle"); >+ let win = Services.wm.getMostRecentWindow("navigator:browser"); >+ if (win && (win.gBrowser.currentURI.prePath == this.DASHBOARD_ORIGIN) && >+ this._manager) { nit: unnecessary brackets. >+ // Set preferences >+ this.MARKETPLACE = PREFS.get("marketplace.url"); >+ this.DASHBOARD_URL = PREFS.get("dashboard.url"); >+ this.TOKEN_SERVER = PREFS.get("tokenServer.url"); These are each used in a single place. There's no need to keep these values around forever, so just inline the pref calls where used. If we need to re-use these in the future, it's an easy change then. >+ function _reschedule() { >+ // Release PUT lock >+ self._putInProgress = false; >+ >+ // We just finished PUTting an object, try the next one immediately, >+ // but only if haven't tried it already in the last putFreq (ms). >+ if (!self._pending.length()) { >+ // If the user is active call _setPoll which will do a GET immediately. >+ if (self._state == self._ACTIVE) { >+ self._setPoll(); >+ } >+ return; We return out of setPoll if there's a queue, so the timer never gets set. We already have the check in setPoll to not trigger on nextTick, so we don't need to check here, but we do need to call _setPoll to get the passive timer created and running. Basically this should now be: "Now that we've uploaded everything, start the GET timer." r=me with this. I assume tests are coming/somewhere, since you've been breaking tests. :D

Fixed

We should go through this tomorrow. I see no other way to detect whether or not the user intends to use apps. (Also fixed mconnor's nits).

AitcQueue.length is no longer a function. I also missed adding the default prefs to package-manifest.in which was causing the try builds to not work.

Comment on attachment 628619 [details] [diff] [review] AITC Manager RC3.2 Review of attachment 628619 [details] [diff] [review]: ----------------------------------------------------------------- Where are the tests? My brain is cooked from being sick. I tried to review manager.js but didn't get anywhere. mconnor will have to be it. :/ ::: services/aitc/service.js @@ +32,2 @@ > break; > + case "sessionstore-windows-restored": Should we remove the observer once it has fired? @@ +35,2 @@ > Cu.import("resource://services-common/preferences.js"); > if (Preferences.get("services.sync.engine.apps", false)) { What about if Sync is not enabled? We may want to throw a check for another pref in here. Maybe check if services.sync.account is defined. But, this is AITC code: you shouldn't need to read Sync prefs. Ideally you would be touching a lightweight Sync API asking through it "is the apps engine enabled?" I think I'm OK with this being a follow-up. Whatever happens, you probably want an explicit test covering this. I could see it breaking easily and possibly silently. @@ +75,5 @@ > + }, > + > + start: function start() { > + Cu.import("resource://services-aitc/main.js"); > + this.aitc = new Aitc(); Your code looks proper. But, I would guard against multiple calls to start() just in case. i.e. no-op if this.aitc exists already. @@ +85,5 @@ > + return true; > + } > + if (gh.isVisited(this.MARKETPLACE_URL)) { > + return true; > + } I remember seeing discussion about this. IMO this seems very hacky. Although, I'm sure it works. It is also very Mozilla-centric. Could there not be multiple marketplaces or dashboards? I would push for this to be an API elsewhere and for the logic to be a little more... robust. Perhaps add a code comment detailing everything. @@ +101,5 @@ > + onPageExpired: function() {}, > + onTitleChanged: function() {}, > + > + onVisit: function onVisit(uri) { > + if (!uri.equals(this.MARKETPLACE_URL) && !uri.equals(this.DASHBOARD_URL)) { Consider doing a prefix match. Shouldn't all "child" URIs also trigger this? ::: services/aitc/modules/main.js @@ +127,5 @@ > + idleSvc.addIdleObserver(this, > + Preferences.get("services.aitc.main.idleTime")); > + }, > + > + observe: function(aSubject, aTopic, aData) { Nit: we don't use Gecko's argument "a" prefixing naming style. @@ +157,5 @@ > + break; > + } > + }, > + > +}; I'd like someone with more experience with the browser/windows/tabs APIs to r+ this file. It seems right to me. But, I don't know that much about things. Too bad we can't use the JetPack APIs - they are so much cleaner! ::: services/aitc/modules/manager.js @@ +41,5 @@ > + let self = this; > + this._pending = new AitcQueue("webapps-pending.json", function _queueDone() { > + // Inform the AitC service that we're good to go! > + self._log.info("AitC manager has finished loading"); > + cb(true); If this throws, we are in a bad state. @@ +73,5 @@ > + return; > + } > + this.__state = value; > + this._setPoll(); > + }, Ugh. I've never seen internal getters and setters before. But, there's nothing wrong with that (I think). I do have a problem with the __state with a double underscore. Please change that variable name so it has only 1 leading underscore. _clientState? @@ +83,5 @@ > + appEvent: function appEvent(type, app) { > + // Add this to the equeue. > + let self = this; > + let obj = {type: type, app: app, retries: 0, lastTime: 0}; > + this._pending.enqueue(obj, function _enqueued(err, rec) { Might look nicer if this is pulled out into the prototype. I really don't like the "let self = this" pattern unless you really, really need a closure. ::: services/aitc/services-aitc.js @@ +18,3 @@ > pref("services.aitc.storage.log.level", "Debug"); > > +pref("services.aitc.tokenServer.url", "https://stage-token.services.mozilla.com"); I'm sure this has been called out already. If it isn't permanent, please comment such.

(In reply to Gregory Szorc [:gps] from comment #23) > Where are the tests? I filed bug 760902 so we can write some more. We need to do some work here to make sure we can tests timers properly (which is the bulk of manager.js) without hitting timeouts. > @@ +35,2 @@ > > Cu.import("resource://services-common/preferences.js"); > > if (Preferences.get("services.sync.engine.apps", false)) { > > What about if Sync is not enabled? We may want to throw a check for another > pref in here. Maybe check if services.sync.account is defined. We also test for services.aitc.enabled (false by default). This line can most likely go away when we remove the old sync engine. > I remember seeing discussion about this. IMO this seems very hacky. > Although, I'm sure it works. It is also very Mozilla-centric. Could there > not be multiple marketplaces or dashboards? > > I would push for this to be an API elsewhere and for the logic to be a > little more... robust. Perhaps add a code comment detailing everything. > > Consider doing a prefix match. Shouldn't all "child" URIs also trigger this? I think the whole logic has to change. Let's brainstorm over in bug 760898. > > +pref("services.aitc.tokenServer.url", "https://stage-token.services.mozilla.com"); > > I'm sure this has been called out already. If it isn't permanent, please > comment such. Bug 760903. I tested against the production server but it's currently broken.

https://hg.mozilla.org/services/services-central/rev/3b82c67c4e12 Please file any new issues as followup bugs!

Verified on Nightly on Windows 7 through doing an initial test pass on AITC desktop. I can confirm that this has indeed landed. All follow-up issues should be filed in separate bugs.

Comment on attachment 628619 [details] [diff] [review] AITC Manager RC3.2 Bug landed, clearing flag.