Closed Bug 1207977 Opened 9 years ago Closed 9 years ago

Test actor's assertHighlightedNode is weak and can fail unexpectedly

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 1 obsolete file)

This assertion helper is currently weak.
Today it just checks that the middle point of the content box model is a point of the expected DOM node.
This is weak because we may be highlighting a parent or a child node and the assertion would still be true.
Also, it can fail unexpectedly if the middle point happens to be on a child node of the asserted node.

Ideally we would check that the boxmodel being displayed relates to the asserted node.

This assertion happens to be failing on luciddream as the layout is a bit different.
Attached patch patch v1 (obsolete) — Splinter Review
Assert that each cardinal point of the node is within the box model
and fix the tests that has broken/weak assertions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e140464464ca
Assignee: nobody → poirot.alex
Comment on attachment 8665327 [details] [diff] [review]
patch v1

I had to pull complex algorithm to do "point is in polygon" checks.
I'm open to simplier checks if you know one.
Attachment #8665327 - Flags: review?(pbrosset)
Comment on attachment 8665327 [details] [diff] [review]
patch v1

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

Sorry Alex, I have just had too many things to do and too many patches to review in the past weeks, and still do, so I prefer forwarding this over to Matteo (who now knows as much as I do about highlighters) to avoid making you wait even longer.
Attachment #8665327 - Flags: review?(pbrosset) → review?(zer0)
Comment on attachment 8665327 [details] [diff] [review]
patch v1

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

Look good to me! Nothing really to say, I just highlighted some minor stuff, that are more about readability, so it's up to you.

::: devtools/client/shared/test/test-actor.js
@@ +664,5 @@
> +          return true;
> +        }
> +        if (newPoints[i][1] <= point[1]) {
> +          if (newPoints[i + 1][1] > point[1]) {
> +            if (r > 0) {

It seems to me we could use `&&` here, instead of nested `if`

@@ +670,5 @@
> +            }
> +          }
> +        } else {
> +          if (newPoints[i + 1][1] <= point[1]) {
> +            if (r < 0) {

Same here.

@@ +677,5 @@
> +          }
> +        }
> +      }
> +      if (wn === 0) {
> +        dump(JSON.stringify(point)+" is outside of "+JSON.stringify(polygon)+"\n");

it should be available a `dumpn` function in `test-actor`, that automatically add `\n` at the end.

@@ +687,5 @@
> +    let {visible, border} = yield this._getBoxModelStatus();
> +    let points = border.points;
> +    if (visible) {
> +      // Check that the node is within the box model
> +      let rect = yield this.getNodeRect(selector);

maybe you could directly get `{ left, top, width, height }` instead of `rect`, so that below we'll have `[left, top], points` as we have `[right, bottom], points`.
Attachment #8665327 - Flags: review?(zer0) → review+
Attached patch patch v2Splinter Review
Addressed all comments.
Attachment #8665327 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d6fc1e8c81f2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: