Move swatch based tooltip editors into their individual file from Tooltip.js

RESOLVED FIXED in Firefox 52

Status

P3
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 52

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Tooltip.js is a pretty large file right now and can benefit from moving some of the Swatch classes into their own individual file.
(Assignee)

Updated

2 years ago
Blocks: 1258390
(Assignee)

Updated

2 years ago
Summary: Make Tooltip.js more modular → Move swatch based tooltip editors into their individual file from Tooltip.js
(Assignee)

Comment 2

2 years ago
Created attachment 8797260 [details] [diff] [review]
1307193.patch [1.0]

Julian, the purpose of this bug is to just move the swatch based editors modules out of Tooltip.js into their own separate files so that it is more manageable.
Attachment #8797260 - Flags: review?(jdescottes)
(Assignee)

Comment 3

2 years ago
Created attachment 8797265 [details] [diff] [review]
1307193.patch [2.0]
Attachment #8797260 - Attachment is obsolete: true
Attachment #8797260 - Flags: review?(jdescottes)
Attachment #8797265 - Flags: review?(jdescottes)
Comment on attachment 8797265 [details] [diff] [review]
1307193.patch [2.0]

Review of attachment 8797265 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thank you for doing this!
It was sad having these html tooltips living in the same file as the (unrelated) XUL tooltip.

::: devtools/client/shared/widgets/SwatchBasedEditorTooltip.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

To preserve history, you could create this file using 

hg copy devtools/client/shared/widgets/Tooltip.js devtools/client/shared/widgets/tooltip/SwatchBasedEditorTooltip.js

And put it in the tooltip/ subfolder?

::: devtools/client/shared/widgets/SwatchColorPickerTooltip.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

To preserve history, you could create this file using 

hg copy devtools/client/shared/widgets/Tooltip.js devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js

And put it in the tooltip/ subfolder?

::: devtools/client/shared/widgets/SwatchCubicBezierTooltip.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

To preserve history, you could create this file using 

hg copy devtools/client/shared/widgets/Tooltip.js devtools/client/shared/widgets/tooltip/SwatchCubicBezierTooltip.js

And put it in the tooltip/ subfolder?

::: devtools/client/shared/widgets/SwatchFilterTooltip.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

To preserve history, you could create this file using 

hg copy devtools/client/shared/widgets/Tooltip.js devtools/client/shared/widgets/tooltip/SwatchFilterTooltip.js

And put it in the tooltip/ subfolder?
Attachment #8797265 - Flags: review?(jdescottes) → review+
I was looking at the remaining call sites for the XUL tooltip and we just have:
- 2 text tooltips in the debugger
- variable view tooltip in the debugger
- text tooltip in the shader editor

Migrating the variable view tooltip to the HTML tooltip seems like a big task and I'd prefer to let debugger html decide how to port this feature.

But we could migrate the shadereditor tooltip to HTML tooltip and then move the XUL tooltip to the debugger module. This way it would no longer pollute the widgets folder and would be removed automatically when the old debugger gets removed.

What do you think?
Flags: needinfo?(gl)
(Assignee)

Comment 7

2 years ago
(In reply to Julian Descottes [:jdescottes] from comment #6)
> I was looking at the remaining call sites for the XUL tooltip and we just
> have:
> - 2 text tooltips in the debugger
> - variable view tooltip in the debugger
> - text tooltip in the shader editor
> 
> Migrating the variable view tooltip to the HTML tooltip seems like a big
> task and I'd prefer to let debugger html decide how to port this feature.
> 
> But we could migrate the shadereditor tooltip to HTML tooltip and then move
> the XUL tooltip to the debugger module. This way it would no longer pollute
> the widgets folder and would be removed automatically when the old debugger
> gets removed.
> 
> What do you think?

I think this is a good idea. I would like to add a new to bug to take care of this remaining work.
Flags: needinfo?(gl)
(Assignee)

Comment 8

2 years ago
Created attachment 8797623 [details] [diff] [review]
1307193.patch [3.0]
Attachment #8797265 - Attachment is obsolete: true
Attachment #8797623 - Flags: review+
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/47615f092e09d8d37d883ca72e35891b242a5d4a
Bug 1307193 - Move swatch based tooltip editors into their individual file from Tooltip.js r=jdescottes

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47615f092e09
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.