From 923eae8b16e4f9e7d121bc7b3084f4e717e2f166 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Mon, 18 Jul 2016 14:46:12 -0700 Subject: JavaScript maps: move binary callbacks out of constructor. This change will help us separate binary support into separate files, because we only refer to binary serialization functions in the actual binary serialization paths. --- js/map.js | 87 +++++++++++++++++++++++------------------------------------ js/message.js | 20 ++------------ 2 files changed, 36 insertions(+), 71 deletions(-) (limited to 'js') diff --git a/js/map.js b/js/map.js index 821765ec..f051e00d 100644 --- a/js/map.js +++ b/js/map.js @@ -44,65 +44,23 @@ goog.forwardDeclare('jspb.BinaryWriter'); * on ES6 itself. * * This constructor should only be called from generated message code. It is not - * intended for general use by library consumers. The callback function - * arguments are references to methods in `BinaryReader` and `BinaryWriter`, as - * well as constructors and reader/writer methods in submessage types if - * appropriate, that are used for binary serialization and parsing. + * intended for general use by library consumers. * * @template K, V * * @param {!Array>} arr * - * @param {function(this:jspb.BinaryWriter,number,K)=} opt_keyWriterFn - * The method on BinaryWriter that writes type K to the stream. - * - * @param {function(this:jspb.BinaryReader):K=} opt_keyReaderFn - * The method on BinaryReader that reads type K from the stream. - * - * @param {function(this:jspb.BinaryWriter,number,V)| - * function(this:jspb.BinaryReader,V,?)=} opt_valueWriterFn - * The method on BinaryWriter that writes type V to the stream. May be - * writeMessage, in which case the second callback arg form is used. - * - * @param {function(this:jspb.BinaryReader):V| - * function(this:jspb.BinaryReader,V, - * function(V,!jspb.BinaryReader))=} opt_valueReaderFn - * The method on BinaryReader that reads type V from the stream. May be - * readMessage, in which case the second callback arg form is used. - * * @param {?function(new:V)|function(new:V,?)=} opt_valueCtor * The constructor for type V, if type V is a message type. * - * @param {?function(V,!jspb.BinaryWriter)=} opt_valueWriterCallback - * The BinaryWriter serialization callback for type V, if V is a message - * type. - * - * @param {?function(V,!jspb.BinaryReader)=} opt_valueReaderCallback - * The BinaryReader parsing callback for type V, if V is a message type. - * * @constructor * @struct */ -jspb.Map = function( - arr, opt_keyWriterFn, opt_keyReaderFn, opt_valueWriterFn, opt_valueReaderFn, - opt_valueCtor, opt_valueWriterCallback, opt_valueReaderCallback) { - +jspb.Map = function(arr, opt_valueCtor) { /** @const @private */ this.arr_ = arr; /** @const @private */ - this.keyWriterFn_ = opt_keyWriterFn; - /** @const @private */ - this.keyReaderFn_ = opt_keyReaderFn; - /** @const @private */ - this.valueWriterFn_ = opt_valueWriterFn; - /** @const @private */ - this.valueReaderFn_ = opt_valueReaderFn; - /** @const @private */ this.valueCtor_ = opt_valueCtor; - /** @const @private */ - this.valueWriterCallback_ = opt_valueWriterCallback; - /** @const @private */ - this.valueReaderCallback_ = opt_valueReaderCallback; /** @type {!Object>} @private */ this.map_ = {}; @@ -385,19 +343,29 @@ jspb.Map.prototype.has = function(key) { * number. * @param {number} fieldNumber * @param {!jspb.BinaryWriter} writer + * @param {function(this:jspb.BinaryWriter,number,K)=} keyWriterFn + * The method on BinaryWriter that writes type K to the stream. + * @param {function(this:jspb.BinaryWriter,number,V)| + * function(this:jspb.BinaryReader,V,?)=} valueWriterFn + * The method on BinaryWriter that writes type V to the stream. May be + * writeMessage, in which case the second callback arg form is used. + * @param {?function(V,!jspb.BinaryWriter)=} opt_valueWriterCallback + * The BinaryWriter serialization callback for type V, if V is a message + * type. */ -jspb.Map.prototype.serializeBinary = function(fieldNumber, writer) { +jspb.Map.prototype.serializeBinary = function( + fieldNumber, writer, keyWriterFn, valueWriterFn, opt_valueWriterCallback) { var strKeys = this.stringKeys_(); strKeys.sort(); for (var i = 0; i < strKeys.length; i++) { var entry = this.map_[strKeys[i]]; writer.beginSubMessage(fieldNumber); - this.keyWriterFn_.call(writer, 1, entry.key); + keyWriterFn.call(writer, 1, entry.key); if (this.valueCtor_) { - this.valueWriterFn_.call(writer, 2, this.wrapEntry_(entry), - this.valueWriterCallback_); + valueWriterFn.call(writer, 2, this.wrapEntry_(entry), + opt_valueWriterCallback); } else { - this.valueWriterFn_.call(writer, 2, entry.value); + valueWriterFn_.call(writer, 2, entry.value); } writer.endSubMessage(); } @@ -410,8 +378,21 @@ jspb.Map.prototype.serializeBinary = function(fieldNumber, writer) { * when a key/value pair submessage is encountered. * @param {!jspb.Map} map * @param {!jspb.BinaryReader} reader + * @param {function(this:jspb.BinaryReader):K=} keyReaderFn + * The method on BinaryReader that reads type K from the stream. + * + * @param {function(this:jspb.BinaryReader):V| + * function(this:jspb.BinaryReader,V, + * function(V,!jspb.BinaryReader))=} valueReaderFn + * The method on BinaryReader that reads type V from the stream. May be + * readMessage, in which case the second callback arg form is used. + * + * @param {?function(V,!jspb.BinaryReader)=} opt_valueReaderCallback + * The BinaryReader parsing callback for type V, if V is a message type. + * */ -jspb.Map.deserializeBinary = function(map, reader) { +jspb.Map.deserializeBinary = function(map, reader, keyReaderFn, valueReaderFn, + opt_valueReaderCallback) { var key = undefined; var value = undefined; @@ -422,14 +403,14 @@ jspb.Map.deserializeBinary = function(map, reader) { var field = reader.getFieldNumber(); if (field == 1) { // Key. - key = map.keyReaderFn_.call(reader); + key = keyReaderFn.call(reader); } else if (field == 2) { // Value. if (map.valueCtor_) { value = new map.valueCtor_(); - map.valueReaderFn_.call(reader, value, map.valueReaderCallback_); + valueReaderFn.call(reader, value, opt_valueReaderCallback); } else { - value = map.valueReaderFn_.call(reader); + value = valueReaderFn.call(reader); } } } diff --git a/js/message.js b/js/message.js index 3863bac0..e8185dee 100644 --- a/js/message.js +++ b/js/message.js @@ -747,29 +747,16 @@ jspb.Message.getFieldProto3 = function(msg, fieldNumber, defaultValue) { * of serialization/parsing callbacks (which are required by the map at * construction time, and the map may be constructed here). * - * The below callbacks are used to allow the map to serialize and parse its - * binary wire format data. Their purposes are described in more detail in - * `jspb.Map`'s constructor documentation. - * * @template K, V * @param {!jspb.Message} msg * @param {number} fieldNumber * @param {boolean|undefined} noLazyCreate * @param {?=} opt_valueCtor - * @param {function(number,K)=} opt_keyWriterFn - * @param {function():K=} opt_keyReaderFn - * @param {function(number,V)|function(number,V,?)| - * function(number,V,?,?,?,?)=} opt_valueWriterFn - * @param {function():V| - * function(V,function(?,?))=} opt_valueReaderFn - * @param {function(?,?)|function(?,?,?,?,?)=} opt_valueWriterCallback - * @param {function(?,?)=} opt_valueReaderCallback * @return {!jspb.Map|undefined} * @protected */ jspb.Message.getMapField = function(msg, fieldNumber, noLazyCreate, - opt_valueCtor, opt_keyWriterFn, opt_keyReaderFn, opt_valueWriterFn, - opt_valueReaderFn, opt_valueWriterCallback, opt_valueReaderCallback) { + opt_valueCtor) { if (!msg.wrappers_) { msg.wrappers_ = {}; } @@ -787,10 +774,7 @@ jspb.Message.getMapField = function(msg, fieldNumber, noLazyCreate, } return msg.wrappers_[fieldNumber] = new jspb.Map( - /** @type {!Array>} */ (arr), - opt_keyWriterFn, opt_keyReaderFn, opt_valueWriterFn, - opt_valueReaderFn, opt_valueCtor, opt_valueWriterCallback, - opt_valueReaderCallback); + /** @type {!Array>} */ (arr), opt_valueCtor); } }; -- cgit v1.2.3 From 7429b91edafef18585d55ada62ba4ff01b88958c Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Mon, 18 Jul 2016 15:58:58 -0700 Subject: JavaScript: move extension binary info to separate struct. --- js/commonjs/export.js | 2 + js/message.js | 71 +++++++++++++++---------- src/google/protobuf/compiler/js/js_generator.cc | 50 +++++++++++++---- 3 files changed, 85 insertions(+), 38 deletions(-) (limited to 'js') diff --git a/js/commonjs/export.js b/js/commonjs/export.js index a3cfbd6f..23ae47f9 100644 --- a/js/commonjs/export.js +++ b/js/commonjs/export.js @@ -9,12 +9,14 @@ goog.require('goog.object'); goog.require('jspb.BinaryReader'); goog.require('jspb.BinaryWriter'); goog.require('jspb.ExtensionFieldInfo'); +goog.require('jspb.ExtensionFieldBinaryInfo'); goog.require('jspb.Message'); exports.Message = jspb.Message; exports.BinaryReader = jspb.BinaryReader; exports.BinaryWriter = jspb.BinaryWriter; exports.ExtensionFieldInfo = jspb.ExtensionFieldInfo; +exports.ExtensionFieldBinaryInfo = jspb.ExtensionFieldBinaryInfo; // These are used by generated code but should not be used directly by clients. exports.exportSymbol = goog.exportSymbol; diff --git a/js/message.js b/js/message.js index e8185dee..f746ee62 100644 --- a/js/message.js +++ b/js/message.js @@ -35,6 +35,7 @@ */ goog.provide('jspb.ExtensionFieldInfo'); +goog.provide('jspb.ExtensionFieldBinaryInfo'); goog.provide('jspb.Message'); goog.require('goog.array'); @@ -84,19 +85,12 @@ goog.forwardDeclare('xid.String'); * @param {?function(new: jspb.Message, Array=)} ctor * @param {?function((boolean|undefined),!jspb.Message):!Object} toObjectFn * @param {number} isRepeated - * @param {?function(number,?)=} opt_binaryReaderFn - * @param {?function(number,?)|function(number,?,?,?,?,?)=} opt_binaryWriterFn - * @param {?function(?,?)=} opt_binaryMessageSerializeFn - * @param {?function(?,?)=} opt_binaryMessageDeserializeFn - * @param {?boolean=} opt_isPacked * @constructor * @struct * @template T */ jspb.ExtensionFieldInfo = function(fieldNumber, fieldName, ctor, toObjectFn, - isRepeated, opt_binaryReaderFn, opt_binaryWriterFn, - opt_binaryMessageSerializeFn, opt_binaryMessageDeserializeFn, - opt_isPacked) { + isRepeated) { /** @const */ this.fieldIndex = fieldNumber; /** @const */ @@ -106,20 +100,37 @@ jspb.ExtensionFieldInfo = function(fieldNumber, fieldName, ctor, toObjectFn, /** @const */ this.toObjectFn = toObjectFn; /** @const */ - this.binaryReaderFn = opt_binaryReaderFn; + this.isRepeated = isRepeated; +}; + +/** + * Stores binary-related information for a single extension field. + * @param {!jspb.ExtensionFieldInfo} fieldInfo + * @param {?function(number,?)=} binaryReaderFn + * @param {?function(number,?)|function(number,?,?,?,?,?)=} binaryWriterFn + * @param {?function(?,?)=} opt_binaryMessageSerializeFn + * @param {?function(?,?)=} opt_binaryMessageDeserializeFn + * @param {?boolean=} opt_isPacked + * @constructor + * @struct + * @template T + */ +jspb.ExtensionFieldBinaryInfo = function(fieldInfo, binaryReaderFn, binaryWriterFn, + binaryMessageSerializeFn, binaryMessageDeserializeFn, isPacked) { /** @const */ - this.binaryWriterFn = opt_binaryWriterFn; + this.fieldInfo = fieldInfo; /** @const */ - this.binaryMessageSerializeFn = opt_binaryMessageSerializeFn; + this.binaryReaderFn = binaryReaderFn; /** @const */ - this.binaryMessageDeserializeFn = opt_binaryMessageDeserializeFn; + this.binaryWriterFn = binaryWriterFn; /** @const */ - this.isRepeated = isRepeated; + this.binaryMessageSerializeFn = binaryMessageSerializeFn; /** @const */ - this.isPacked = opt_isPacked; + this.binaryMessageDeserializeFn = binaryMessageDeserializeFn; + /** @const */ + this.isPacked = isPacked; }; - /** * @return {boolean} Does this field represent a sub Message? */ @@ -491,11 +502,13 @@ jspb.Message.toObjectExtension = function(proto, obj, extensions, jspb.Message.serializeBinaryExtensions = function(proto, writer, extensions, getExtensionFn) { for (var fieldNumber in extensions) { - var fieldInfo = extensions[fieldNumber]; + var binaryFieldInfo = extensions[fieldNumber]; + var fieldInfo = binaryFieldInfo.fieldInfo; + // The old codegen doesn't add the extra fields to ExtensionFieldInfo, so we // need to gracefully error-out here rather than produce a null dereference // below. - if (!fieldInfo.binaryWriterFn) { + if (!binaryFieldInfo.binaryWriterFn) { throw new Error('Message extension present that was generated ' + 'without binary serialization support'); } @@ -508,16 +521,17 @@ jspb.Message.serializeBinaryExtensions = function(proto, writer, extensions, // message may require binary support, so we can *only* catch this error // here, at runtime (and this decoupled codegen is the whole point of // extensions!). - if (fieldInfo.binaryMessageSerializeFn) { - fieldInfo.binaryWriterFn.call(writer, fieldInfo.fieldIndex, - value, fieldInfo.binaryMessageSerializeFn); + if (binaryFieldInfo.binaryMessageSerializeFn) { + binaryFieldInfo.binaryWriterFn.call(writer, fieldInfo.fieldIndex, + value, binaryFieldInfo.binaryMessageSerializeFn); } else { throw new Error('Message extension present holding submessage ' + 'without binary support enabled, and message is ' + 'being serialized to binary format'); } } else { - fieldInfo.binaryWriterFn.call(writer, fieldInfo.fieldIndex, value); + binaryFieldInfo.binaryWriterFn.call( + writer, fieldInfo.fieldIndex, value); } } } @@ -535,12 +549,13 @@ jspb.Message.serializeBinaryExtensions = function(proto, writer, extensions, */ jspb.Message.readBinaryExtension = function(msg, reader, extensions, getExtensionFn, setExtensionFn) { - var fieldInfo = extensions[reader.getFieldNumber()]; - if (!fieldInfo) { + var binaryFieldInfo = extensions[reader.getFieldNumber()]; + var fieldInfo = binaryFieldInfo.fieldInfo; + if (!binaryFieldInfo) { reader.skipField(); return; } - if (!fieldInfo.binaryReaderFn) { + if (!binaryFieldInfo.binaryReaderFn) { throw new Error('Deserializing extension whose generated code does not ' + 'support binary format'); } @@ -548,14 +563,14 @@ jspb.Message.readBinaryExtension = function(msg, reader, extensions, var value; if (fieldInfo.isMessageType()) { value = new fieldInfo.ctor(); - fieldInfo.binaryReaderFn.call( - reader, value, fieldInfo.binaryMessageDeserializeFn); + binaryFieldInfo.binaryReaderFn.call( + reader, value, binaryFieldInfo.binaryMessageDeserializeFn); } else { // All other types. - value = fieldInfo.binaryReaderFn.call(reader); + value = binaryFieldInfo.binaryReaderFn.call(reader); } - if (fieldInfo.isRepeated && !fieldInfo.isPacked) { + if (fieldInfo.isRepeated && !binaryFieldInfo.isPacked) { var currentList = getExtensionFn.call(msg, fieldInfo); if (!currentList) { setExtensionFn.call(msg, fieldInfo, [value]); diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc index e030066e..d3852a95 100755 --- a/src/google/protobuf/compiler/js/js_generator.cc +++ b/src/google/protobuf/compiler/js/js_generator.cc @@ -2524,6 +2524,29 @@ void Generator::GenerateClassExtensionFieldInfo(const GeneratorOptions& options, "$class$.extensions = {};\n" "\n", "class", GetPath(options, desc)); + + if (options.binary) { + printer->Print( + "\n" + "/**\n" + " * The extensions registered with this message class. This is a " + "map of\n" + " * extension field number to fieldInfo object.\n" + " *\n" + " * For example:\n" + " * { 123: {fieldIndex: 123, fieldName: {my_field_name: 0}, " + "ctor: proto.example.MyMessage} }\n" + " *\n" + " * fieldName contains the JsCompiler renamed field name property " + "so that it\n" + " * works in OPTIMIZED mode.\n" + " *\n" + " * @type {!Object.}\n" + " */\n" + "$class$.extensionsBinary = {};\n" + "\n", + "class", GetPath(options, desc)); + } } } @@ -2571,7 +2594,7 @@ void Generator::GenerateClassDeserializeBinary(const GeneratorOptions& options, " default:\n"); if (IsExtendable(desc)) { printer->Print( - " jspb.Message.readBinaryExtension(msg, reader, $extobj$,\n" + " jspb.Message.readBinaryExtension(msg, reader, $extobj$Binary,\n" " $class$.prototype.getExtension,\n" " $class$.prototype.setExtension);\n" " break;\n", @@ -2705,8 +2728,8 @@ void Generator::GenerateClassSerializeBinary(const GeneratorOptions& options, if (IsExtendable(desc)) { printer->Print( - " jspb.Message.serializeBinaryExtensions(this, writer, $extobj$,\n" - " $class$.prototype.getExtension);\n", + " jspb.Message.serializeBinaryExtensions(this, writer,\n" + " $extobj$Binary, $class$.prototype.getExtension);\n", "extobj", JSExtensionsObjectName(options, desc->file(), desc), "class", GetPath(options, desc)); } @@ -2874,7 +2897,7 @@ void Generator::GenerateExtension(const GeneratorOptions& options, " /** @type {?function((boolean|undefined),!jspb.Message=): " "!Object} */ (\n" " $toObject$),\n" - " $repeated$", + " $repeated$);\n", "index", SimpleItoa(field->number()), "name", JSObjectFieldName(options, field), "ctor", (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE ? @@ -2886,12 +2909,18 @@ void Generator::GenerateExtension(const GeneratorOptions& options, if (options.binary) { printer->Print( - ",\n" + "\n" + "$extendName$Binary[$index$] = new jspb.ExtensionFieldBinaryInfo(\n" + " $class$.$name$,\n" " $binaryReaderFn$,\n" " $binaryWriterFn$,\n" " $binaryMessageSerializeFn$,\n" - " $binaryMessageDeserializeFn$,\n" - " $isPacked$);\n", + " $binaryMessageDeserializeFn$,\n", + "extendName", JSExtensionsObjectName(options, field->file(), + field->containing_type()), + "index", SimpleItoa(field->number()), + "class", extension_scope, + "name", JSObjectFieldName(options, field), "binaryReaderFn", JSBinaryReaderMethodName(options, field), "binaryWriterFn", JSBinaryWriterMethodName(options, field), "binaryMessageSerializeFn", @@ -2901,10 +2930,11 @@ void Generator::GenerateExtension(const GeneratorOptions& options, "binaryMessageDeserializeFn", (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) ? (SubmessageTypeRef(options, field) + - ".deserializeBinaryFromReader") : "null", + ".deserializeBinaryFromReader") : "null"); + + printer->Print( + " $isPacked$);\n", "isPacked", (field->is_packed() ? "true" : "false")); - } else { - printer->Print(");\n"); } printer->Print( -- cgit v1.2.3 From e0e7377119b5ed6294ea9bc4b94a4208cfab897c Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Mon, 18 Jul 2016 20:49:11 -0700 Subject: Fix goog.require()/goog.provide() ordering. --- js/commonjs/export.js | 2 +- js/message.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'js') diff --git a/js/commonjs/export.js b/js/commonjs/export.js index 23ae47f9..fb56b30b 100644 --- a/js/commonjs/export.js +++ b/js/commonjs/export.js @@ -8,8 +8,8 @@ goog.require('goog.object'); goog.require('jspb.BinaryReader'); goog.require('jspb.BinaryWriter'); -goog.require('jspb.ExtensionFieldInfo'); goog.require('jspb.ExtensionFieldBinaryInfo'); +goog.require('jspb.ExtensionFieldInfo'); goog.require('jspb.Message'); exports.Message = jspb.Message; diff --git a/js/message.js b/js/message.js index f746ee62..631ebe69 100644 --- a/js/message.js +++ b/js/message.js @@ -34,8 +34,8 @@ * @author mwr@google.com (Mark Rawling) */ -goog.provide('jspb.ExtensionFieldInfo'); goog.provide('jspb.ExtensionFieldBinaryInfo'); +goog.provide('jspb.ExtensionFieldInfo'); goog.provide('jspb.Message'); goog.require('goog.array'); -- cgit v1.2.3