Closed
Bug 1264686
Opened 9 years ago
Closed 8 years ago
Reps: Use grip-array rep to display NamedNodeMap
Categories
(DevTools :: Shared Components, enhancement, P1)
DevTools
Shared Components
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: linclark, Assigned: gasolin)
References
Details
(Whiteboard: [reserve-html])
Attachments
(1 file)
See Bug 1257552
Updated•9 years ago
|
Severity: normal → enhancement
Whiteboard: [devtools-html]
Updated•9 years ago
|
Blocks: devtools-html-2
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Reporter | ||
Comment 1•8 years ago
|
||
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?
Flags: needinfo?(odvarko)
Comment 2•8 years ago
|
||
(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
Flags: needinfo?(odvarko) → needinfo?(lclark)
Reporter | ||
Comment 3•8 years ago
|
||
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.
Flags: needinfo?(lclark)
Reporter | ||
Updated•8 years ago
|
Summary: [rep tests] Add tests for named-node-map rep → Reps: Use grip-array rep to display NamedNodeMap
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•8 years ago
|
||
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[]
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/65392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65392/
Attachment #8772652 -
Flags: review?(lclark)
Assignee | ||
Comment 6•8 years ago
|
||
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
Attachment #8772652 -
Flags: review?(lclark)
Assignee | ||
Comment 7•8 years ago
|
||
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/
Attachment #8772652 -
Flags: review?(lclark)
Assignee | ||
Comment 8•8 years ago
|
||
added test case (NamedNodeMap grip object is pasted from rep tester)
Local test passed, triggered try run to see the result.
Assignee | ||
Comment 9•8 years ago
|
||
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/
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
all test passed, please kindly review it
Reporter | ||
Comment 12•8 years ago
|
||
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?
Attachment #8772652 -
Flags: review?(lclark) → review-
Assignee | ||
Comment 13•8 years ago
|
||
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/
Attachment #8772652 -
Attachment description: Bug 1264686 - Use grip-array rep to display NamedNodeMap; → Bug 1264686 - Reps: Use grip-array rep to display NamedNodeMap;
Attachment #8772652 -
Flags: review- → review?(lclark)
Assignee | ||
Comment 14•8 years ago
|
||
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]
Assignee | ||
Comment 15•8 years ago
|
||
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.
Attachment #8772652 -
Flags: feedback?(odvarko)
Assignee | ||
Comment 16•8 years ago
|
||
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/
Attachment #8772652 -
Flags: feedback?(odvarko)
Assignee | ||
Updated•8 years ago
|
Attachment #8772652 -
Flags: feedback?(odvarko)
Comment 17•8 years ago
|
||
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
Attachment #8772652 -
Flags: feedback?(odvarko) → feedback+
Assignee | ||
Comment 18•8 years ago
|
||
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/
Attachment #8772652 -
Flags: feedback+
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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/
Reporter | ||
Comment 21•8 years ago
|
||
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.
Attachment #8772652 -
Flags: review?(lclark) → review+
Reporter | ||
Comment 22•8 years ago
|
||
Nevermind, Honza's out of town. I'd say this one is ready to go.
Assignee | ||
Comment 23•8 years ago
|
||
Thanks!
Honza has given feedback+ at comment 17
Keywords: checkin-needed
Comment 24•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/68e53c6c8494
Reps: Use grip-array rep to display NamedNodeMap. r=linclark
Keywords: checkin-needed
Comment 25•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•8 years ago
|
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•