1275887 - Remove longPropIterator/shortPropIterator in reps

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Lin Clark [:linclark]]

It seems like longPropIterator is never used. Honza, is that correct?

We could probably remove that and change shortPropIterator so it's just the entry point to propIterator, with a configurable max.

Comment 1

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

(In reply to Lin Clark [:linclark] from comment #0)
> It seems like longPropIterator is never used. Honza, is that correct?
True

> We could probably remove that and change shortPropIterator so it's just the
> entry point to propIterator, with a configurable max.
Yes, sounds good to me.

Honza

Comment 2

[Dalimil Hajek [:dalimil]]

Attachment: Bug1275887.patch - rev1

I merged the two functions into one with an optional 'max' parameter in both grip.js and object.js.

Comment 3

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

Comment on attachment 8771996 [details] [diff] [review]
Bug1275887.patch - rev1

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

Looks good, thanks!

R+, but there are couple of nits, please resolve.

Honza

::: devtools/client/shared/components/reps/grip.js
@@ +38,5 @@
>        }
>        return object.class || "Object";
>      },
>  
> +    shortPropIterator: function (object, max) {

I think we should rename this to `safePropIterator`, which corresponds to its purpose now.

::: devtools/client/shared/components/reps/object.js
@@ +35,5 @@
>        }
>        return "Object";
>      },
>  
> +    shortPropIterator: function (object, max) {

The same, please rename this to `safePropIterator`.

Comment 4

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

Also, please push to try to make sure all tests pass.

Honza

Comment 5

[Dalimil Hajek [:dalimil]]

Attachment: Bug1275887.patch - rev2

OK, it's ready for test now.

Comment 6

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

Comment on attachment 8772836 [details] [diff] [review]
Bug1275887.patch - rev2

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

LGTM!

Honza

Comment 7

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

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=509f71723167

@Dalimil, please setup level 1 access so, you can push to try.

Honza

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9728fac2aed0
Remove longPropIterator/shortPropIterator in reps. r=odvarko

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9728fac2aed0