Closed
Bug 1458749
Opened 6 years ago
Closed 6 years ago
Remove checks for old traits in the inspector
Categories
(DevTools :: Inspector, enhancement, P3)
DevTools
Inspector
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 | ||
Updated•6 years ago
|
Assignee: nobody → gl
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
Comment 3•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
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]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
I also went ahead with removing 2 more traits: addNewRule and customHighlighters
try https://treeherder.mozilla.org/#/jobs?repo=try&revision=9825bbb51caa8f1db0fdc1cc8687094d28211720
Comment 8•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-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.
Comment 10•6 years ago
|
||
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
Comment 11•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•