Closed Bug 1458749 Opened Last year Closed Last year

Remove checks for old traits in the inspector

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: gl, Assigned: gl)

Details

Attachments

(1 file)

We check for a lot of old traits that has been around for years now in inspector.js. My proposal is to clean these checks up by removing them.
Assignee: nobody → gl
Comment on attachment 8972747 [details]
Bug 1458749 - Remove checks for old traits in the inspector.

https://reviewboard.mozilla.org/r/241288/#review247174

::: devtools/client/inspector/inspector.js
(Diff revision 1)
> -  get canGetCssPath() {
> -    return this._target.client.traits.getCssPath;
> -  },

Our policy for backward compat is to support all the way to the last ESR. 
It currently is 52. However the getCssPath trait was added in 53, in bug 1323700. 
So we need to keep this one.

::: devtools/client/inspector/inspector.js
(Diff revision 1)
> -  get canGetXPath() {
> -    return this._target.client.traits.getXPath;
> -  },

Same for this one. It was added in 56, so we need to continue using the trait for remote debugging of 52, 53, 54 or 55.

::: devtools/client/inspector/inspector.js
(Diff revision 1)
>        id: "node-menu-copycsspath",
>        label: INSPECTOR_L10N.getStr("inspectorCopyCSSPath.label"),
>        accesskey:
>          INSPECTOR_L10N.getStr("inspectorCopyCSSPath.accesskey"),
>        disabled: !isSelectionElement,
> -      hidden: !this.canGetCssPath,

This one needs to stay for a little while longer.

::: devtools/client/inspector/inspector.js
(Diff revision 1)
>        id: "node-menu-copyxpath",
>        label: INSPECTOR_L10N.getStr("inspectorCopyXPath.label"),
>        accesskey:
>          INSPECTOR_L10N.getStr("inspectorCopyXPath.accesskey"),
>        disabled: !isSelectionElement,
> -      hidden: !this.canGetXPath,

This one too.

::: devtools/server/actors/root.js
(Diff revision 1)
> -    // Whether the dom node actor implements the getCssPath method
> -    getCssPath: true,
> -    // Whether the dom node actor implements the getXPath method
> -    getXPath: true,

So, those need to stay too.
Attachment #8972747 - Flags: review?(pbrosset)
Comment on attachment 8972747 [details]
Bug 1458749 - Remove checks for old traits in the inspector.

https://reviewboard.mozilla.org/r/241288/#review247380


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/framework/toolbox-highlighter-utils.js:301
(Diff revision 2)
>     * @return a promise that resolves to the highlighter
>     */
>    exported.getHighlighterByType = requireInspector(async function(typeName) {
>      let highlighter = null;
>  
>      if (supportsCustomHighlighters()) {

Error: 'supportscustomhighlighters' is not defined. [eslint: no-undef]

::: devtools/client/inspector/rules/rules.js:247
(Diff revision 2)
>      if (this.selectorHighlighter) {
>        return this.selectorHighlighter;
>      }
>  
>      try {
>        let h = await utils.getHighlighterByType("SelectorHighlighter");

Error: 'utils' is not defined. [eslint: no-undef]
I also went ahead with removing 2 more traits: addNewRule and customHighlighters

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=9825bbb51caa8f1db0fdc1cc8687094d28211720
Comment on attachment 8972747 [details]
Bug 1458749 - Remove checks for old traits in the inspector.

https://reviewboard.mozilla.org/r/241288/#review247458

::: devtools/client/framework/toolbox-highlighter-utils.js:61
(Diff revision 3)
>    let isRemoteHighlightable = exported.isRemoteHighlightable = function() {
>      return target.client.traits.highlightable;
>    };

This part is really really old and should be removed too, but there are more implications in the code to doing this, so I suggest moving this to a different bug.
Attachment #8972747 - Flags: review?(pbrosset) → review+
Comment on attachment 8972747 [details]
Bug 1458749 - Remove checks for old traits in the inspector.

https://reviewboard.mozilla.org/r/241288/#review247692

::: devtools/client/framework/toolbox-highlighter-utils.js:61
(Diff revision 3)
>    let isRemoteHighlightable = exported.isRemoteHighlightable = function() {
>      return target.client.traits.highlightable;
>    };

Yes, I was trying to remove this, but the implications made me decide otherwise.
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f64dc9958646
Remove checks for old traits in the inspector. r=pbro
https://hg.mozilla.org/mozilla-central/rev/f64dc9958646
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.