Closed Bug 1282791 Opened 4 years ago Closed 3 years ago

Reps: uninteresting props algorithm is wrong

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: linclark, Assigned: evanxd)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file, 3 obsolete files)

See Bug 1264676, comment #2

> Imagine the following object:
> 
> 
> let obj = {
>   p1: "John"    // Interesting prop
>   p2: [1,2,3],  // Not interesting prop
>   p3: true      // Interesting prop
> }
>
>Rendered object would look like as follows:
>Object{p1: "John", p3: true, p2: [1, 2, 3]}
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
Here is more info about this bug.

When an object is rendered using Grip rep (shared/components/reps/grip) there is a simple logic that picks interesting properties to render.

Let's imagine the following object:

let obj = {
  p1: null,     // Not interesting prop
  p2: null,     // Not interesting prop
  p3: null,     // Not interesting prop
  p4: "John",   // Interesting prop
  p5: null,     // Not interesting prop
  p6: null,     // Not interesting prop
  p7: true      // Interesting prop
}

Such object could be rendered as follows:
Object{p1: null, p2: null, p3: null}

Rendered object would look like as follows:
Object{p1: "John", p3: true, p2: [1, 2, 3]}


* The algorithm
Summary: Reps: uninteresting props algorithm reorders props → Reps: uninteresting props algorithm is wrong
The previous comment is not finished. Here is updated version

When an object is rendered using Grip rep (shared/components/reps/grip) there is a simple logic that picks interesting properties to render.

Let's imagine the following object:

window._obj = {
  p1: null,     // Not interesting prop
  p2: null,     // Not interesting prop
  p3: null,     // Not interesting prop
  p4: "John",   // Interesting prop
  p5: null,     // Not interesting prop
  p6: 5,        // Interesting prop
  p7: true      // Interesting prop
}
Such object could be rendered as follows:
Object{p1: null, p2: null, p3: null, more...}

But it doesn't provide useful info to the user. Better is to pick 'interesting' props that represent the the object nicely and can be used by the user to identify the instance.

Let's see better version:
Object {p4: "John", p6: 5, p7: true, more...}

In this case, the user can see a lot more info about the object being rendered.

---

Now, there are following issues with the current implementation:

1) Null object and empty strings are considered being 'interested' => bug they are not. The filter function should rather looks as follows:

let isInterestingProp = (type, value) => {
  return (
    type == "boolean" ||
    type == "number" ||
    (type == "string" && value.type != "null") ||
    (type == "object" && value.type != "null")
  );
};

Needs to be tested with various values.

2)  When there is not enough interested props to show, the algorithm should pick more from the rest. Let's see an example:

window._obj = {
  p1: null,     // Not interesting prop
  p2: null,     // Not interesting prop
  p3: null,     // Not interesting prop
  p4: "John",   // Interesting prop
  p5: null,     // Not interesting prop
  p6: null,     // Not interesting prop
  p7: null      // Not interesting prop
}

Object {p4: "John", p1: null, p2: null, more...}

But props are reordered -> bug, the order should be: p1, p2, p3, p4

3) The number of displayed props is wrong. 

When rendering the following object in short mode (max 3 props):

window._obj = {
  p1: null,     // Not interesting prop
  p2: null,     // Not interesting prop
  p3: null,     // Not interesting prop
  p4: "John",   // Interesting prop
  p5: null,     // Not interesting prop
  p6: 5,        // Interesting prop
  p7: true      // Interesting prop
}

Following is the actual output:
Object {p4: "John", p6: 5, p7: true, p1: null, p2: null, p3: null, more...}

There are 6 props -> BUG there should be just 3!

All should be covered by a test (we might want to enhance the existing test shared/components/tests/mochitest/test_reps_grip.html)

---

There is also bug 1276376 related to this.

Honza
Assignee: nobody → evan
I am using the following tool for testing reps:
https://github.com/janodvarko/prototypes/tree/master/reps

There is a lot what could be simplified on the tool, but it does its job.
Let me know if there is something we should really improve of if you have
troubles to run it.

Honza
Status: NEW → ASSIGNED
Hi Honza,

Thanks a lot for Comment 2. It's really clear. Now I can understand what we want to do in this bug.

