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

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Are these intending to read/write element.style.marginInlineStart instead of just element.marginInlineStart?

This is a followup to bug 1111440 (see above).

MozReview-Commit-ID: BHCexWmrHoT

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 3

[Greg Tatum [:gregtatum]]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Fix some tree performance tests to read/write element.style.marginInlineStart instead of just element.marginInlineStart

This is a followup to bug 1111440.

MozReview-Commit-ID: BHCexWmrHoT

Comment 6

[Greg Tatum [:gregtatum]]

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!

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a83a85ac378b&group_state=expanded

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f4868096fd6