Bugzilla

Updated

•

8 years ago

Blocks: devtools-html-2

Updated

•

8 years ago

Flags: qe-verify-

Priority: -- → P3

Whiteboard: [devtools-html] [triage] → [reserve-html]

Comment 1

•

8 years ago

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

Comment 2

•

8 years ago

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

Updated

•

8 years ago

Assignee: nobody → evan

Comment 3

•

8 years ago

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

Updated

•

8 years ago

Status: NEW → ASSIGNED

Assignee

Comment 4

•

8 years ago

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

Assignee

Comment 5

•

8 years ago

(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

Assignee

Updated

•

8 years ago

Flags: needinfo?(odvarko)

Comment 6

•

8 years ago

(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)

Assignee

Comment 7

•

8 years ago

(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.

Assignee

Comment 8

•

8 years ago

I think I can test my patch with DOM panel. Then I'll continue to fix the problem.

Assignee

Comment 9

•

8 years ago

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.

Assignee

Comment 10

•

8 years ago

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

Assignee

Comment 11

•

8 years ago

(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

Comment 12

•

8 years ago

(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

Comment 13

•

8 years ago

(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

Comment 15

•

8 years ago

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

Assignee

Comment 16

•

8 years ago

Attached patch bug-1282791.patch (obsolete) — Details — 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)

Assignee

Updated

•

8 years ago

Attachment #8772425 - Attachment is obsolete: true

Attachment #8772425 - Flags: review?(odvarko)

Assignee

Comment 17

•

8 years ago

Attached patch bug-1282791.patch (obsolete) — Details — Splinter Review

Hi Honza,

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

Thanks.

Attachment #8772798 - Flags: review?(odvarko)

Updated

•

8 years ago

Priority: P3 → P2

Comment 18

•

8 years ago

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)

Assignee

Updated

•

8 years ago

Attachment #8772798 - Attachment is obsolete: true

Assignee

Comment 19

•

8 years ago

Attached patch bug-1282791.patch (obsolete) — Details — 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 20

•

8 years ago

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 21

•

8 years ago

Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e0c25055994

Honza

Assignee

Comment 22

•

8 years ago

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.

Assignee

Updated

•

8 years ago

Flags: needinfo?(odvarko)

Comment 23

•

8 years ago

(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)

Assignee

Updated

•

8 years ago

Attachment #8773172 - Attachment is obsolete: true

Assignee

Comment 24

•

8 years ago

Attached patch bug-1282791.patch — Details — Splinter Review

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)

Assignee

Comment 25

•

8 years ago

Hi Honza,

The try[1] is passed.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbcb83969751

Updated

•

8 years ago

Blocks: 1282465

Comment 26

•

8 years ago

Comment on attachment 8773317 [details] [diff] [review]
bug-1282791.patch

Nice work Evan!

Honza

Attachment #8773317 - Flags: review?(odvarko) → review+

Assignee

Comment 27

•

8 years ago

Thanks for the review, Honza.

And patch is r+, and the try is passed. Do checkin-needed.

Keywords: checkin-needed

Pulsebot

Comment 28

•

8 years ago

Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/393a662045b9
Order the props, r=Honza

Keywords: checkin-needed

Assignee

Comment 29

•

8 years ago

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

Assignee

Updated

•

8 years ago

Blocks: 1284838

Assignee

Updated

•

8 years ago

Flags: needinfo?(wkocher)

Carsten Book [:Tomcat]

Comment 30

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/393a662045b9

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox50: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 50

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Updated

•

8 years ago

Iteration: --- → 50.4 - Aug 1

Priority: P2 → P1

Updated

•

8 years ago

Flags: needinfo?(wkocher)