Two questions here:
1. Why nothing happened after change the filter function[1]?
I changed the filter function like below
```
let isInterestingProp = (type, value) => {
  return (
    type == "string" ||
    type == "object"
  );
};
```
And input the below object in web console.
```
var obj = {
  p1: 1,
  p2: null,
  p3: true
}
```
The output should be `Object { p2: null }` but it still outputs `Object { p1: 1, p2: null, p3: true }`.

Looks like changing filter function cannot change the web console output?

2. About the short mode. What situation will be the short mode? And what situation will be the long mode? For example, is web console always in the short mode?

Thanks.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/reps/grip.js#57-64
(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> I am using the following tool for testing reps:
> https://github.com/janodvarko/prototypes/tree/master/reps
> 
> There is a lot what could be simplified on the tool, but it does its job.
> Let me know if there is something we should really improve of if you have
> troubles to run it.
> 
> Honza

Hi Honza,

I got troubles of running Rep Tester. The below is what I did and got the error message.

1. Run `npm install` to install dependency modules of Rep Tester.
2. Change the alias of `devtools` directory to `"devtools": path.join(__dirname, "../../mozilla-central/devtools")`[1] because the path of `prototypes` directory is `the/path/mozilla/prototypes/reps` and the path of `mozilla-central` directory is `the/path/mozilla/mozilla-central/devtools`.
3. Run `npm start`.

Then get the below error message.
```
ERROR in the/path/mozilla/mozilla-central/devtools/client/shared/components/reps/rep-utils.js
Module build failed: Error: Couldn't find preset "es2015" relative to directory "the/path/mozilla/mozilla-central/devtools/client/shared/components/reps"
    at the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:395:17
    at Array.map (native)
    at OptionManager.resolvePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:387:20)
    at OptionManager.mergePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:370:10)
    at OptionManager.mergeOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:330:14)
    at OptionManager.init (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:488:10)
    at File.initOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:223:65)
    at new File (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:140:24)
    at Pipeline.transform (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at transpile (the/path/mozilla/prototypes/reps/node_modules/babel-loader/index.js:14:22)
 @ ./index.js 8:15-74
 @ multi main

ERROR in the/path/mozilla/mozilla-central/devtools/client/shared/components/reps/grip.js
Module build failed: Error: Couldn't find preset "es2015" relative to directory "the/path/mozilla/mozilla-central/devtools/client/shared/components/reps"
    at the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:395:17
    at Array.map (native)
    at OptionManager.resolvePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:387:20)
    at OptionManager.mergePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:370:10)
    at OptionManager.mergeOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:330:14)
    at OptionManager.init (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:488:10)
    at File.initOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:223:65)
    at new File (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:140:24)
    at Pipeline.transform (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at transpile (the/path/mozilla/prototypes/reps/node_modules/babel-loader/index.js:14:22)
 @ ./index.js 16:16-70
 @ multi main

ERROR in the/path/mozilla/mozilla-central/devtools/client/shared/components/reps/rep.js
Module build failed: Error: Couldn't find preset "es2015" relative to directory "/Users/itoyxd/evan/dev/mozilla/mozilla-central/devtools/client/shared/components/reps"
    at the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:395:17
    at Array.map (native)
    at OptionManager.resolvePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:387:20)
    at OptionManager.mergePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:370:10)
    at OptionManager.mergeOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:330:14)
    at OptionManager.init (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:488:10)
    at File.initOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:223:65)
    at new File (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:140:24)
    at Pipeline.transform (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at transpile (the/path/mozilla/prototypes/reps/node_modules/babel-loader/index.js:14:22)
 @ ./index.js 12:39-92
 @ multi main

ERROR in the/path/mozilla/mozilla-central/devtools/client/shared/components/tree/tree-view.js
Module build failed: Error: Couldn't find preset "es2015" relative to directory "the/path/mozilla/mozilla-central/devtools/client/shared/components/tree"
    at the/path/dev/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:395:17
    at Array.map (native)
    at OptionManager.resolvePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:387:20)
    at OptionManager.mergePresets (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:370:10)
    at OptionManager.mergeOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:330:14)
    at OptionManager.init (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/options/option-manager.js:488:10)
    at File.initOptions (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:223:65)
    at new File (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/file/index.js:140:24)
    at Pipeline.transform (the/path/mozilla/prototypes/reps/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at transpile (the/path/mozilla/prototypes/reps/node_modules/babel-loader/index.js:14:22)
 @ ./index.js 25:35-94
 @ multi main
```

And then, if I click the `Connect` button on `http://localhost:3333`(Reps Tester), the web console will show the `ReferenceError: onConnect is not defined` error.

Honza, did you get the troubles before?

Thanks.

[1]: https://github.com/janodvarko/prototypes/blob/master/reps/webpack.config.js#L41
Flags: needinfo?(odvarko)
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #5)
> (In reply to Jan Honza Odvarko [:Honza] from comment #3)
> > I am using the following tool for testing reps:
> > https://github.com/janodvarko/prototypes/tree/master/reps
> > 
> > There is a lot what could be simplified on the tool, but it does its job.
> > Let me know if there is something we should really improve of if you have
> > troubles to run it.
> > 
> > Honza
> 
> Hi Honza,
> 
> I got troubles of running Rep Tester. The below is what I did and got the
> error message.
> 
> 1. Run `npm install` to install dependency modules of Rep Tester.
> 2. Change the alias of `devtools` directory to `"devtools":
> path.join(__dirname, "../../mozilla-central/devtools")`[1] because the path
> of `prototypes` directory is `the/path/mozilla/prototypes/reps` and the path
> of `mozilla-central` directory is
> `the/path/mozilla/mozilla-central/devtools`.
> 3. Run `npm start`.
Did you also clone the debugger.html repo?
https://github.com/jlongster/debugger.html.git

`npm start` should be executed in debugger.html repo

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> (In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #5)
> > (In reply to Jan Honza Odvarko [:Honza] from comment #3)
> > > I am using the following tool for testing reps:
> > > https://github.com/janodvarko/prototypes/tree/master/reps
> > > 
> > > There is a lot what could be simplified on the tool, but it does its job.
> > > Let me know if there is something we should really improve of if you have
> > > troubles to run it.
> > > 
> > > Honza
> > 
> > Hi Honza,
> > 
> > I got troubles of running Rep Tester. The below is what I did and got the
> > error message.
> > 
> > 1. Run `npm install` to install dependency modules of Rep Tester.
> > 2. Change the alias of `devtools` directory to `"devtools":
> > path.join(__dirname, "../../mozilla-central/devtools")`[1] because the path
> > of `prototypes` directory is `the/path/mozilla/prototypes/reps` and the path
> > of `mozilla-central` directory is
> > `the/path/mozilla/mozilla-central/devtools`.
> > 3. Run `npm start`.
> Did you also clone the debugger.html repo?
> https://github.com/jlongster/debugger.html.git
> 
> `npm start` should be executed in debugger.html repo
> 
> Honza

Hi Honza,

Yes, I followed the instructions on the page.
I think I can test my patch with DOM panel. Then I'll continue to fix the problem.
Hi Honza,

For the Rep Tester problem, in my experience, it might be related with node version. My node version is `v5.10.1`. Do you know what version will be good for Rep Tester, or what is your node version?

And a little suggestion, we can move Rep Tester from the prototypes repo to a individual repo and publish on npm website. It will be good for it's maintenance and usage.
Hi Honza,

One question about the reorder issue in Comment 2.

How about we just reorder the below props?

window.obj = {
  p1: null,     // Not interesting prop
  p2: null,     // Not interesting prop
  p3: null,     // Not interesting prop
  p4: "John",   // Interesting prop
  p5: null,     // Not interesting prop
  p6: null,     // Not interesting prop
  p7: null      // Not interesting prop
}


(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> The previous comment is not finished. Here is updated version
> 
> When an object is rendered using Grip rep (shared/components/reps/grip)
> there is a simple logic that picks interesting properties to render.
> 
> Let's imagine the following object:
> 
> window._obj = {
>   p1: null,     // Not interesting prop
>   p2: null,     // Not interesting prop
>   p3: null,     // Not interesting prop
>   p4: "John",   // Interesting prop
>   p5: null,     // Not interesting prop
>   p6: 5,        // Interesting prop
>   p7: true      // Interesting prop
> }
> Such object could be rendered as follows:
> Object{p1: null, p2: null, p3: null, more...}
> 
> But it doesn't provide useful info to the user. Better is to pick
> 'interesting' props that represent the the object nicely and can be used by
> the user to identify the instance.
> 
> Let's see better version:
> Object {p4: "John", p6: 5, p7: true, more...}
> 
> In this case, the user can see a lot more info about the object being
> rendered.
> 
> ---
> 
> Now, there are following issues with the current implementation:
> 
> 1) Null object and empty strings are considered being 'interested' => bug
> they are not. The filter function should rather looks as follows:
> 
> let isInterestingProp = (type, value) => {
>   return (
>     type == "boolean" ||
>     type == "number" ||
>     (type == "string" && value.type != "null") ||
>     (type == "object" && value.type != "null")
>   );
> };
> 
> Needs to be tested with various values.
> 
> 2)  When there is not enough interested props to show, the algorithm should
> pick more from the rest. Let's see an example:
> 
> window._obj = {
>   p1: null,     // Not interesting prop
>   p2: null,     // Not interesting prop
>   p3: null,     // Not interesting prop
>   p4: "John",   // Interesting prop
>   p5: null,     // Not interesting prop
>   p6: null,     // Not interesting prop
>   p7: null      // Not interesting prop
> }
> 
> Object {p4: "John", p1: null, p2: null, more...}
> 
> But props are reordered -> bug, the order should be: p1, p2, p3, p4

I think we can try to reorder for this situation. The reason I think the way is that if we have the below object in the long mode and do not reorder props, it will not be easy to see the interesting props.

// The p1 to p100 props are null.
window.obj = {
  p1: null,
  p2: null,
  p3: null,
  ...
  p98: null,
  p99: null,
  p100: null,
  p101: 1
}

If we don't reorder the props, the output will be `Object { p1: null, p2: null, p3: null, ..., p99:null, p101: 1, more...}` User will see 99 uninteresting props first then see the interesting prop p101. I think it might not good to users. How do you think we reorder the props? Then the output will be `Object { p101: 1, p1: null, p2: null, p3: null, ..., p99:null, more... }` It might be easier for users to check the interesting props.

> 3) The number of displayed props is wrong. 
> 
> When rendering the following object in short mode (max 3 props):
> 
> window._obj = {
>   p1: null,     // Not interesting prop
>   p2: null,     // Not interesting prop
>   p3: null,     // Not interesting prop
>   p4: "John",   // Interesting prop
>   p5: null,     // Not interesting prop
>   p6: 5,        // Interesting prop
>   p7: true      // Interesting prop
> }
> 
> Following is the actual output:
> Object {p4: "John", p6: 5, p7: true, p1: null, p2: null, p3: null, more...}
> 
> There are 6 props -> BUG there should be just 3!
> 
> All should be covered by a test (we might want to enhance the existing test
> shared/components/tests/mochitest/test_reps_grip.html)
> 
> ---
> 
> There is also bug 1276376 related to this.
> 
> Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> The previous comment is not finished. Here is updated version
> 
> When an object is rendered using Grip rep (shared/components/reps/grip)
> there is a simple logic that picks interesting properties to render.
> 
> Let's imagine the following object:
> 
> window._obj = {
>   p1: null,     // Not interesting prop
>   p2: null,     // Not interesting prop
>   p3: null,     // Not interesting prop
>   p4: "John",   // Interesting prop
>   p5: null,     // Not interesting prop
>   p6: 5,        // Interesting prop
>   p7: true      // Interesting prop
> }
> Such object could be rendered as follows:
> Object{p1: null, p2: null, p3: null, more...}
> 
> But it doesn't provide useful info to the user. Better is to pick
> 'interesting' props that represent the the object nicely and can be used by
> the user to identify the instance.
> 
> Let's see better version:
> Object {p4: "John", p6: 5, p7: true, more...}
> 
> In this case, the user can see a lot more info about the object being
> rendered.
> 
> ---
> 
> Now, there are following issues with the current implementation:
> 
> 1) Null object and empty strings are considered being 'interested' => bug
> they are not. The filter function should rather looks as follows:
> 
> let isInterestingProp = (type, value) => {
>   return (
>     type == "boolean" ||
>     type == "number" ||
>     (type == "string" && value.type != "null") ||
>     (type == "object" && value.type != "null")
>   );
> };
> 
> Needs to be tested with various values.
> 
> 2)  When there is not enough interested props to show, the algorithm should
> pick more from the rest. Let's see an example:
> 
> window._obj = {
>   p1: null,     // Not interesting prop
>   p2: null,     // Not interesting prop
>   p3: null,     // Not interesting prop
>   p4: "John",   // Interesting prop
>   p5: null,     // Not interesting prop
>   p6: null,     // Not interesting prop
>   p7: null      // Not interesting prop
> }
> 
> Object {p4: "John", p1: null, p2: null, more...}
> 
> But props are reordered -> bug, the order should be: p1, p2, p3, p4

