Fix DoCompareTreePosition frame tree version with null aCommonAncestor.

REOPENED
Assigned to

Status

()

enhancement
P3
normal
REOPENED
a year ago
11 months ago

People

(Reporter: emilio, Assigned: emilio, NeedInfo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Right now we don't use it but it's broken. We need to use it for bug 1414303, so this is a preliminar patch for that.
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8939410 [details]
Bug 1427635: Fix DoCompareTreePosition frame tree version with null aCommonAncestor.

https://reviewboard.mozilla.org/r/209748/#review215294

::: layout/base/nsLayoutUtils.cpp:1916
(Diff revision 1)
> -  if (aCommonAncestor &&
> -      !FillAncestors(aFrame1, aCommonAncestor, &frame1Ancestors)) {
> +  if (!FillAncestors(aFrame1, aCommonAncestor, &frame1Ancestors) &&
> +      aCommonAncestor) {

Consider adding a comment here stating that we are relying on the side effect of `FillAncestors` to fill the array, so the order is important... in case anyone wonders and tries to "optimize" it in the future.
Attachment #8939410 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
mozreview-review-reply
Comment on attachment 8939410 [details]
Bug 1427635: Fix DoCompareTreePosition frame tree version with null aCommonAncestor.

https://reviewboard.mozilla.org/r/209748/#review215294

> Consider adding a comment here stating that we are relying on the side effect of `FillAncestors` to fill the array, so the order is important... in case anyone wonders and tries to "optimize" it in the future.

Agreed, done! Thanks for the review :)

Comment 5

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9a95b205f3e8
Fix DoCompareTreePosition frame tree version with null aCommonAncestor. r=xidorn

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a95b205f3e8
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 7

a year ago
Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/2b0691e2e31b
Backed out changeset 9a95b205f3e8 for cl failures at dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html r=backout a=backout
Backed out for cl failures at dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html

Treeeherder link with the push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9a95b205f3e8df9cd967c3f8a0fae52a54dac6a9&filter-resultStatus=testfailed&filter-resultStatus=runnable&selectedJob=153757245

Backout link: https://hg.mozilla.org/mozilla-central/rev/2b0691e2e31bb2d30d74de99818d20856479587d

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=153757245&repo=autoland&lineNumber=14590
Status: RESOLVED → REOPENED
Flags: needinfo?(emilio)
Resolution: FIXED → ---
Target Milestone: mozilla59 → ---

Comment 9

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9eb8652389f2
Fix DoCompareTreePosition frame tree version with null aCommonAncestor. r=xidorn
(Assignee)

Comment 10

a year ago
I tried to make a sense of it, but I couldn't. There is a single caller of this function, which is:

  https://searchfox.org/mozilla-central/rev/5bbbbe17bc262ded6d4f0e0906ed84aafb4ca0c8/layout/base/AccessibleCaretManager.cpp#1221

That's loosely related to that test, but that only means that that caller is completely busted. I pushed to try today and didn't see that failure, so I pushed again for now... Otherwise I guess disabling that test in win7 debug may be worth doing.
(Assignee)

Updated

a year ago
Flags: needinfo?(emilio)
[ Triage 2017/02/20: P3 ]
Priority: -- → P3
(Assignee)

Updated

11 months ago
No longer blocks: shadowdom-layout
You need to log in before you can comment on or make changes to this bug.