Respond to more review comments

core/field_variable.js

@@ -130,8 +130,9 @@ Blockly.FieldVariable.prototype.getValue = function() {
 };
 
 /**
- * Get the text from this field.
- * @return {string} Current text.
+ * Get the text from this field, which is the selected variable's name.
+ * @return {string} The selected variable's name, or the empty string if no
+ *     variable is selected.
  */
 Blockly.FieldVariable.prototype.getText = function() {
   return this.variable_ ? this.variable_.name : '';
@@ -171,7 +172,7 @@ Blockly.FieldVariable.prototype.setValue = function(id) {
   if (this.sourceBlock_ && Blockly.Events.isEnabled()) {
     var oldValue = this.variable_ ? this.variable_.getId() : null;
     Blockly.Events.fire(new Blockly.Events.BlockChange(
-        this.sourceBlock_, 'field', this.name, oldValue, variable.getId()));
+        this.sourceBlock_, 'field', this.name, oldValue, id));
   }
   this.variable_ = variable;
   this.value_ = id;

core/variables.js

@@ -378,16 +378,20 @@ Blockly.Variables.generateVariableFieldXml_ = function(variableModel) {
  * @param {!Blockly.Workspace} workspace The workspace to search for the
  *     variable.  It may be a flyout workspace or main workspace.
  * @param {string} id The ID to use to look up or create the variable, or null.
- * @param {string} name The string to use to look up or create the variable,
- * @param {string} type The type to use to look up or create the variable.
+ * @param {string=} opt_name The string to use to look up or create the
+ *     variable.
+ * @param {string=} opt_type The type to use to look up or create the variable.
  * @return {!Blockly.VariableModel} The variable corresponding to the given ID
  *     or name + type combination.
  * @package
  */
-Blockly.Variables.getOrCreateVariable = function(workspace, id, name, type) {
-  var variable = Blockly.Variables.getVariable(workspace, id, name, type);
+Blockly.Variables.getOrCreateVariable = function(workspace, id, opt_name,
+    opt_type) {
+  var variable = Blockly.Variables.getVariable(workspace, id, opt_name,
+      opt_type);
   if (!variable) {
-    variable = Blockly.Variables.createVariable_(workspace, id, name, type);
+    variable = Blockly.Variables.createVariable_(workspace, id, opt_name,
+        opt_type);
   }
   return variable;
 };
@@ -405,7 +409,7 @@ Blockly.Variables.getOrCreateVariable = function(workspace, id, name, type) {
  *     if lookup by ID fails.
  * @return {?Blockly.VariableModel} The variable corresponding to the given ID
  *     or name + type combination, or null if not found.
- * @private
+ * @package
  */
 Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) {
   var potentialVariableMap = workspace.getPotentialVariableMap();
@@ -422,6 +426,8 @@ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) {
     if (!variable && potentialVariableMap) {
       variable = potentialVariableMap.getVariable(opt_name, opt_type);
     }
+  } else {
+    throw new Error('Tried to look up a variable by name without a type');
   }
   return variable;
 };
@@ -431,25 +437,26 @@ Blockly.Variables.getVariable = function(workspace, id, opt_name, opt_type) {
  * @param {!Blockly.Workspace} workspace The workspace in which to create the
  * variable.  It may be a flyout workspace or main workspace.
  * @param {string} id The ID to use to create the variable, or null.
- * @param {string} name The string to use to create the variable.
- * @param {string} type The type to use to create the variable.
+ * @param {string=} opt_name The string to use to create the variable.
+ * @param {string=} opt_type The type to use to create the variable.
  * @return {!Blockly.VariableModel} The variable corresponding to the given ID
  *     or name + type combination.
  * @private
  */
-Blockly.Variables.createVariable_ = function(workspace, id, name, type) {
+Blockly.Variables.createVariable_ = function(workspace, id, opt_name,
+    opt_type) {
   var potentialVariableMap = workspace.getPotentialVariableMap();
   // Variables without names get uniquely named for this workspace.
-  if (!name) {
+  if (!opt_name) {
     var ws = workspace.isFlyout ? workspace.targetWorkspace : workspace;
-    name = Blockly.Variables.generateUniqueName(ws);
+    opt_name = Blockly.Variables.generateUniqueName(ws);
   }
 
   // Create a potential variable if in the flyout.
   if (potentialVariableMap) {
-    var variable = potentialVariableMap.createVariable(name, type, id);
+    var variable = potentialVariableMap.createVariable(opt_name, opt_type, id);
   } else {  // In the main workspace, create a real variable.
-    var variable = workspace.createVariable(name, type, id);
+    var variable = workspace.createVariable(opt_name, opt_type, id);
   }
   return variable;
 };

core/workspace.js

@@ -86,7 +86,7 @@ Blockly.Workspace = function(opt_options) {
   this.variableMap_ = new Blockly.VariableMap(this);
 
   /**
-   * Blocks in the flyout can refer to variables that don't exist in the
+   * Blocks in the flyout can refer to variables that don't exist in the main
    * workspace.  For instance, the "get item in list" block refers to an "item"
    * variable regardless of whether the variable has been created yet.
    * A FieldVariable must always refer to a Blockly.VariableModel.  We reconcile
@@ -227,7 +227,7 @@ Blockly.Workspace.prototype.createVariable = function(name, opt_type, opt_id) {
 };
 
 /**
- * Find all the uses of a named variable.
+ * Find all the uses of the given variable, which is identified by ID.
  * @param {string} id ID of the variable to find.
  * @return {!Array.<!Blockly.Block>} Array of block usages.
  */

core/xml.js

@@ -88,7 +88,7 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) {
 
 /**
  * Encode a variable field as XML.
- * @param {!Blockly.Field} field The field to encode.
+ * @param {!Blockly.FieldVariable} field The field to encode.
  * @return {?Element} XML element, or null if the field did not need to be
  *     serialized.
  * @private
@@ -96,12 +96,18 @@ Blockly.Xml.blockToDomWithXY = function(block, opt_noId) {
 Blockly.Xml.fieldToDomVariable_ = function(field) {
   var id = field.getValue();
   // The field had not been initialized fully before being serialized.
+  // This can happen if a block is created directly through a call to
+  // workspace.newBlock instead of from XML.
+  // The new block will be serialized for the first time when firing a block
+  // creation event.
   if (id == null) {
     field.initModel();
     id = field.getValue();
   }
   // Get the variable directly from the field, instead of doing a lookup.  This
-  // will work even if the variable has already been deleted.
+  // will work even if the variable has already been deleted.  This can happen
+  // because the flyout defers deleting blocks until the next time the flyout is
+  // opened.
   var variable = field.getVariable();
 
   if (!variable) {