I think we can try to reorder for this situation. The reason I think the way is that if we have the below object in the long mode and do not reorder props, it will not be easy to see the interesting props.

// The p1 to p100 props are null.
window.obj = {
  p1: null,
  p2: null,
  p3: null,
  ...
  p98: null,
  p99: null,
  p100: null,
  p101: 1
}

If we don't reorder the props, the output will be `Object { p1: null, p2: null, p3: null, ..., p99:null, p101: 1, more...}` User will see 99 uninteresting props first then see the interesting prop p101. I think it might not good to users. How do you think we reorder the props? Then the output will be `Object { p101: 1, p1: null, p2: null, p3: null, ..., p99:null, more... }` It might be easier for users to check the interesting props.

> 3) The number of displayed props is wrong. 
> 
> When rendering the following object in short mode (max 3 props):
> 
> window._obj = {
>   p1: null,     // Not interesting prop
>   p2: null,     // Not interesting prop
>   p3: null,     // Not interesting prop
>   p4: "John",   // Interesting prop
>   p5: null,     // Not interesting prop
>   p6: 5,        // Interesting prop
>   p7: true      // Interesting prop
> }
> 
> Following is the actual output:
> Object {p4: "John", p6: 5, p7: true, p1: null, p2: null, p3: null, more...}
> 
> There are 6 props -> BUG there should be just 3!
> 
> All should be covered by a test (we might want to enhance the existing test
> shared/components/tests/mochitest/test_reps_grip.html)
> 
> ---
> 
> There is also bug 1276376 related to this.
> 
> Honza
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #11)
> // The p1 to p100 props are null.
> window.obj = {
>   p1: null,
>   p2: null,
>   p3: null,
>   ...
>   p98: null,
>   p99: null,
>   p100: null,
>   p101: 1
> }
> 
> If we don't reorder the props, the output will be `Object { p1: null, p2:
> null, p3: null, ..., p99:null, p101: 1, more...}` User will see 99
> uninteresting props first then see the interesting prop p101. I think it
> might not good to users. How do you think we reorder the props? Then the
> output will be `Object { p101: 1, p1: null, p2: null, p3: null, ...,
> p99:null, more... }` It might be easier for users to check the interesting
> props.
Good question. I think we should keep the order as is, to be consistent with the behavior in other modes. Otherwise the order would be different depending on what mode is used, which would lead to confusion.

Honza
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #9)
> Hi Honza,
> 
> For the Rep Tester problem, in my experience, it might be related with node
> version. My node version is `v5.10.1`. Do you know what version will be good
> for Rep Tester, or what is your node version?

My node version is 5.6.0

> And a little suggestion, we can move Rep Tester from the prototypes repo to
> a individual repo and publish on npm website. It will be good for it's
> maintenance and usage.
Agree, done.

There is a new repo at: https://github.com/janodvarko/rep-tester

Honza
Here are two comments that initiated this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1264676#c2
https://bugzilla.mozilla.org/show_bug.cgi?id=1264685#c4

Evan, please let me know if you need any help with this bug.

Honza
Attached patch bug-1282791.patch (obsolete) — Splinter Review
Hi Honza,

Could you help to review the patch?

I'll continue to add tests for it tomorrow.

Thanks.
Attachment #8772425 - Flags: review?(odvarko)
Attachment #8772425 - Attachment is obsolete: true
Attachment #8772425 - Flags: review?(odvarko)
Attached patch bug-1282791.patch (obsolete) — Splinter Review
Hi Honza,

I fixed the conflict issues. Please help to review the patch.

Thanks.
Attachment #8772798 - Flags: review?(odvarko)
Priority: P3 → P2
Comment on attachment 8772798 [details] [diff] [review]
bug-1282791.patch

Review of attachment 8772798 [details] [diff] [review]:
-----------------------------------------------------------------

I tested this and it works for me, good job here!

Please see my comments inline.

More comments:

1) Make sure the code passes eslint check (looks like you don't use it)
2) More comprehensible comments in general would help to understand the code better.
3) Make sure all tests pass.

Thanks,
Honza

::: devtools/client/shared/components/reps/grip.js
@@ +63,5 @@
>          return (
>            type == "boolean" ||
>            type == "number" ||
> +          (type == "string" && value.length != 0) ||
> +          (type == "object" && value.type != "null")

This line should be removed, Object type is not interesting.

@@ +69,5 @@
>        };
>  
> +      let ownProperties = object.preview ? object.preview.ownProperties : [];
> +      let index = this.getInterestingIndexes(ownProperties, max, isInterestingProp);
> +      if (index.length < max && index.length < object.ownPropertyLength) {

Please use different name for the array with indexes (e.g. `indexes` or `indexArr`...)

@@ +71,5 @@
> +      let ownProperties = object.preview ? object.preview.ownProperties : [];
> +      let index = this.getInterestingIndexes(ownProperties, max, isInterestingProp);
> +      if (index.length < max && index.length < object.ownPropertyLength) {
> +        // There are not enough props yet. Then add uninteresting props to display them.
> +        index = index.concat(this.getInterestingIndexes(ownProperties, max - index.length, (t, value) => {

This line exceeds 90 chars, please make it shorter.

@@ +131,5 @@
> +      return props;
> +    },
> +
> +    /**
> +     * Get the indexes of interesting props in the object.

The comment in misleading, the function can also return indexes of uninteresting props - depends on the filter. Please fix the comment.

@@ +139,5 @@
> +     * @param {Function} filter Filter the interesting props.
> +     * @return {Array} Indexes of interesting props in the object.
> +     */
> +    getInterestingIndexes: function (ownProperties, max, filter) {
> +      let index = [];

Please use different name for the array (preferable the same choice as in the above code)

@@ +162,2 @@
>            }
> +          i++

missing semicolon
Attachment #8772798 - Flags: review?(odvarko)
Attachment #8772798 - Attachment is obsolete: true
Attached patch bug-1282791.patch (obsolete) — Splinter Review
Hi Honza,

I updated the patch for your comments.

> 1) Make sure the code passes eslint check (looks like you don't use it)
I fixed all eslint issues.

> 2) More comprehensible comments in general would help to understand the code
> better.
I updated the comments of `getPropIndexes` function.

> 3) Make sure all tests pass.
Yes, I ran `./mach test`, and it showed
```
Summary
=======

Ran 18 tests
Expected results: 18
Unexpected results: 0

OK
```

Please help to review the patch.
Thanks.
Attachment #8773172 - Flags: review?(odvarko)
Comment on attachment 8773172 [details] [diff] [review]
bug-1282791.patch

Review of attachment 8773172 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update.

Couple more comments.

Honza

::: devtools/client/shared/components/reps/grip.js
@@ -113,5 @@
>  
> -      max = max || 3;
> -      if (!object) {
> -        return props;
> -      }

This piece of code is no longer used. Is that ok? (`max` updated to 3 in not set and checking whether `object` is defined)

Honza

@@ +149,2 @@
>          for (let name in ownProperties) {
> +          if (indexes.length == max) {

Nit: use `if (indexes.length >= max) {`
Attachment #8773172 - Flags: review?(odvarko)
Comment on attachment 8773172 [details] [diff] [review]
bug-1282791.patch

Review of attachment 8773172 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/components/reps/grip.js
@@ -113,5 @@
>  
> -      max = max || 3;
> -      if (!object) {
> -        return props;
> -      }

Hi Honza,

Yes, we can remove the code. Because the value `max` might be 1 or 2. And we don't pass `object` to `getPropIndexes` function anymore, we pass `ownProperties` now.

If we want to do checking params things here, we might do
```
if (!ownProperties || max == 0) {
  return indexes;
}
```

But I think we don't need to do checking params things here. How do you think?

@@ +149,2 @@
>          for (let name in ownProperties) {
> +          if (indexes.length == max) {

Sure.
Flags: needinfo?(odvarko)
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #22)
> Comment on attachment 8773172 [details] [diff] [review]
> bug-1282791.patch
> 
> Review of attachment 8773172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/components/reps/grip.js
> @@ -113,5 @@
> >  
> > -      max = max || 3;
> > -      if (!object) {
> > -        return props;
> > -      }
> 
> Hi Honza,
> 
> Yes, we can remove the code. Because the value `max` might be 1 or 2. And we
> don't pass `object` to `getPropIndexes` function anymore, we pass
> `ownProperties` now.
> 
> If we want to do checking params things here, we might do
> ```
> if (!ownProperties || max == 0) {
>   return indexes;
> }
> ```
> 
> But I think we don't need to do checking params things here. How do you
> think?
OK, sounds good.

> 
> @@ +149,2 @@
> >          for (let name in ownProperties) {
> > +          if (indexes.length == max) {
> 
> Sure.
Great, please update the patch and I'll r+ (assuming try push is green)

Thanks Evan!
Honza
Flags: needinfo?(odvarko)
Attachment #8773172 - Attachment is obsolete: true
Hi Honza,

I updated for the nit.

The dt5 task is failed in the try[1] you pushed. The error log is listed in below. The failure looks like not related with the patch. It looks more like about environment issues of Treeherder. And I pushed another try[2] to make sure the patch doesn't make any test failed.

So let's r+ the patch if the try[2] is good. :)

The error log:
```
ImportError: No module named pygtk
429 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_stylesheets_nested-iframes.js | Test timed out -

    1284358 Intermittent devtools/server/tests/browser/browser_stylesheets_nested-iframes.js | Test timed out -

467 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_timeline.js | A promise chain failed to handle a rejection: - at chrome://mochikit/content/browser-test.js:805 - TypeError: this.SimpleTest.info is not a function

1225375 Intermittent browser_timeline.js,browser_net_charts-06.js | application crashed [@ js::jit::JitFrameIterator::isFakeExitFrame()]

    1225379 Intermittent browser_net_filter-02.js,browser_perf-jit-view-01.js,browser_timeline.js | application crashed [@ js::jit::MarkJitActivations(JSRuntime*, JSTracer*)]

error while validating Perfherder data; ignoring
Traceback (most recent call last):
11:52:38 ERROR -
File "/home/worker/workspace/mozharness/mozharness/base/python.py", line 644, in _log_resource_usage
jsonschema.validate(data, schema)
11:52:38 ERROR -
File "/home/worker/workspace/build/venv/lib/python2.7/site-packages/jsonschema/validators.py", line 478, in validate
cls(schema, *args, **kwargs).validate(instance)
11:52:38 ERROR -
File "/home/worker/workspace/build/venv/lib/python2.7/site-packages/jsonschema/validators.py", line 123, in validate
raise error
11:52:38 ERROR -
ValidationError: None is not of type u'number'
11:52:38 ERROR -
Failed validating u'type' in schema[u'properties'][u'suites'][u'items'][u'properties'][u'subtests'][u'items'][u'properties'][u'value']:
{u'description': u'Summary value for subtest',
u'title': u'Subtest value',
u'type': u'number'}
11:52:38 ERROR -
On instance[u'suites'][2][u'subtests'][1][u'value']:
None
11:52:38 ERROR - 
```

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e0c25055994&selectedJob=24300275
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbcb83969751
Attachment #8773317 - Flags: review?(odvarko)
Comment on attachment 8773317 [details] [diff] [review]
bug-1282791.patch

Nice work Evan!

Honza
Attachment #8773317 - Flags: review?(odvarko) → review+
Thanks for the review, Honza.

And patch is r+, and the try is passed. Do checkin-needed.
Keywords: checkin-needed
Hi Wes,

Could you help to land the patch[1] into mozilla-central? Thanks a lot.

[1]: https://bugzilla.mozilla.org/attachment.cgi?id=8773317
Blocks: 1284838
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/393a662045b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
Blocks: 1289369
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.