1335691 - Removing `RectHighlighter`

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

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?

Comment 2

[Greg Tatum [:gregtatum]]

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.

Comment 3

[Tom Tromey :tromey]

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.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

As confirmed with Greg and Tom, we can safely remove this highlighter since is not going to be used anytime soon anywhere.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1335691 - Removed `RectHighlighter`, tests, and all reference to them;

Review commit: https://reviewboard.mozilla.org/r/119708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119708/

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

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

https://reviewboard.mozilla.org/r/119708/#review121582

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b43d8eb6e6c9
Removed `RectHighlighter`, tests, and all reference to them; r=gl

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

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

https://reviewboard.mozilla.org/r/119708/#review121594

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13b05e58f420
Backed out changeset b43d8eb6e6c9 for build bustage. r=backout on a CLOSED TREE

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22874b29ac93
Removed `RectHighlighter`, tests, and all reference to them; r=gl

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22874b29ac93

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

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?

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

(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.

Comment 16

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 17

[Patrick Brosset <:pbro>]

We forgot to remove the CSS for this highlighter when it was removed.
I have filed bug 1352369 to clean this up.