Devtools Color: Basic support for #RRGGBBAA and #RGBA

Devtools Color: Basic support for #RRGGBBAA and #RGBA

BUG=611687
Committed: https://crrev.com/97c7757e7ae143f63e5d7281d1a9837338b12562
Cr-Commit-Position: refs/heads/master@{#395801}

[samli, 2016-05-17 07:46:08]

Description was changed from

==========
Devtools Color: Basic support for #RRGGBBAA

BUG=611687
==========

to

==========
Devtools Color: Basic support for #RRGGBBAA and #RGBA

BUG=611687
==========

[samli, 2016-05-17 07:46:08]

samli@chromium.org changed reviewers:
+ lushnikov@chromium.org

[samli, 2016-05-17 07:46:45]

PTAL. Ideally we should land this before branch (2 days) since support has
shipped in M52.

[Noel Gordon, 2016-05-19 06:46:57]

The CQ bit was checked by noel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 06:47:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986053004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986053004/20001

[Noel Gordon, 2016-05-19 07:08:25]

Thanks for the test updates, they LGTM (though, I'm not familiar with the other
parts of the Inspector that might also need to change, but they could be the
subject of new bugs I suppose?).

[dgozman, 2016-05-19 07:10:59]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2016-05-19 07:11:00]

Thanks for the patch, Sam!

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Color.js (right):

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:65: AlphaHEX:
"alphahex",
I'd go for HEXA and ShortHEXA (which is missing here anyway).

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:100: var a = format
=== WebInspector.Color.Format.AlphaHEX ? parseInt(hex.substring(6,8), 16) / 255
: 1;
style: missing space after comma

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:267: canBeShortHex:
function()
I think this function is wrong now. We can replace it with detectHexFormat()
which returns HEX, ShortHEX, HEXA or ShortHEXA and check/upadte all usages of
|hasAlpha| and |canBeShortHex|.

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:336: return
String.sprintf("#%s%s%s%s", toHexValue(this._rgba[0]),
toHexValue(this._rgba[1]), toHexValue(this._rgba[2]),
toHexValue(this._rgba[3])).toLowerCase();;
style: double semicolon

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js (right):

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js:812: else if
(this._colorFormat === cf.HSL)
This change seems to be wrong. We cannot represent color with alpha in hex. Or
is it spectrum-only format?

