Closed Bug 1275887 Opened 4 years ago Closed 4 years ago

Remove longPropIterator/shortPropIterator in reps

Categories

(DevTools :: Shared Components, defect)

defect
Not set
normal

Tracking

(firefox49 affected, firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: linclark, Assigned: dalimil)

Details

Attachments

(1 file, 1 obsolete file)

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.
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)
Attached 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
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
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/9728fac2aed0
Remove longPropIterator/shortPropIterator in reps. r=odvarko
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9728fac2aed0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.