[DevTools] Color values should be case insensitive while suggestions should be case aware

[DevTools] Color values should be case insensitive while suggestions should be case aware

This CL makes following changes:

* Color swatch is not shown for capitalized CSS property names. 
  As the CSS property names in caps are allowed and applied by the browser,
  added support for showing color swatch for the color values ignoring the case.

* Prompt Suggestions should be case-aware to the user input
  Added support for showing the suggestions in CSS value/name prompts honoring the user entered case.
  
BUG=392052
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178093

[vivekg, 2014-07-08 06:04:49]

PTAL, thank you!

[apavlov, 2014-07-08 07:20:48]

This patch has broken
inspector/elements/styles/inherited-mixed-case-properties.html. Please also make
sure that the involved code works as expected through a new test.

[vivekg, 2014-07-08 11:03:37]

Updated, PTAL.

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/elements/StylesSidebarPane.js (right):

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/elements/StylesSidebarPane.js:2310: if (format ===
WebInspector.Color.Format.Original) {
This change is done to reflect the actual entered color value. e.g. "BlAck". The
color.toString(format) converts it to "black"

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/CSSMetadata.js (right):

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/CSSMetadata.js:79: return
WebInspector.CSSMetadata._colorAwareProperties[propertyName.toLowerCase()] ===
true;
This enables the swatch in non-case sensitive way.

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/CSSMetadata.js:730: if (propertyName.toLowerCase()
in WebInspector.CSSMetadata._colorAwareProperties)
This change enables the suggestion keywords for the propertyName

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/ui/TextPrompt.js (right):

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/ui/TextPrompt.js:476: var prefixText =
this._userEnteredRange.toString();
This change takes the user input into account rather than relying on the
autocomplete one. This makes sure the entered text is preserved.

[apavlov, 2014-07-08 11:16:38]

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/CSSMetadata.js (right):

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/CSSMetadata.js:79: return
WebInspector.CSSMetadata._colorAwareProperties[propertyName.toLowerCase()] ===
true;
On 2014/07/08 11:03:37, vivekg_ wrote:
> This enables the swatch in non-case sensitive way.

Generally, we should never compare to boolean literals. Could you replace the
comparison by "!!WebInspector.CSSMetadata...."?

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/CSSMetadata.js:727: var descriptor =
WebInspector.CSSMetadata.descriptor(propertyName.toLowerCase());
This conversion should be done inside WebInspector.CSSMetadata.descriptor, since
it relies on the propertyName being lower-cased.

https://codereview.chromium.org/376803002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/sdk/CSSMetadata.js:730: if (propertyName.toLowerCase()
in WebInspector.CSSMetadata._colorAwareProperties)
On 2014/07/08 11:03:37, vivekg_ wrote:
> This change enables the suggestion keywords for the propertyName

Hmm, in fact, this does exactly the same as
WebInspector.CSSMetadata.isColorAwareProperty(). Could you replace this check by
a call to isColorAwareProperty()?

[vivekg, 2014-07-08 12:05:14]

Done with the review comments incorporation. PTAL, thanks!

[apavlov, 2014-07-08 13:04:08]

https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
File Source/devtools/front_end/elements/StylesSidebarPane.js (right):

