Bug 1276206 - Avoid caption clobber if more prop exists. r=linclark
authorJan Odvarko <odvarko@gmail.com>
Tue, 27 Sep 2016 16:30:01 +0200
changeset 316892 327bd3387ff6377bc9862890fc8c949213312201
parent 316891 41cee0dc469fcfd233279fc2756045fe583e048f
child 316893 97d32293dec48cd9e22b91c9f96799f6c77feac5
push id30786
push userryanvm@gmail.com
push dateFri, 07 Oct 2016 13:38:16 +0000
treeherdermozilla-central@9c11fedf4367 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslinclark
bugs1276206
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1276206 - Avoid caption clobber if more prop exists. r=linclark
devtools/client/shared/components/reps/array.js
devtools/client/shared/components/reps/grip-array.js
devtools/client/shared/components/reps/grip.js
devtools/client/shared/components/reps/object.js
devtools/client/shared/components/test/mochitest/test_reps_grip.html
devtools/client/shared/components/test/mochitest/test_reps_object.html
--- a/devtools/client/shared/components/reps/array.js
+++ b/devtools/client/shared/components/reps/array.js
@@ -33,36 +33,33 @@ define(function (require, exports, modul
 
       for (let i = 0; i < array.length && i < max; i++) {
         try {
           let value = array[i];
 
           delim = (i == array.length - 1 ? "" : ", ");
 
           items.push(ItemRep({
-            key: i,
             object: value,
             // Hardcode tiny mode to avoid recursive handling.
             mode: "tiny",
             delim: delim
           }));
         } catch (exc) {
           items.push(ItemRep({
-            key: i,
             object: exc,
             mode: "tiny",
             delim: delim
           }));
         }
       }
 
       if (array.length > max) {
         let objectLink = this.props.objectLink || DOM.span;
         items.push(Caption({
-          key: "more",
           object: objectLink({
             object: this.props.object
           }, (array.length - max) + " more…")
         }));
       }
 
       return items;
     },
@@ -119,34 +116,34 @@ define(function (require, exports, modul
       let items;
       let brackets;
       let needSpace = function (space) {
         return space ? { left: "[ ", right: " ]"} : { left: "[", right: "]"};
       };
 
       if (mode == "tiny") {
         let isEmpty = object.length === 0;
-        items = DOM.span({className: "length"}, isEmpty ? "" : object.length);
+        items = [DOM.span({className: "length"}, isEmpty ? "" : object.length)];
         brackets = needSpace(false);
       } else {
         let max = (mode == "short") ? 3 : 300;
         items = this.arrayIterator(object, max);
         brackets = needSpace(items.length > 0);
       }
 
       let objectLink = this.props.objectLink || DOM.span;
 
       return (
         DOM.span({
           className: "objectBox objectBox-array"},
           objectLink({
             className: "arrayLeftBracket",
             object: object
           }, brackets.left),
-          items,
+          ...items,
           objectLink({
             className: "arrayRightBracket",
             object: object
           }, brackets.right),
           DOM.span({
             className: "arrayProperties",
             role: "group"}
           )
--- a/devtools/client/shared/components/reps/grip-array.js
+++ b/devtools/client/shared/components/reps/grip-array.js
@@ -65,34 +65,31 @@ define(function (require, exports, modul
       for (let i = 0; i < array.length && i < max; i++) {
         try {
           let itemGrip = array[i];
           let value = provider ? provider.getValue(itemGrip) : itemGrip;
 
           delim = (i == delimMax ? "" : ", ");
 
           items.push(GripArrayItem(Object.assign({}, this.props, {
-            key: i,
             object: value,
-            delim: delim}
-          )));
+            delim: delim
+          })));
         } catch (exc) {
           items.push(GripArrayItem(Object.assign({}, this.props, {
             object: exc,
-            delim: delim,
-            key: i}
-          )));
+            delim: delim
+          })));
         }
       }
       if (array.length > max || grip.preview.length > array.length) {
         let objectLink = this.props.objectLink || span;
         let leftItemNum = grip.preview.length - max > 0 ?
           grip.preview.length - max : grip.preview.length - array.length;
         items.push(Caption({
-          key: "more",
           object: objectLink({
             object: this.props.object
           }, leftItemNum + " more…")
         }));
       }
 
       return items;
     },
@@ -105,17 +102,17 @@ define(function (require, exports, modul
       let brackets;
       let needSpace = function (space) {
         return space ? { left: "[ ", right: " ]"} : { left: "[", right: "]"};
       };
 
       if (mode == "tiny") {
         let objectLength = this.getLength(object);
         let isEmpty = objectLength === 0;
-        items = span({className: "length"}, isEmpty ? "" : objectLength);
+        items = [span({className: "length"}, isEmpty ? "" : objectLength)];
         brackets = needSpace(false);
       } else {
         let max = (mode == "short") ? 3 : 300;
         items = this.arrayIterator(object, max);
         brackets = needSpace(items.length > 0);
       }
 
       let objectLink = this.props.objectLink || span;
@@ -124,17 +121,17 @@ define(function (require, exports, modul
       return (
         span({
           className: "objectBox objectBox-array"},
           title,
           objectLink({
             className: "arrayLeftBracket",
             object: object
           }, brackets.left),
-          items,
+          ...items,
           objectLink({
             className: "arrayRightBracket",
             object: object
           }, brackets.right),
           span({
             className: "arrayProperties",
             role: "group"}
           )
--- a/devtools/client/shared/components/reps/grip.js
+++ b/devtools/client/shared/components/reps/grip.js
@@ -71,17 +71,16 @@ define(function (require, exports, modul
       }
 
       let props = this.getProps(ownProperties, indexes);
       if (props.length < object.ownPropertyLength) {
         // There are some undisplayed props. Then display "more...".
         let objectLink = this.props.objectLink || span;
 
         props.push(Caption({
-          key: "more",
           object: objectLink({
             object: object
           }, ((object ? object.ownPropertyLength : 0) - max) + " more…")
         }));
       } else if (props.length > 0) {
         // Remove the last comma.
         // NOTE: do not change comp._store.props directly to update a property,
         // it should be re-rendered or cloned with changed props
@@ -109,17 +108,16 @@ define(function (require, exports, modul
         return a - b;
       });
 
       indexes.forEach((i) => {
         let name = Object.keys(ownProperties)[i];
         let prop = ownProperties[name];
         let value = prop.value !== undefined ? prop.value : prop;
         props.push(PropRep(Object.assign({}, this.props, {
-          key: name,
           mode: "tiny",
           name: name,
           object: value,
           equal: ": ",
           delim: ", ",
           defaultRep: Grip
         })));
       });
@@ -185,17 +183,17 @@ define(function (require, exports, modul
 
       return (
         span({className: "objectBox objectBox-object"},
           this.getTitle(object),
           objectLink({
             className: "objectLeftBrace",
             object: object
           }, " { "),
-          props,
+          ...props,
           objectLink({
             className: "objectRightBrace",
             object: object
           }, " }")
         )
       );
     },
   });
--- a/devtools/client/shared/components/reps/object.js
+++ b/devtools/client/shared/components/reps/object.js
@@ -70,17 +70,16 @@ define(function (require, exports, modul
         }));
       }
 
       if (props.length > max) {
         props.pop();
         let objectLink = this.props.objectLink || span;
 
         props.push(Caption({
-          key: "more",
           object: objectLink({
             object: object
           }, (Object.keys(object).length - max) + " more…")
         }));
       } else if (props.length > 0) {
         // Remove the last comma.
         props[props.length - 1] = React.cloneElement(
           props[props.length - 1], { delim: "" });
@@ -111,17 +110,16 @@ define(function (require, exports, modul
             value = object[name];
           } catch (exc) {
             continue;
           }
 
           let t = typeof value;
           if (filter(t, value)) {
             props.push(PropRep({
-              key: name,
               mode: mode,
               name: name,
               object: value,
               equal: ": ",
               delim: ", ",
             }));
           }
         }
@@ -147,17 +145,17 @@ define(function (require, exports, modul
 
       return (
         span({className: "objectBox objectBox-object"},
           this.getTitle(object),
           objectLink({
             className: "objectLeftBrace",
             object: object
           }, " { "),
-          props,
+          ...props,
           objectLink({
             className: "objectRightBrace",
             object: object
           }, " }")
         )
       );
     },
   });
--- a/devtools/client/shared/components/test/mochitest/test_reps_grip.html
+++ b/devtools/client/shared/components/test/mochitest/test_reps_grip.html
@@ -26,16 +26,19 @@ window.onload = Task.async(function* () 
     // Test property iterator
     yield testMaxProps();
     yield testMoreThanMaxProps();
     yield testUninterestingProps();
 
     // Test that properties are rendered as expected by PropRep
     yield testNestedObject();
     yield testNestedArray();
+
+    // Test that 'more' property doesn't clobber the caption.
+    yield testMoreProp();
   } catch(e) {
     ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
   } finally {
     SimpleTest.finish();
   }
 
   function testBasic() {
     // Test object: `{}`
@@ -194,16 +197,45 @@ window.onload = Task.async(function* () 
         mode: "long",
         expectedOutput: defaultOutput,
       }
     ];
 
     testRepRenderModes(modeTests, testName, componentUnderTest, getGripStub(testName));
   }
 
+  function testMoreProp() {
+    // Test object: `{a: undefined, b: 1, more: 2, d: 3}`;
+    const testName = "testMoreProp";
+
+    const defaultOutput = `Object { b: 1, more: 2, d: 3, 1 more… }`;
+    const longOutput = `Object { a: undefined, b: 1, more: 2, d: 3 }`;
+
+    const modeTests = [
+      {
+        mode: undefined,
+        expectedOutput: defaultOutput,
+      },
+      {
+        mode: "tiny",
+        expectedOutput: `Object`,
+      },
+      {
+        mode: "short",
+        expectedOutput: defaultOutput,
+      },
+      {
+        mode: "long",
+        expectedOutput: longOutput,
+      }
+    ];
+
+    testRepRenderModes(modeTests, testName, componentUnderTest, getGripStub(testName));
+  }
+
   function getGripStub(functionName) {
     switch (functionName) {
       case "testBasic":
         return {
           "type": "object",
           "class": "Object",
           "actor": "server1.conn0.obj304",
           "extensible": true,
@@ -400,15 +432,59 @@ window.onload = Task.async(function* () 
                 }
               }
             },
             "ownPropertiesLength": 1,
             "safeGetterValues": {}
           },
         };
 
+      case "testMoreProp":
+        return {
+          "type": "object",
+          "class": "Object",
+          "actor": "server1.conn0.obj342",
+          "extensible": true,
+          "frozen": false,
+          "sealed": false,
+          "ownPropertyLength": 4,
+          "preview": {
+            "kind": "Object",
+            "ownProperties": {
+              "a": {
+                "configurable": true,
+                "enumerable": true,
+                "writable": true,
+                "value": {
+                  "type": "undefined"
+                }
+              },
+              "b": {
+                "configurable": true,
+                "enumerable": true,
+                "writable": true,
+                "value": 1
+              },
+              "more": {
+                "configurable": true,
+                "enumerable": true,
+                "writable": true,
+                "value": 2
+              },
+              "d": {
+                "configurable": true,
+                "enumerable": true,
+                "writable": true,
+                "value": 3
+              }
+            },
+            "ownPropertiesLength": 4,
+            "safeGetterValues": {}
+          }
+        };
+
     }
   }
 });
 </script>
 </pre>
 </body>
 </html>
