Removing `RectHighlighter`

RESOLVED FIXED in Firefox 55

Status

P3
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: zer0, Assigned: zer0)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
It seems so far that nothing in our codebase is using this highlighter: https://dxr.mozilla.org/mozilla-central/search?q=recthighlighter&redirect=true

It seems it was used from the perf tool; but if there is no planning of using it anymore I would like to remove it.
(Assignee)

Comment 1

2 years ago
Greg, according to Patrick this highlighter was used to highlight a custom area in the page from the perf tool, I don't think this is happening anymore; can you confirm? Is there any plan to do so in the close future or we can safely remove it?
Flags: needinfo?(gtatum)
Priority: -- → P3
(Assignee)

Updated

2 years ago
Blocks: 1333358
I wasn't part of any conversations about its use. We have no current plans to integrate something like that, so I think it's probably fine to remove.
Flags: needinfo?(gtatum)

Comment 3

2 years ago
I'm just guessing but at one point one of the timeline markers (some layout or paint thing)
carried coordinates with it, and the idea was to highlight the relevant area when the marker
was examined.
(Assignee)

Comment 4

2 years ago
As confirmed with Greg and Tom, we can safely remove this highlighter since is not going to be used anytime soon anywhere.
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8846709 [details]
Bug 1335691 - Removed `RectHighlighter`, tests, and all reference to them;

https://reviewboard.mozilla.org/r/119708/#review121582
Attachment #8846709 - Flags: review?(gl) → review+

Comment 7

2 years ago
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b43d8eb6e6c9
Removed `RectHighlighter`, tests, and all reference to them; r=gl
(Assignee)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8846709 [details]
Bug 1335691 - Removed `RectHighlighter`, tests, and all reference to them;

https://reviewboard.mozilla.org/r/119708/#review121594
Attachment #8846709 - Flags: review?(zer0)
(Assignee)

Updated

2 years ago
Attachment #8846709 - Flags: review?(zer0)

Comment 9

2 years ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/13b05e58f420
Backed out changeset b43d8eb6e6c9 for build bustage. r=backout on a CLOSED TREE
Backed out for build bustage:

https://hg.mozilla.org/integration/autoland/rev/13b05e58f420c64c047ea14cc74c467c632f11a1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b43d8eb6e6c9f3e0a512fbde2c0a992009689f34&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=83428213&repo=autoland

[task 2017-03-13T17:23:09.065061Z] 17:23:09     INFO -  The error occurred while processing the following file or one of the files it includes:
[task 2017-03-13T17:23:09.065245Z] 17:23:09     INFO -      /home/worker/workspace/build/src/devtools/server/actors/highlighters/moz.build
[task 2017-03-13T17:23:09.065429Z] 17:23:09     INFO -  The error occurred when validating the result of the execution. The reported error is:
[task 2017-03-13T17:23:09.065629Z] 17:23:09     INFO -      File listed in FINAL_TARGET_FILES does not exist: /home/worker/workspace/build/src/devtools/server/actors/highlighters/rect.js
[task 2017-03-13T17:23:09.171575Z] 17:23:09     INFO -  *** Fix above errors and then restart with\
[task 2017-03-13T17:23:09.171800Z] 17:23:09     INFO -                 "/usr/local/bin/gmake -f client.mk build"
[task 2017-03-13T17:23:09.171984Z] 17:23:09     INFO -  client.mk:379: recipe for target 'configure' failed
[task 2017-03-13T17:23:09.172160Z] 17:23:09     INFO -  gmake: *** [configure] Error 1

Comment 11

2 years ago
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22874b29ac93
Removed `RectHighlighter`, tests, and all reference to them; r=gl
(Assignee)

Updated

2 years ago
Assignee: nobody → zer0

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22874b29ac93
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
This slipped my radar.. I was actually going to use it to highlight accessible object bounds as part of accessibility inspector that I'm working on in bug 1151468. Is there a remaining highlighter that allow topick custom rectangular areas (not related to DOM)? Would it be possible to bring this back?
Flags: needinfo?(zer0)
Flags: needinfo?(gl)
(Assignee)

Comment 14

2 years ago
(In reply to Yura Zenevich [:yzen] from comment #13)
> This slipped my radar.. I was actually going to use it to highlight
> accessible object bounds as part of accessibility inspector that I'm working
> on in bug 1151468. Is there a remaining highlighter that allow topick custom
> rectangular areas (not related to DOM)? Would it be possible to bring this
> back?

I don't think it would be a good idea to bring it back as is: since it was never used, it was already broken considered the refactoring we had in the past months; but definitely we can bring back the functionality.

May I ask what is the concrete use case? I took a quick look at bug 1151468 but I wasn't able to figure it out – also in the mockups, but maybe I missed it.
Flags: needinfo?(zer0)
Flags: needinfo?(yzenevich)
Flags: needinfo?(gl)
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #14)
> (In reply to Yura Zenevich [:yzen] from comment #13)
> > This slipped my radar.. I was actually going to use it to highlight
> > accessible object bounds as part of accessibility inspector that I'm working
> > on in bug 1151468. Is there a remaining highlighter that allow topick custom
> > rectangular areas (not related to DOM)? Would it be possible to bring this
> > back?
> 
> I don't think it would be a good idea to bring it back as is: since it was
> never used, it was already broken considered the refactoring we had in the
> past months; but definitely we can bring back the functionality.
> 
> May I ask what is the concrete use case? I took a quick look at bug 1151468
> but I wasn't able to figure it out – also in the mockups, but maybe I missed
> it.

Hi Matteo,

Yes so here are some concrete details about the concrete usecase:

Platform accessibility module builds its internal tree (similar to the DOM tree) of accessible objects. The accessible tree spans across all documents and across chrome. So to visualize it, for a browser app, the tree will have the browser native window at the top and content tabs would be in its subtree. Accessible object represents information about some element on the page, its semantics, etc, that screen readers and other assisitive technologies use.

One of such accessible properties is its bounds of the accessible (this we can use via XPCOM - https://dxr.mozilla.org/mozilla-central/source/accessible/interfaces/nsIAccessible.idl#228-232). The bounds calculation is not straightforward and in most cases does not correspond to just element bounds or box model bounds.

Essentially RectHighlighter was useful because it allowed for arbitrary rectangular shapes based on coordinates and offsets. Its downside however was that it would not work for XUL in Chrome, where we do have accessible objects.

I guess for my use case generic highlighter is not that necessary and it could be an "accessible bounds" highlighter since I am going to be adding accessible object spec/actor/front stuff.
Flags: needinfo?(yzenevich) → needinfo?(zer0)
(Assignee)

Comment 16

2 years ago
(In reply to Yura Zenevich [:yzen] from comment #15)

Hi Yura, thanks for your quick reply.

> The bounds calculation is not straightforward
> and in most cases does not correspond to just element bounds or box model
> bounds.
> 
> Essentially RectHighlighter was useful because it allowed for arbitrary
> rectangular shapes based on coordinates and offsets. Its downside however
> was that it would not work for XUL in Chrome, where we do have accessible
> objects.

> I guess for my use case generic highlighter is not that necessary and it
> could be an "accessible bounds" highlighter since I am going to be adding
> accessible object spec/actor/front stuff.

From the sounds of it, seems to me that you need a dedicated highlighter, since you're going to add dedicated specs/actor/front. Also, you might want to handle the case where you want to highlight more than one arbitrary rectangular shapes at the same time – not sure if it will be in you use case, but the `RectHighlighter` wasn't able to do so.
Flags: needinfo?(zer0)
We forgot to remove the CSS for this highlighter when it was removed.
I have filed bug 1352369 to clean this up.

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.