674354 - New blank tab multivariate test

Status: RESOLVED INCOMPLETE

(Mozilla Labs Graveyard :: Test Pilot Studies, defect)

Description

[Lilian Weng]

Tracking the blank tab multivariate test study!

Comment 1

[Jono Xia]

A few lines in this file are really long and hard to read -- maybe wrap lines that are longer than 100 characters into multiple lines?

Code that's not being used should actually be removed, not just commented out  (fine to comment it out during development /testing, but when it's getting ready for release you should remove this stuff)

All dump statements should be removed before releasing a study.  (You can replace them with console.info() or console.warn() in order to write to the log file.)

line 54- let's run for 5 percent of users to begin with, then increase to 100 if no bugs found.

line 56 - let's turn the "don't run for users on release channel" filter back on actually - 1.2 still not released :-(

line 142 - This unload() function is kind of hacky - I see that it works by hooking into the uninstallAll function of the base class with objects that have uninstall() methods, so they will be called from the base class onExperimentShutdown.

There's already a built-in method to call functions when the module is unloaded (see the function desctructor() inside require("unload") at the bottom of the file) - could this cleanup be done there instead?

If not, maybe keep your own list of functions to call from onExperimentShutdown, instead of pushing them into a list in the base class which was really designed for something else.

Line 156 - comment looks incomplete

Line 157 - Since UserAction is a singleton object that always refers to the last user action (there cannot be multiples created to store other user actions) maybe makes more sense to call it "LastUserAction" or something?

Line 158 comment looks out of date -- i assume addon = 4, 5 means option E, F ?

Why is the addon stored in UserAction?  It makes sense that you want the number of the addon to be accessible as a global variable, but the addon will not change as a result of the user's interaction with the blank tab, unlike the rest of the properties here, so having addon as a property of UserAction seems a little out-of-place.

Line 217 - since hashedString() uses so many properties of exports.handlers (the global observer) and doesn't use any properties of the BlankTabWindowObserver, maybe it should be a method of the global observer instead.

Line 229 - Why are you toHexStringing every character of the md5 hash, instead of just returning the md5 hash?  Is this neccessary?

Line 230 - Should have a semicolon.

Line 244 - Comment on BlankTabWindowObserver.prototype.getUrlBarString is misleading - it doesn't get URL string exactly, it gets an object with a hash of the host and a boolean telling whether the tab is blank.

Line 249 - You use a lot of services (sessionService, IOService, md5service, etc) by storing the references as properties of exports.handlers.  But you use AddonManager and Services.jsm by importing them as global modules.  Maybe it would make more sense to use the other services by importing as global modules, too?  This might just be a symptom of the flaws in the current test pilot API, that there's not a good way to deal with these external dependencies right now...

Line 251 - All Dump statements should be removed before releasing.

Line 279 - getCurrentTabId - why do you need to check tabIDString.length > 10 ?  Either it's a timestamp (because you set it to a timestamp in setCurrentTabId) or it's blank, right?  Why would it be a string with length <= 10 ?

Line 330 - Really minor style issue with function names:  if you call a function "switchTab" it sounds like calling the function will cause the tab to switch.  That's not what this function is, it's responding to a tab switch.  So maybe makes more sense to call it "onTabSwitch" or something?

Line 331 - You set "self = this" at the begining of this function, but there are no anonymous callback functions defined here, so you don't actually need "self", you can just use "this.getCurrentTabId()" and "this.getNumTabs()" etc.

