Closed Bug 1272857 Opened 4 years ago Closed 4 years ago

do tree performance tests intend to set margins on node.style rather than node?

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

Details

Attachments

(1 file, 1 obsolete file)

This is a followup to bug 1111440 comment 10 (and 11 and 12).

The substitution made there brought to light that some devtools performance test code is setting marginInlineStart (previously MozMarginStart) on node rather than on node.style.  It's not clear whether this was intended or not, so I'm filing this bug to ask.
This is a followup to bug 1111440 (see above).

MozReview-Commit-ID: BHCexWmrHoT
Attachment #8752438 - Flags: feedback?(vporof)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Performance Tools (Profiler/Timeline)
Comment on attachment 8752438 [details] [diff] [review]
Are these intending to read/write element.style.marginInlineStart instead of just element.marginInlineStart?

:vporof is away from Bugzilla for a while working on Tofino.  Let's ask :gregtatum about this.
Attachment #8752438 - Flags: feedback?(vporof) → feedback?(gtatum)
It looks like the intent was to modify the style, not just set some random value. This is in a test for the AbstractTreeItem, so it doesn't break anything to change it as the test should pass either way. The more correct code would be to have the node.style.marginInlineStart set.

Also there is a comment in the `devtools/client/shared/widgets/AbstractTreeItem.jsm` file which should also probably be updated: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/AbstractTreeItem.jsm#53
Attachment #8752438 - Flags: feedback?(gtatum) → feedback+
Priority: -- → P3
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #3)
> Also there is a comment in the
> `devtools/client/shared/widgets/AbstractTreeItem.jsm` file which should also
> probably be updated:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/
> widgets/AbstractTreeItem.jsm#53

That comment is already updated within the patch.
Attachment #8752438 - Attachment is obsolete: true
Comment on attachment 8756021 [details] [diff] [review]
Fix some tree performance tests to read/write element.style.marginInlineStart instead of just element.marginInlineStart

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

LGTM, thanks!
Attachment #8756021 - Flags: review?(gtatum) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4868096fd610dc4a205632e170f6332479ac22
Bug 1272857 - Fix some tree performance tests to read/write element.style.marginInlineStart instead of just element.marginInlineStart.  r=gregtatum
https://hg.mozilla.org/mozilla-central/rev/6f4868096fd6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.