https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
Source/devtools/front_end/elements/StylesSidebarPane.js:2310: if (format ===
WebInspector.Color.Format.Original) {
No braces around single-statement blocks in Blink, please

https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
File Source/devtools/front_end/ui/TextPrompt.js (right):

https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
Source/devtools/front_end/ui/TextPrompt.js:477: var suffixText =
completionText.substring(wordPrefixLength);
So, this will retain the case of entered prefix but force the lower case for the
suggested suffix? Can we fix the completions provider to force the UPPER-CASE
items if the prefix is upper-case (I've seen this style in a stylesheet of a
Russian mass-media site)? If the prefix case is mIxEd, we really don't care.

[vivekg, 2014-07-08 15:00:11]

On 2014/07/08 13:04:08, apavlov wrote:
>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> File Source/devtools/front_end/elements/StylesSidebarPane.js (right):
> 
>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> Source/devtools/front_end/elements/StylesSidebarPane.js:2310: if (format ===
> WebInspector.Color.Format.Original) {
> No braces around single-statement blocks in Blink, please
> 

Done.

>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> File Source/devtools/front_end/ui/TextPrompt.js (right):
> 
>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> Source/devtools/front_end/ui/TextPrompt.js:477: var suffixText =
> completionText.substring(wordPrefixLength);
> So, this will retain the case of entered prefix but force the lower case for
the
> suggested suffix? Can we fix the completions provider to force the UPPER-CASE
> items if the prefix is upper-case (I've seen this style in a stylesheet of a
> Russian mass-media site)? If the prefix case is mIxEd, we really don't care.

Done, PTAL!

[vivekg, 2014-07-09 17:36:18]

On 2014/07/08 15:00:11, vivekg_ wrote:
> On 2014/07/08 13:04:08, apavlov wrote:
> >
>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> > File Source/devtools/front_end/elements/StylesSidebarPane.js (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> > Source/devtools/front_end/elements/StylesSidebarPane.js:2310: if (format ===
> > WebInspector.Color.Format.Original) {
> > No braces around single-statement blocks in Blink, please
> > 
> 
> Done.
> 
> >
>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> > File Source/devtools/front_end/ui/TextPrompt.js (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/376803002/diff/40001/Source/devtools/f...
> > Source/devtools/front_end/ui/TextPrompt.js:477: var suffixText =
> > completionText.substring(wordPrefixLength);
> > So, this will retain the case of entered prefix but force the lower case for
> the
> > suggested suffix? Can we fix the completions provider to force the
UPPER-CASE
> > items if the prefix is upper-case (I've seen this style in a stylesheet of a
> > Russian mass-media site)? If the prefix case is mIxEd, we really don't care.
> 
> Done, PTAL!

Ping

[apavlov, 2014-07-10 12:53:53]

looks good. Let's test that (a) mixed-case color-aware properties are actually
considered color-aware, (b) all-caps prefix results in all-caps completions.

[vivekg, 2014-07-14 11:03:26]

On 2014/07/10 12:53:53, apavlov wrote:
> looks good. Let's test that (a) mixed-case color-aware properties are actually
> considered color-aware, (b) all-caps prefix results in all-caps completions.

Done. PTAL, thank you!

[apavlov, 2014-07-14 11:25:54]

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
File LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html
(right):

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html:31: var
proxyElement = document.createElement("span");
... = document.createChild("span");

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html:32:
document.body.appendChild(proxyElement);
this is not required given the change above

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html:40:
function isUpperCase(str) {
brace on the next line

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html:44:
function isLowerCase(str) {
ditto

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html:58: case
Case.Upper:
no "case" indentation

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
File
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html
(right):

https://codereview.chromium.org/376803002/diff/100001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html:13:
function randomizeStringCase(str)
All tests must be deterministic, so this is not an option here. Please randomize
the case once, manually. You also don't have to check all properties, since (a)
their list is always in flux, (b) they are all checked in the same way. Please
rewrite the test and check a few most common multi-cased properties.

[vivekg, 2014-07-14 11:42:52]

Done, thank you!

[apavlov, 2014-07-14 12:23:42]

lgtm with nits

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
File LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html
(right):

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html:26: },
we try to avoid dangling commas, unlike Python/GYP

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/case-sensitive-suggestions.html:79: Tests
that text prompt suggestions are case sensitive, either UPPERCASE or lowercase,
to user input.
Tests that text prompt suggestions' casing follows that of the user input

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
File
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html
(right):

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html:11:
if (WebInspector.CSSMetadata.isColorAwareProperty(colorAwareProperties[i]))
braces presence must be consistent across if-arms (then-else), see
https://sites.google.com/a/chromium.org/dev/blink/coding-style
[braces-must-match]

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html:11:
if (WebInspector.CSSMetadata.isColorAwareProperty(colorAwareProperties[i]))
We should rely on test expectations rather than sophisticated test logic. You
should always go the simplest route, like this one:

for (var i = 0; i < colorAwareProperties.length; ++i) {
    var isColorAware =
WebInspector.CSSMetadata.isColorAwareProperty(colorAwareProperties[i]);
    InspectorTest.addResult(colorAwareProperties[i] + (isColorAware ? " is" : "
is NOT") + " color aware");
}

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html:26:
Tests that color related mixed-cased CSS properties are really color aware.
color related -> color-related
mixed-cased -> mix-cased
really -> actually
(for better readability)

[vivekg, 2014-07-14 17:57:51]

Thank you @apavlov for your time. I have incorporated all the review comments.

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
File
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html
(right):

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html:11:
if (WebInspector.CSSMetadata.isColorAwareProperty(colorAwareProperties[i]))
On 2014/07/14 12:23:42, apavlov wrote:
> braces presence must be consistent across if-arms (then-else), see
> https://sites.google.com/a/chromium.org/dev/blink/coding-style
> [braces-must-match]

Done.

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html:11:
if (WebInspector.CSSMetadata.isColorAwareProperty(colorAwareProperties[i]))
On 2014/07/14 12:23:42, apavlov wrote:
> We should rely on test expectations rather than sophisticated test logic. You
> should always go the simplest route, like this one:
> 
> for (var i = 0; i < colorAwareProperties.length; ++i) {
>     var isColorAware =
> WebInspector.CSSMetadata.isColorAwareProperty(colorAwareProperties[i]);
>     InspectorTest.addResult(colorAwareProperties[i] + (isColorAware ? " is" :
"
> is NOT") + " color aware");
> }

Done.

https://codereview.chromium.org/376803002/diff/120001/LayoutTests/inspector/e...
LayoutTests/inspector/elements/styles/mixed-case-color-aware-properties.html:26:
Tests that color related mixed-cased CSS properties are really color aware.
On 2014/07/14 12:23:42, apavlov wrote:
> color related -> color-related
> mixed-cased -> mix-cased
> really -> actually
> (for better readability)

Done.

[vivekg, 2014-07-14 17:57:58]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2014-07-14 17:58:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/376803002/140001

[commit-bot: I haz the power, 2014-07-14 19:00:40]

Change committed as 178093