1264686 - Reps: Use grip-array rep to display NamedNodeMap

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Lin Clark [:linclark]]

See Bug 1257552

Comment 1

[Lin Clark [:linclark]]

Do we need this rep? Given that bug you posted, and looking at the returned grip, it seems like NamedNodeMap is a standard grip-array. Could we just use that rep?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #1)
> Do we need this rep? Given that bug you posted, and looking at the returned
> grip, it seems like NamedNodeMap is a standard grip-array. Could we just use
> that rep?
Yes, I like having just one. The only concern I have is that the user should be able to see whether the rendered thing is regular array or NamedNodeMap.

What if we moved getTitle() method to ArrayRep and displayed "NamedNodeMap" label in front of the value in case of NamedNodeMap type?

Honza

Comment 3

[Lin Clark [:linclark]]

I think that makes sense (I'm assuming you mean GripArray and not ArrayRep). We might even just have all grip-arrays display their kind... would be worth looking into when we make the change.

Comment 4

[[e-mail]]

Here's how I test NamedNodeMap

1. set devtools.webconsole.new-frontend-enabled;true in about:config
2. open http://dbaron.org/dom/test/one-core-html/NamedNodeMap
3. open webconsole 

> tableelem = document.getElementsByTagName("table").item(0);
> nnm = tableelem.attributes;
<- NamedNodeMap[]

Comment 5

[[e-mail]]

Attachment: Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review commit: https://reviewboard.mozilla.org/r/65392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65392/

Comment 6

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

will add basic test in test_reps_grip-array.html before set review

Comment 7

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/1-2/

Comment 8

[[e-mail]]

added test case (NamedNodeMap grip object is pasted from rep tester)

Local test passed, triggered try run to see the result.

Comment 9

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/2-3/

Comment 10

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/3-4/

Comment 11

[[e-mail]]

all test passed, please kindly review it

Comment 12

[Lin Clark [:linclark]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

https://reviewboard.mozilla.org/r/65392/#review62508

Thanks for the patch. This is going in the right direction. Just a few comments below.

::: devtools/client/shared/components/reps/grip-array.js:189
(Diff revision 4)
>    function supportsObject(grip, type) {
>      if (!isGrip(grip)) {
>        return false;
>      }
>  
> -    return (grip.preview && grip.preview.kind == "ArrayLike");
> +    return (grip.preview && grip.preview.kind == "ArrayLike") ||

We don't need to have the comment.

Also, I don't think you need to change this code at all. A NamedNodeMap should be an ArrayLike already. See https://github.com/linclark/webconsole-rdp/blob/f3a3072b25b199a8b858552e3c69e0cea18e885f/data/evaluationResult/NamedNodeMap/attributes.json#L261

::: devtools/client/shared/components/test/mochitest/test_reps_grip-array.html:162
(Diff revision 4)
>    }
>  
> +  function testNamedNodeMap() {
> +    const testName = "testNamedNodeMap";
> +
> +    const defaultOutput = `[class="myclass", cellpadding="7", border="3"]`;

The type (NamedNodeMap) should be displayed in this case even though there isn't an object link component provided.

We may want to display the type for all ArrayLikes. We should get Honza's opinion on that.

::: devtools/client/shared/components/test/mochitest/test_reps_grip-array.html:166
(Diff revision 4)
> +
> +    const defaultOutput = `[class="myclass", cellpadding="7", border="3"]`;
> +
> +    const modeTests = [
> +      {
> +        mode: "long",

Why does this only test long mode? Shouldn't it test all modes like the other tests?

Comment 13

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/4-5/

Comment 14

[[e-mail]]

Thanks for review. I've add test case for all mode, modified getTitle to always show the title when not in tiny mode, even there isn't an object link component provided. The output result will looks like:

> [1,2,3]
Array[1,2,3]

(tinymode)
[3]

> nnm
NodeMap[a="1",b="2",c="3"]

(tinymode)
[3]

Comment 15

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

> We may want to display the type for all ArrayLikes. We should get Honza's opinion on that.

Honza, please help check if `title[items]` is the expected output format for grip-arrays. You can check the test cases in the above patch.

Comment 16

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/5-6/

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

(In reply to Fred Lin [:gasolin] from comment #15)
> Comment on attachment 8772652 [details]
> Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;
> 
> > We may want to display the type for all ArrayLikes. We should get Honza's opinion on that.
> 
> Honza, please help check if `title[items]` is the expected output format for
Yes, this is correct.

One quick thing related to the patch

> getTitle: function (object, context) {
I noticed that you are passing `objectLink` and `mode` in an argument called `context`. A `context` is an existing concept in React and using this name is a bit misleading. I'd prefer to avoid it. Also, you can access both (objectLink & mode) arguments within the getTitle function through `this.props`, which I think is better.

Thanks for working on this.
Honza

Comment 18

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/6-7/

Comment 19

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/7-8/

Comment 20

[[e-mail]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65392/diff/8-9/

Comment 21

[Lin Clark [:linclark]]

Comment on attachment 8772652 [details]
Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;

https://reviewboard.mozilla.org/r/65392/#review63150

This looks good to me, but I'd like to have Honza give it a look too.

Comment 22

[Lin Clark [:linclark]]

Nevermind, Honza's out of town. I'd say this one is ready to go.

Comment 23

[[e-mail]]

Thanks!

Honza has given feedback+ at comment 17

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/68e53c6c8494
Reps: Use grip-array rep to display NamedNodeMap. r=linclark

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68e53c6c8494