DevTools: Reduce color parsing by passing in Color to ColorSwatch

DevTools: Reduce color parsing by passing in Color to ColorSwatch

Often times we are doing double work parsing color text to handle invalid
colors and then parsing color text again in setColorText. We should just
parse once and pass in the Color object to ColorSwatch.

BUG=642782
Committed: https://crrev.com/3254b5dba1a5071e33db5955e131ec34ed65b864
Cr-Commit-Position: refs/heads/master@{#417488}

[flandy, 2016-09-02 21:00:58]

flandy@google.com changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[flandy, 2016-09-02 21:00:58]

Please take a look.

[dgozman, 2016-09-02 22:08:01]

I don't like that all the clients now have to handle whether the color has
parsed successfully or not.

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js
(right):

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js:102:
var themeColor = WebInspector.Color.parse(stringProperty("theme_color")) ||
WebInspector.Color.parse("white");
This is a semantic change. Leave it as parse(stringPorperty(...) || "white").

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js:110:
var backgroundColor =
WebInspector.Color.parse(stringProperty("background_color")) ||
WebInspector.Color.parse("white");
ditto

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:204: var
color = WebInspector.Color.parse(colorString)
style: missing semicolon

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (left):

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:101:
this.setColorText("white");
Why removed?

[flandy, 2016-09-02 23:09:18]

On 2016/09/02 22:08:01, dgozman wrote:
> I don't like that all the clients now have to handle whether the color has
> parsed successfully or not.

Most of the clients were already handling this anyways, and if they weren't, it
seems like a mistake because the handling in setColorText is just:
console.assert(this._color, "Color text could not be parsed.");

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js
(right):

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/resources/AppManifestView.js:102:
var themeColor = WebInspector.Color.parse(stringProperty("theme_color")) ||
WebInspector.Color.parse("white");
On 2016/09/02 22:08:01, dgozman wrote:
> This is a semantic change. Leave it as parse(stringPorperty(...) || "white").

We need the extra || WebInspector.Color.parse("white") because if
stringProperty(...) is a valid string but not a valid color, there is no
guarantee that a Color was returned instead of null.

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:204: var
color = WebInspector.Color.parse(colorString)
On 2016/09/02 22:08:01, dgozman wrote:
> style: missing semicolon

Sorry, done.

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (left):

https://codereview.chromium.org/2310633002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:101:
this.setColorText("white");
On 2016/09/02 22:08:01, dgozman wrote:
> Why removed?

All of the clients set the color on their own after creating the color swatch,
so there is no need for this default value. Without this, it defaults to
url(Images/checker.png) anyways, which seems good enough. 

We could pass in a color as an argument to WebInspector.ColorSwatch.create so
clients know they should provide a color?

[lushnikov, 2016-09-06 17:00:58]

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (left):

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:101:
this.setColorText("white");
what does it render if there's no color set?

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

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:40: var
colorString = this._color.asString(this._format);
I have a memory of color.format() being a little sketchy;
please, make sure this preserves format as original where necessary

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:221: var color =
WebInspector.Color.parse(result.value);
let's get color from model

[flandy, 2016-09-08 16:51:49]

Please take another look.

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (left):

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:101:
this.setColorText("white");
On 2016/09/06 17:00:58, lushnikov wrote:
> what does it render if there's no color set? 

It looks like an empty swatch with url(Images/checker.png).
See the bug for a screenshot.

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

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:40: var
colorString = this._color.asString(this._format);
On 2016/09/06 17:00:58, lushnikov wrote:
> I have a memory of color.format() being a little sketchy;
> please, make sure this preserves format as original where necessary

I don't see any issues.

https://codereview.chromium.org/2310633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:221: var color =
WebInspector.Color.parse(result.value);
On 2016/09/06 17:00:58, lushnikov wrote:
> let's get color from model 

Done.

[lushnikov, 2016-09-09 00:16:42]

lgtm, i like the patch

let's make sure @dgozman doesn't have anything against it

https://codereview.chromium.org/2310633002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
(right):

https://codereview.chromium.org/2310633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:244: var
colorString = /** @type {string} */ (event.data);
spectrm should've fired a Color objects here, but that should be a separate
patch...

[dgozman, 2016-09-09 00:21:15]

lgtm

[flandy, 2016-09-09 00:23:47]

The CQ bit was checked by flandy@google.com

[commit-bot: I haz the power, 2016-09-09 00:24:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 00:34:38]

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

[commit-bot: I haz the power, 2016-09-09 00:34:39]

Exceeded global retry quota

[flandy, 2016-09-09 00:42:58]

The CQ bit was checked by flandy@google.com

[commit-bot: I haz the power, 2016-09-09 00:43:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 00:49:11]

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

[commit-bot: I haz the power, 2016-09-09 00:49:12]

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

[flandy, 2016-09-09 02:09:54]

The CQ bit was checked by flandy@google.com

[commit-bot: I haz the power, 2016-09-09 02:10:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 03:00:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-09 03:02:05]

Description was changed from

==========
DevTools: Reduce color parsing by passing in Color to ColorSwatch

Often times we are doing double work parsing color text to handle invalid
colors and then parsing color text again in setColorText. We should just
parse once and pass in the Color object to ColorSwatch.

BUG=642782
==========

to

==========
DevTools: Reduce color parsing by passing in Color to ColorSwatch

Often times we are doing double work parsing color text to handle invalid
colors and then parsing color text again in setColorText. We should just
parse once and pass in the Color object to ColorSwatch.

BUG=642782

Committed: https://crrev.com/3254b5dba1a5071e33db5955e131ec34ed65b864
Cr-Commit-Position: refs/heads/master@{#417488}
==========

[commit-bot: I haz the power, 2016-09-09 03:02:06]

Patchset 3 (id:??) landed as
https://crrev.com/3254b5dba1a5071e33db5955e131ec34ed65b864
Cr-Commit-Position: refs/heads/master@{#417488}