Closed Bug 1076866 Opened 5 years ago Closed 5 years ago

Rect highlighter

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file, 2 obsolete files)

The devtools highlighters right now usually highlight nodes in the page.

For the timeline tool, we want to highlight arbitrary rectangles to show things that got painted at some stage during the recording.
See Also: → 1050770
Assignee: nobody → pbrosset
Note that a RectsHighlighter (plural) would be more useful than a RectHighlighter. Highlighting multiple rects at once would be useful if we want to show the reflowed frame tree for instance.
This should be ready for review.

Brian: this is a new type of custom highlighter. It is intended to be used by the timeline panel, to highlighter painted rectangles when hovering over paint markers.
This patch is *only about the highlighter* though, the hovering mechanism and paint markers meta-data will come later with bug 1069661.

For now, the only pieces of code that use this new highlighter are the 2 tests  I added in this patch.
My intention is to create a new separate patch that uses this highlighter in the web console to highlight Rect objects (as returned by getBoundClientRect, getBoxQuads).
Attachment #8520023 - Attachment is obsolete: true
Attachment #8523009 - Flags: review?(bgrinstead)
Comment on attachment 8523009 [details] [diff] [review]
bug1076866-rect-highlighter v2.patch

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

I think the options validation is too strict, but looks good besides that

::: browser/devtools/inspector/test/doc_inspector_highlighter_rect_iframe.html
@@ +9,5 @@
> +    }
> +  </style>
> +</head>
> +<body>
> +  

Nit: trailing whitespace

::: toolkit/devtools/server/actors/highlighter.js
@@ +349,5 @@
> +   * method.
> +   *
> +   * Most custom highlighters are made to highlight DOM nodes, hence the first
> +   * NodeActor argument (NodeActor as in toolkit/devtools/server/actor/inspector).
> +   * Note however that some highlighter use this argument merely as a context

"some highlighters"

@@ +1503,5 @@
> +
> +    let container = doc.createElement("div");
> +    container.className = "highlighter-container";
> +
> +    let rect = doc.createElement("div");

doesn't really matter, but these 5 lines could be replaced by:

container.innerHTML = '<div id="highlighted-rect" class="highlighted-rect" hidden="true">'

@@ +1522,5 @@
> +  _hasValidOptions: function(options) {
> +    let isValidNb = n => typeof n === "number" && n >= 0 && isFinite(n);
> +    return options &&
> +           options.rect &&
> +           options.rect.x && isValidNb(options.rect.x) &&

Is it intentional that x = 0 or y = 0 will not be a valid rectangle?  I believe the check options.rect.x and options.rect.y can be removed here and the test case updated to include this case.  Same for width and height (though you may actually want to consider those invalid).
Attachment #8523009 - Flags: review?(bgrinstead)
Status: NEW → ASSIGNED
This should address all review comments.

I've kept the checks for options.rect.width and options.rect.height to avoid drawing the rectangle if they're 0.
I've added a couple of test cases for this.
I've also fixed a bug in the highlighter whereby if you called show() twice in a row, once with valid options and then with invalid options, the second call should actually hide the highlighter.
Attachment #8523009 - Attachment is obsolete: true
Attachment #8524329 - Flags: review?(bgrinstead)
Comment on attachment 8524329 [details] [diff] [review]
bug1076866-rect-highlighter v3.patch

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

Looks good
Attachment #8524329 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/888dd71a867f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl
Component: Developer Tools: Timeline → Developer Tools: Performance Tools (Profiler/Timeline)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.