Removing `RectHighlighter`

RESOLVED FIXED in Firefox 55

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: zer0, Assigned: zer0)

Tracking

(Blocks 1 bug)

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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.
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
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)
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.
As confirmed with Greg and Tom, we can safely remove this highlighter since is not going to be used anytime soon anywhere.
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+
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b43d8eb6e6c9
Removed `RectHighlighter`, tests, and all reference to them; r=gl
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)
Attachment #8846709 - Flags: review?(zer0)
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
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: nobody → zer0
https://hg.mozilla.org/mozilla-central/rev/22874b29ac93
Status: NEW → RESOLVED
Closed: 2 years ago
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)
(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)
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.