1281931 - Remove server-only code from devtools/client/framework/selection.js

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

[Greg Tatum [:gregtatum]]

There is some server-only in the Selection class that should be cleaned up.

The server-only code only seems to appear in 2 places:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-commands.js#28
https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/layout/test/head.js#47

It seems that Selection should be completely cleaned out of server code, as well as some of the other features that are no longer being used, like the node and track parameters in the constructor.

Comment 1

[Tom Tromey :tromey]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #0)

> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> inspector-commands.js#28

I suspect this code can actually only be run on the server, since
it's a gcli command with runAt: "server".

> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> layout/test/head.js#47

I think it's fine for tests to do anything.

Comment 2

[Joe Walker [:jwalker]]

(In reply to Tom Tromey :tromey from comment #1)
> (In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #0)
> 
> > https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> > inspector-commands.js#28
> 
> I suspect this code can actually only be run on the server, since
> it's a gcli command with runAt: "server".

Yes - a command can be either runAt:server or runAt:client, and GCLI does the smart thing and ensures that the correct process executes the command.

Comment 3

[Tom Tromey :tromey]

I've removed a few things and fixed the gcli problem.
There are two remaining server-only methods, setNode and node.
These are both only used from tests, which isn't great, but which I
think is outside the scope of this project.

Comment 4

[Tom Tromey :tromey]

Attachment: Bug 1281931 - make framework/selection.js eslint-clean;

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

Comment 5

[Tom Tromey :tromey]

Attachment: Bug 1281931 - remove unused parameters from Selection constructor;

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

Comment 6

[Tom Tromey :tromey]

Attachment: Bug 1281931 - remove some unused Selection methods;

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

Comment 7

[Greg Tatum [:gregtatum]]

Comment on attachment 8775578 [details]
Bug 1281931 - make framework/selection.js eslint-clean;

https://reviewboard.mozilla.org/r/67744/#review64810

Comment 8

[Greg Tatum [:gregtatum]]

Comment on attachment 8775579 [details]
Bug 1281931 - remove unused parameters from Selection constructor;

https://reviewboard.mozilla.org/r/67746/#review64812

::: devtools/client/framework/selection.js:59
(Diff revision 1)
> - *    Can be null. Can be (un)set in the future via the "node" property;
> - * @param trackAttribute Tell if events should be fired when the attributes of
> - *    the node change.
> - *
>   */
> -function Selection(walker, node = null, track = {attributes: true, detached: true}) {
> +function Selection(walker) {

Thanks for cleaning this up!

Comment 9

[Greg Tatum [:gregtatum]]

Comment on attachment 8775580 [details]
Bug 1281931 - remove some unused Selection methods;

https://reviewboard.mozilla.org/r/67748/#review64814

Comment 10

[Tom Tromey :tromey]

It turns out that selection uses |this.document| itself.
I'm looking to see if a somewhat deeper cleanup is feasible.

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8775578 [details]
Bug 1281931 - make framework/selection.js eslint-clean;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67744/diff/1-2/

Comment 12

[Tom Tromey :tromey]

Comment on attachment 8775579 [details]
Bug 1281931 - remove unused parameters from Selection constructor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67746/diff/1-2/

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8775580 [details]
Bug 1281931 - remove some unused Selection methods;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67748/diff/1-2/

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8775578 [details]
Bug 1281931 - make framework/selection.js eslint-clean;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67744/diff/2-3/

Comment 15

[Tom Tromey :tromey]

Comment on attachment 8775579 [details]
Bug 1281931 - remove unused parameters from Selection constructor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67746/diff/2-3/

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8775580 [details]
Bug 1281931 - remove some unused Selection methods;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67748/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff904be8285d
make framework/selection.js eslint-clean; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/5830a35eaaea
remove unused parameters from Selection constructor; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/18decc2c35b2
remove some unused Selection methods; r=gregtatum

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff904be8285d
https://hg.mozilla.org/mozilla-central/rev/5830a35eaaea
https://hg.mozilla.org/mozilla-central/rev/18decc2c35b2