Closed
Bug 1076866
Opened 10 years ago
Closed 10 years ago
Rect highlighter
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: pbro, Assigned: pbro)
References
Details
Attachments
(1 file, 2 obsolete files)
17.92 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pbrosset
Assignee | ||
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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)
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
I forgot to link to the try build : https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=316d0e5090b9
Assignee | ||
Comment 8•10 years ago
|
||
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/888dd71a867f
https://hg.mozilla.org/mozilla-central/rev/888dd71a867f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Comment 10•9 years ago
|
||
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)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•