--- a/devtools/client/shared/components/test/mochitest/test_reps_object.html
+++ b/devtools/client/shared/components/test/mochitest/test_reps_object.html
@@ -25,16 +25,19 @@ window.onload = Task.async(function* () 
 
     // Test property iterator
     yield testMaxProps();
     yield testMoreThanMaxProps();
     yield testUninterestingProps();
 
     // Test that properties are rendered as expected by PropRep
     yield testNested();
+
+    // Test that 'more' property doesn't clobber the caption.
+    yield testMoreProp();
   } catch(e) {
     ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
   } finally {
     SimpleTest.finish();
   }
 
   function testBasic() {
     const stub = {};
@@ -178,13 +181,43 @@ window.onload = Task.async(function* () 
       {
         mode: "long",
         expectedOutput: defaultOutput,
       }
     ];
 
     testRepRenderModes(modeTests, "testNestedObject", componentUnderTest, stub);
   }
-});
+
+  function testMoreProp() {
+    const stub = {
+      a: undefined,
+      b: 1,
+      'more': 2,
+      d: 3
+    };
+    const defaultOutput = `Object { b: 1, more: 2, d: 3, 1 more… }`;
+
+    const modeTests = [
+      {
+        mode: undefined,
+        expectedOutput: defaultOutput,
+      },
+      {
+        mode: "tiny",
+        expectedOutput: `Object`,
+      },
+      {
+        mode: "short",
+        expectedOutput: defaultOutput,
+      },
+      {
+        mode: "long",
+        expectedOutput: defaultOutput,
+      }
+    ];
+
+    testRepRenderModes(modeTests, "testMoreProp", componentUnderTest, stub);
+  }});
 </script>
 </pre>
 </body>
 </html>