Line 346 - the comment here looks out-of-date (says "NewTabWindowObserver" but the class is now called "BlankTabWindowObserver"?

line 358 - is it redundant to check that domain.hashed.length <= 0 and also that domain.isBlankTab ??  Is it ever possible for domain.isBlankTab to be true but the hash length to be > 0?  Or is it possible for the hash length to be <= 0 but domain.isBlankTab is false?

Line 366: What happens if gBrow is null?  Is that something that should never happen?  Maybe we should log an error just so that we'll know if it ever does happen?

Line 390 - 396:  This code assumes that the event listener here is called AFTER targ's "pinned" attribute is changed due to the click.  If they happen in the other order, the recorded event will be wrong.  Make sure to test that the recorded event is correct!

Should there be code here to handle the case where targClass doesn't match any of the expected classes?  It's unlikely, but then if something weird starts happening (the user is clicking something that we didn't expect to be there) we would know what was going on.

Every time you call addonRecord (in the whole file), the eventString argument is "addon".  Might as well hard-code it into the addonRecord function?

Line 444:  You already checked if (method) above, so checking "&& method" on this line is redundant.

Line 450: What is the error that can happen here? Maybe add a comment to explain?

By the way, if an exception is raised by study code and is not caught, the extension is supposed to log it and report it back to us.  So it may actually be better, for a study that is getting released, to remove the try-catch and let the error happen so it can be logged.

Line 506 - It's generally best to always put { } around the body statements of an "if", even if it is only one line, as it helps to avoid a whole class of silly mistakes.  Same goes for the body of "else".

Line 530 - You can also move down in the awesomebar suggestions by hitting the TAB key, besides the Down Arrow key.  Your code should listen for the Tab key here as well.

Line 596 - What happens if user clicks in the bookmark toolbar, but it's on an empty space, not on one of the links?  Maybe nothing, but you should test to make sure.

Line 676 - The only time in the code that you call installAddon(), you check that designIndex > 0 first before calling.  So checking it again inside the function is redundant.

We had a bug in an old study once where if the user was in experiment group 0 (the control group), and onExperimentStartup got installed again, it would re-randomize and assign the user to another experiment group.  The result was that almost no users stayed in the control group since they would keep randomizing until they got into a different group.  The cause of the bug was that I had "if (experimentGroup)" instead of "if (prefs.prefHasUserValue(EXPERIMENT_GROUP_PREF))".  And of course 0 is false, so it counted group 0 the same as un-set.

Anyway I'm telling this story to say that it's really important to double check that users can't be re-assigned to different experiment groups once they've already got an experiment group.  I think you're doing it right; your design using "randNum % NUM_DESIGN" avoids this problem.


Line 687 - if addon is null, then saying "addon.isActive" will cause a "null has no properties" error.  Maybe better to write this line as
let isActive = installed && addon.isActive;

because then if the first part is null, the second part is not evaluated.

line 706 - Should put {} around the body of this if statement (I almost misread it as AddonManager.getInstallForURL only gets called if prefHasUserValue is true).

line 733 - Is there no way to pass the icon URL into Services.ww.openWindow as an argument?  This here seems like kind of a weird way to set the icon...

Line 748 - If an error occurs when installing the add-on, then PREF_INSTALLED will be set to true anyway, even though the pref isn't really installed?  This seems wrong.

(And what is PREF_INSTALLED used for, anyway?  The only place I see it was being used was on line 855, where it's commented out now, and on line 872, which is just clearing it.  If we don't need this pref anymore, remove it!)

Line 756 - Good that we're recording the choice of the user not to install the add-on.  I'm wondering how we should treat the data from these users?  They don't have the new interface obviously but they aren't the same as control group users either, because they're not random - they made a choice not to change their interface.  (That might mean these users are happy with the current interface, or just that they're too busy to try something new.)


Line 774 - This is a pretty clever way to make a listener.  But wouldn't it be easier to make the strings in the array capitalized to begin with?  ["Disabled", "Enabled", "Installed", "Uninstalled"]

Line 807 - Not sure what you mean by 'call them once in order to speed up' -- you're just loading the services here in order to have them loaded later, right?'

Line 834 - I don't think this is true.  If you look at the extension source code, modules/tasks.js, doExperimentCleanup is called from line 721 (if the test finishes) and from line 903 (if user opts out) -- in both cases there is a call to onExperimentShutdown right before.  So I don't think you need to call onExperimentShutdown here.  Does something bad happen in testing if you don't call it?  If so, that bug might have another cause.

(Oh, this might be related to my comment on line 142 -- are you calling onExperimentShutdown because you need to trigger all of your unload() functions here?)

Line 839 - If designIndex is 0 (meaning user is in the control group), then we don't need to uninstall the addon, but we should still do the other stuff in this function -- clearing out the prefs and setting meta.designIndex, right?

Line 868 - "meta" appears to be undefined?  Should have
let meta = {};
maybe?

Line 899 - Since you have 3 data plots and not just 1, you may need to rewrite this.saveButtons - the method implemented in the base class only expects to save one plot.

Comment 2

[Lilian Weng]

users complain about that new tab add is still working while TP is disabled or all the studies are turnt off. FIXED by changing new tab add-on.