Closed Bug 1332049 Opened 3 years ago Closed 3 years ago

Add a new working copy of the Spectrum widget

Categories

(DevTools :: Inspector: Rules, defect, P3)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

We want to add a new working copy of the Spectrum widget to act as the copy of the spectrum code to do the color widget refresh. This should be pref'd on/off from about:config.
Blocks: 1277352
Attached patch 1332049.patch [1.0] (obsolete) — Splinter Review
Attachment #8828106 - Flags: review?(pbrosset)
Comment on attachment 8828106 [details] [diff] [review]
1332049.patch [1.0]

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

Looks good to me. Just one note about making it extra obvious in the code why we're doing this.

::: devtools/client/shared/widgets/ColorWidget.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Please add a comment near the top with a warning saying what this file is, what it is for, and that it's behind a pref, etc.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +34,5 @@
>   */
>  function SwatchColorPickerTooltip(document,
>                                    inspector,
>                                    {supportsCssColor4ColorFunction}) {
> +  let stylesheet = Services.prefs.getBoolPref("devtools.inspector.colorWidget.enabled") ?

Instead of accessing this thing twice, why no store it in a const at the top of the file? And add a comment to explain what this is.

// Is the new color widget enabled?
const NEW_COLOR_WIDGET = Services.prefs.getBoolPref("devtools.inspector.colorWidget.enabled");

Should make the rest of the code simpler.
Attachment #8828106 - Flags: review?(pbrosset) → review+
Attached patch 1332049.patch [2.0] (obsolete) — Splinter Review
Attachment #8828106 - Attachment is obsolete: true
Attachment #8828312 - Flags: review+
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=70344714&repo=mozilla-inbound&lineNumber=17074

seems the real error is here:
WARNING: Found 32 duplicated files taking 88103 bytes (uncompressed)

[task 2017-01-19T14:23:36.204851Z] 14:23:36     INFO -  ERROR: The following duplicated files are not allowed:

[task 2017-01-19T14:23:36.205050Z] 14:23:36     INFO -  browser/chrome/devtools/content/shared/widgets/color-widget.css

[task 2017-01-19T14:23:36.205243Z] 14:23:36     INFO -  browser/chrome/devtools/content/shared/widgets/spectrum.css
Flags: needinfo?(gl)
Attachment #8828312 - Attachment is obsolete: true
Flags: needinfo?(gl)
Attachment #8828393 - Flags: review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3338b6713e0
Add a new working copy of the Spectrum widget for the color picker refresh. r=pbro
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/118b9a68bb7e
Add a new working copy of the Spectrum widget for the color picker refresh. r=pbro
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/fce1a1dde304
Backed out changeset b3338b6713e0 for bustage on a CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/118b9a68bb7e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.