Closed Bug 1281931 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Framework, defect, P1)

49 Branch
defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox51 --- fixed

People

(Reporter: gregtatum, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(3 files)

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.
Whiteboard: [devtools-html]
Flags: qe-verify-
Whiteboard: [devtools-html] → [devtools-html] [triage]
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
(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.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
(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.
Depends on: 1274303
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 on attachment 8775578 [details]
Bug 1281931 - make framework/selection.js eslint-clean;

https://reviewboard.mozilla.org/r/67744/#review64810
Attachment #8775578 - Flags: review?(gtatum) → review+
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!
Attachment #8775579 - Flags: review?(gtatum) → review+
Comment on attachment 8775580 [details]
Bug 1281931 - remove some unused Selection methods;

https://reviewboard.mozilla.org/r/67748/#review64814
Attachment #8775580 - Flags: review?(gtatum) → review+
It turns out that selection uses |this.document| itself.
I'm looking to see if a somewhat deeper cleanup is feasible.
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 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 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/
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Depends on: 1291675
Pushed by ttromey@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ff904be8285d
https://hg.mozilla.org/mozilla-central/rev/5830a35eaaea
https://hg.mozilla.org/mozilla-central/rev/18decc2c35b2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.