Remove longPropIterator/shortPropIterator in reps

RESOLVED FIXED in Firefox 50

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: linclark, Assigned: dalimil)

Tracking

Trunk
Firefox 50

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Assignee

Comment 2

3 years ago
Posted patch Bug1275887.patch - rev1 (obsolete) — Splinter Review
I merged the two functions into one with an optional 'max' parameter in both grip.js and object.js.
Assignee: nobody → dalimilhajek
Status: NEW → ASSIGNED
Attachment #8771996 - Flags: review?(odvarko)
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`.
Attachment #8771996 - Flags: review?(odvarko) → review+
Also, please push to try to make sure all tests pass.

Honza
Assignee

Comment 5

3 years ago
OK, it's ready for test now.
Attachment #8771996 - Attachment is obsolete: true
Attachment #8772836 - Flags: review?(odvarko)
Comment on attachment 8772836 [details] [diff] [review]
Bug1275887.patch - rev2

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

LGTM!

Honza
Attachment #8772836 - Flags: review?(odvarko) → review+
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=509f71723167

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

Honza
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 8

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/9728fac2aed0
Remove longPropIterator/shortPropIterator in reps. r=odvarko
Keywords: checkin-needed

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9728fac2aed0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.