[pfeldman, 2016-05-19 07:14:05]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-05-19 07:14:07]

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Color.js (right):

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:64: ShortHEX:
"shorthex",
Is there ShortHEXA?

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:65: AlphaHEX:
"alphahex",
HEXA

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:80: var simple =
/^(?:#([0-9a-f]{3,4}|[0-9a-f]{6}|[0-9a-f]{8})|rgb\(((?:-?\d+%?,){2}-?\d+%?)\)|(\w+)|hsl\((-?\d+\.?\d*(?:,-?\d+\.?\d*%){2})\))$/i;
why different treatment of 3-4 ws 6-8?

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js (right):

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js:169:
colorFormat = WebInspector.Color.Format.RGBA;
Should this retain RGB?

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js:592: else if
(colorFormat === WebInspector.Color.Format.AlphaHEX)
This also seems to go the other way around.

[commit-bot: I haz the power, 2016-05-19 08:07:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-19 08:07:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[samli, 2016-05-20 00:22:19]

dgozman@ ptal

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Color.js (right):

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:64: ShortHEX:
"shorthex",
On 2016/05/19 at 07:14:06, pfeldman wrote:
> Is there ShortHEXA?

Done.

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:65: AlphaHEX:
"alphahex",
On 2016/05/19 at 07:14:06, pfeldman wrote:
> HEXA

Done.

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:80: var simple =
/^(?:#([0-9a-f]{3,4}|[0-9a-f]{6}|[0-9a-f]{8})|rgb\(((?:-?\d+%?,){2}-?\d+%?)\)|(\w+)|hsl\((-?\d+\.?\d*(?:,-?\d+\.?\d*%){2})\))$/i;
On 2016/05/19 at 07:14:06, pfeldman wrote:
> why different treatment of 3-4 ws 6-8?

"6,8" includes 7
"3,4" is just 3 & 4

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:100: var a = format
=== WebInspector.Color.Format.AlphaHEX ? parseInt(hex.substring(6,8), 16) / 255
: 1;
On 2016/05/19 at 07:10:59, dgozman wrote:
> style: missing space after comma

Done.

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:267: canBeShortHex:
function()
On 2016/05/19 at 07:10:59, dgozman wrote:
> I think this function is wrong now. We can replace it with detectHexFormat()
which returns HEX, ShortHEX, HEXA or ShortHEXA and check/upadte all usages of
|hasAlpha| and |canBeShortHex|.

Done. Not all |hasAlpha| changed since asString() should convert if possible.

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:336: return
String.sprintf("#%s%s%s%s", toHexValue(this._rgba[0]),
toHexValue(this._rgba[1]), toHexValue(this._rgba[2]),
toHexValue(this._rgba[3])).toLowerCase();;
On 2016/05/19 at 07:10:59, dgozman wrote:
> style: double semicolon

Done.

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js (right):

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js:169:
colorFormat = WebInspector.Color.Format.RGBA;
On 2016/05/19 at 07:14:06, pfeldman wrote:
> Should this retain RGB?

Yes for mildly better readability, doesn't actually change anything.

https://codereview.chromium.org/1986053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js:812: else if
(this._colorFormat === cf.HSL)
On 2016/05/19 at 07:10:59, dgozman wrote:
> This change seems to be wrong. We cannot represent color with alpha in hex. Or
is it spectrum-only format?

Spectrum-only format, bit confusing I know sorry :(.

[samli, 2016-05-20 00:22:20]

dgozman@ ptal

[samli, 2016-05-20 00:44:00]

Patchset #4 (id:60001) has been deleted

[dgozman, 2016-05-20 00:55:16]

lgtm with comments

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Color.js (right):

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:346: if
(this.detectHEXFormat() !== WebInspector.Color.Format.ShortHEXA &&
this.detectHEXFormat() !== WebInspector.Color.Format.ShortHEX)
Let's call detectHEXFormat once.

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js (right):

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js:648: return
/** @type {string} */(color.asString(color.hasAlpha() ? cf.HEXA : cf.HEX));
If I'm not missing something, this could return null, and we should check it.

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right):

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:138: if
(!color.hasAlpha())
I think this whole if should be just detectHEXFormat().

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:150: if
(!color.hasAlpha())
Ditto.

[samli, 2016-05-20 01:58:03]

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Color.js (right):

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Color.js:346: if
(this.detectHEXFormat() !== WebInspector.Color.Format.ShortHEXA &&
this.detectHEXFormat() !== WebInspector.Color.Format.ShortHEX)
On 2016/05/20 at 00:55:15, dgozman wrote:
> Let's call detectHEXFormat once.

Done

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js (right):

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/Spectrum.js:648: return
/** @type {string} */(color.asString(color.hasAlpha() ? cf.HEXA : cf.HEX));
On 2016/05/20 at 00:55:15, dgozman wrote:
> If I'm not missing something, this could return null, and we should check it.

Done.

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right):

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:138: if
(!color.hasAlpha())
On 2016/05/20 at 00:55:15, dgozman wrote:
> I think this whole if should be just detectHEXFormat().

Done.

https://codereview.chromium.org/1986053004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:150: if
(!color.hasAlpha())
On 2016/05/20 at 00:55:15, dgozman wrote:
> Ditto.

Done.

[samli, 2016-05-22 23:09:15]

The CQ bit was checked by samli@chromium.org

[samli, 2016-05-22 23:09:16]

The patchset sent to the CQ was uploaded after l-g-t-m from noel@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1986053004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-05-22 23:09:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986053004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986053004/80001

[commit-bot: I haz the power, 2016-05-23 00:22:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 00:22:46]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[samli, 2016-05-23 03:01:12]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2016-05-23 03:01:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986053004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986053004/80001

[commit-bot: I haz the power, 2016-05-23 05:30:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 05:30:14]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[samli, 2016-05-23 05:35:14]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2016-05-23 05:35:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986053004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986053004/80001

[commit-bot: I haz the power, 2016-05-23 07:59:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 07:59:46]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Noel Gordon, 2016-05-23 22:15:50]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2016-05-23 22:16:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986053004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986053004/80001

[commit-bot: I haz the power, 2016-05-23 22:24:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 22:24:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[samli, 2016-05-24 02:34:17]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2016-05-24 02:34:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986053004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986053004/80001

[commit-bot: I haz the power, 2016-05-24 08:36:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 08:36:12]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[samli, 2016-05-25 00:44:37]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2016-05-25 00:45:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986053004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986053004/80001

[commit-bot: I haz the power, 2016-05-25 04:25:54]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-25 04:27:45]

Description was changed from

==========
Devtools Color: Basic support for #RRGGBBAA and #RGBA

BUG=611687
==========

to

==========
Devtools Color: Basic support for #RRGGBBAA and #RGBA

BUG=611687

Committed: https://crrev.com/97c7757e7ae143f63e5d7281d1a9837338b12562
Cr-Commit-Position: refs/heads/master@{#395801}
==========

[commit-bot: I haz the power, 2016-05-25 04:27:47]

Patchset 4 (id:??) landed as
https://crrev.com/97c7757e7ae143f63e5d7281d1a9837338b12562
Cr-Commit-Position: refs/heads/master@{#395801}

[luoe, 2016-07-26 23:06:36]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2186773002/ by luoe@chromium.org.

The reason for reverting is: [DevTools] Entering a HEX value into color picker
and changing opacity results in an 8-digit HEX, currently seen as invalid.

This should reland once 8-digit hex (#rrggbbaa) is a fully supported value for
colors (once #76362 lands)..