Clean up legacy Font Editor experiment [devtools/devtools-frontend : main]
Natallia Harshunova (Gerrit)
Natallia Harshunova voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alex Rudenko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer added 3 comments![]( "Open in Gerrit")
if (this.stylesContainer.swatchPopoverHelper().isShowing() ||
this.styleInternal.type === SDK.CSSStyleDeclaration.Type.Inline) {
return;
}
if (this.fontEditorToolbar) {
this.fontEditorToolbar.classList.add('font-toolbar-hidden');
}
if (this.newStyleRuleToolbar) {
this.newStyleRuleToolbar.classList.remove('shifted-toolbar');
}Is it really ok to delete all of this?
this.#parentSection.resetToolbars();In case we need to keep `resetToolbars`, we'll need to keep this in as well.
"../../../ui/components/input:bundle",Is this needed even though this CL only removes code?
Related details
- Natallia Harshunova
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Natallia Harshunova (Gerrit)
Natallia Harshunova added 1 comment![]( "Open in Gerrit")
if (this.stylesContainer.swatchPopoverHelper().isShowing() ||
this.styleInternal.type === SDK.CSSStyleDeclaration.Type.Inline) {
return;
}
if (this.fontEditorToolbar) {
this.fontEditorToolbar.classList.add('font-toolbar-hidden');
}
if (this.newStyleRuleToolbar) {
this.newStyleRuleToolbar.classList.remove('shifted-toolbar');
}Is it really ok to delete all of this?
Hi, i just saw it was added in exact CL that introduced this feature. Take a look
https://chromium-review.git.corp.google.com/c/devtools/devtools-frontend/+/2305613/70/front_end/elements/StylesSidebarPane.js#1306
Do you have any reason to consider it unsafe?
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Natallia Harshunova (Gerrit)
Natallia Harshunova added 1 comment![]( "Open in Gerrit")
"../../../ui/components/input:bundle",Natallia HarshunovaIs this needed even though this CL only removes code?
Hi.The file [front_end/panels/elements/components/StylePropertyEditor.ts](https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/elements/components/StylePropertyEditor.ts;l=10) directly imports ui/components/input/input.js, but its direct dependency was never declared in this BUILD.gn. It used to compile because it leaked through a transitive dependency chain from another module. However, cleaning up the dead code for the font-editor in inline_editor and removing its unused deps broke that hidden chain, and the compiler started complaining about missing files. Adding this direct dependency is the correct fix and follows our best practices.
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |