Bug 96763 – Web Inspector: CSS property names autocomplete: Suggest most used rather than alphabetically first

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Nikita Vasilyev]

f|
 fill
 fill-opacity
 fill-rule
 filter
 float
 flood-color
 flood-opacity
 font
 font-family
 font-size
 font-stretch
 font-style
 font-variant
 font-weight

The list is sorted alphabetically. It isn’t very convenient. I never use "fill" or "flood" (it’s for SVG, it doesn’t even work for HTML).

One solution would be to store a hash table with usage frequency of every property.

WebInspector.CSSCompletions.cssPropertiesUsage = {
    "-webkit-align-content": 0,
    "font": 42,
    ...remaining 382 properties...
}

We could store it in the localStorage.

[Pavel Feldman]

Let's just put them in the order we like in the CSSKeywordCompletions.js and not sort it at runtime. I.e.

1) sort them alphabetically inside the source
2) move the ones we don't like to the end

...or if you want to have more fun

use page cycler data to calculate every value's weight and use it when generating the source.

[Nikita Vasilyev]

(In reply to comment #1)
> Let's just put them in the order we like in the CSSKeywordCompletions.js and not sort it at runtime. I.e.
> 
> 1) sort them alphabetically inside the source
> 2) move the ones we don't like to the end

Not sure if get it right. Where exactly I can sort them the way I like? They are currently fetched from the backend via CSSAgent.getSupportedCSSProperties. I couldn’t trace them from there.

All properties are sorted alphabetically. _firstIndexOfPrefix uses binary search. Are use suggesting to do reordering after finding the suggested values in sorted array?

[Pavel Feldman]

Ah, I gave you a bad advice. I thought we were talking about the property values. But we could do the same for names - collect the stats and assign weight to every property name, then I would just hardcode it in the front-end and sort by weight at runtime.

[Nikita Vasilyev]

How do I collect the stats?

(In reply to comment #3)
> Ah, I gave you a bad advice. I thought we were talking about the property values. But we could do the same for names - collect the stats and assign weight to every property name, then I would just hardcode it in the front-end and sort by weight at runtime.

How do we collect stats? How does page cycler help here? We need to know most frequently used property names. Used by users in the wild. One way to get them would be to analise CSS files of several websites. Is there an easier way?

[Pavel Feldman]

> How do we collect stats? How does page cycler help here? We need to know most frequently used property names. Used by users in the wild. One way to get them would be to analise CSS files of several websites. Is there an easier way?

Page cycler iterates over a lot of popular pages (http://www.chromium.org/developers/testing/page-cyclers). If you modify WebCore CSS parser to collect stats on property names in some static variable (count them?), you can run the cycler against such build and dump the property frequencies.

[Nikita Vasilyev]

(In reply to comment #5)
> > How do we collect stats? How does page cycler help here? We need to know most frequently used property names. Used by users in the wild. One way to get them would be to analise CSS files of several websites. Is there an easier way?
> 
> Page cycler iterates over a lot of popular pages (http://www.chromium.org/developers/testing/page-cyclers).

I don’t see any mention of it running over popular pages neither on http://www.chromium.org/developers/testing/page-cyclers nor http://src.chromium.org/viewvc/chrome/trunk/src/tools/page_cycler/

> If you modify WebCore CSS parser to collect stats on property names in some static variable (count them?), you can run the cycler against such build and dump the property frequencies.

I’m not really fluent with neither C++ nor WebCore CSS parser. I’d rather go for JS and DOM (document.styleSheets).

[Nikita Vasilyev]

I made a simple script for PhantomJS (http://phantomjs.org/) to collect the stats. https://gist.github.com/3751436
Most used property is width, its weight is 255.

It isn’t terrible accurate, but it does give an idea that "font" is much frequently used than "fill".

(In reply to comment #3)
> ... then I would just hardcode it in the front-end and sort by weight at runtime.

If we hardcode them, how do we keep them up to date? If the answer is "manually" then I’d rather keep it on the backend as it now.

[Nikita Vasilyev]

Created attachment 164763 [details]
WIP

[Pavel Feldman]

I guess the list should always look the same way. It is just that the default selection should be based on the weight. I.e. when I type "f", here is what I expect to see:

f|<ont>
 fill
 fill-opacity
 fill-rule
 filter
 float
 flood-color
 flood-opacity
>font<
 font-family
 font-size
 font-stretch
 font-style
 font-variant
 font-weight

> If we hardcode them, how do we keep them up to date? If the answer is "manually" then I’d rather keep it on the backend as it now.

The reason the list of supported properties is on the backend is that it exposes the rendering engine capabilities. You ask backend: "what do you support"? And it responds.

But the "weight" of the property name is has purely IDE nature, it is not a feature of the instrumented device. Why would a mobile browser hold this information in its code? It would be as if gdb holded a list of C++ keywords.

[Nikita Vasilyev]

(In reply to comment #9)
> I guess the list should always look the same way. It is just that the default selection should be based on the weight. I.e. when I type "f", here is what I expect to see:

I agree. "font-" properties spread through the list look messy:

font
font-size
float
font-weight
font-family
font-style
fill
filter
flood-color
flood-opacity
fill-opacity
fill-rule
font-stretch
font-variant

It requires some changes in TextPrompt.js though.

> > If we hardcode them, how do we keep them up to date? If the answer is "manually" then I’d rather keep it on the backend as it now.
> 
> The reason the list of supported properties is on the backend is that it exposes the rendering engine capabilities. You ask backend: "what do you support"? And it responds.
> 
> But the "weight" of the property name is has purely IDE nature, it is not a feature of the instrumented device. Why would a mobile browser hold this information in its code? It would be as if gdb holded a list of C++ keywords.

Sounds reasonable.

[Nikita Vasilyev]

Created attachment 164996 [details]
Implement selection of non-first item in WebInspector.TextPrompt.SuggestBox

[WebKit Review Bot]

Attachment 164996 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:19:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikita Vasilyev]

Created attachment 164998 [details]
Implement selection of non-first item in WebInspector.TextPrompt.SuggestBox

[Nikita Vasilyev]

Created attachment 165000 [details]
After the patch #164998

[Alexander Pavlov (apavlov)]

Comment on attachment 164998 [details]
Implement selection of non-first item in WebInspector.TextPrompt.SuggestBox

View in context: https://bugs.webkit.org/attachment.cgi?id=164998&action=review

> Source/WebCore/ChangeLog:21
> +                replaced them with this method.

indent too big

> Source/WebCore/inspector/front-end/CSSCompletions.js:93
> +WebInspector.CSSCompletions.Weight = {

You should perhaps add a small comment saying how these weights were derived.

> Source/WebCore/inspector/front-end/CSSCompletions.js:222
> +

inadvertent change?

> Source/WebCore/inspector/front-end/TextPrompt.js:436
> +     * @param {number=0} selectedIndex

"..=0" is invalid JsDoc syntax. It should be
@param {number=} selectedIndex

> Source/WebCore/inspector/front-end/TextPrompt.js:455
> +        if (!selectedIndex)

As a shortcut, you may use
selectedIndex = selectedIndex || 0;

> Source/WebCore/inspector/front-end/TextPrompt.js:1096
> +            index = Math.max(index, 0);

In utilities.js, we have Number.constrain(), so you can write
index = Number.constrain(index, 0, this._length - 1);

> Source/WebCore/inspector/front-end/TextPrompt.js:1146
>       * @param {boolean=} canShowForSingleItem

This annotation seems to be invalid - please remove it.

> Source/WebCore/inspector/front-end/TextPrompt.js:1151
> +        var fragment = document.createDocumentFragment();

Is using DocumentFragment any faster than the direct modification of element's children in this case?

> Source/WebCore/inspector/front-end/TextPrompt.js:1176
> +        this._selectedElement = this.contentElement.childNodes[index];

Should this be .children instead? (to avoid considering TextNodes, should they somehow crawl into the children collection)

[Nikita Vasilyev]

Comment on attachment 164998 [details]
Implement selection of non-first item in WebInspector.TextPrompt.SuggestBox

View in context: https://bugs.webkit.org/attachment.cgi?id=164998&action=review

Thanks for the review, Alexander!

>> Source/WebCore/inspector/front-end/TextPrompt.js:1151
>> +        var fragment = document.createDocumentFragment();
> 
> Is using DocumentFragment any faster than the direct modification of element's children in this case?

I should guarantee a single repaint.

[Nikita Vasilyev]

Created attachment 165397 [details]
Fix nits

> >> Source/WebCore/inspector/front-end/TextPrompt.js:1151
> >> +        var fragment = document.createDocumentFragment();
> > 
> > Is using DocumentFragment any faster than the direct modification of element's children in this case?
> 
> I should guarantee a single repaint.

I did some profiling: it does a single repaint in both cases and there is no performance deviation between them (takes 1–4ms on my machine). Therefor I keep it as it was.

[Nikita Vasilyev]

> I should guarantee a single repaint.

I mean, "It should".

[Alexander Pavlov (apavlov)]

Comment on attachment 165397 [details]
Fix nits

View in context: https://bugs.webkit.org/attachment.cgi?id=165397&action=review

> Source/WebCore/inspector/front-end/CSSCompletions.js:93
> +WebInspector.CSSCompletions.Weight = {

A small comment on how the data were derived, and we are ready to land.

[Nikita Vasilyev]

Created attachment 166530 [details]
Add a comment

[WebKit Review Bot]

Comment on attachment 166530 [details]
Add a comment

Clearing flags on attachment: 166530

Committed r130140: <http://trac.webkit.org/changeset/130140>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Feldman]

Comment on attachment 166530 [details]
Add a comment

View in context: https://bugs.webkit.org/attachment.cgi?id=166530&action=review

> Source/WebCore/inspector/front-end/TextPrompt.js:1142
> +     * @param {number} selectedIndex

Optional parameters should follow the required ones. This breaks front-end compilation.

[Nikita Vasilyev]

(In reply to comment #23)
> (From update of attachment 166530 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166530&action=review
> 
> > Source/WebCore/inspector/front-end/TextPrompt.js:1142
> > +     * @param {number} selectedIndex
> 
> Optional parameters should follow the required ones. This breaks front-end compilation.

How do I compile front-end? I mean, without compiling the whole WebKit.

I’ve found ./Source/WebCore/inspector/compile-front-end.py. I downloaded the latest Closure Compiler into ~/closure/compiler.jar to make it work. compile-front-end.py shows lots of warnings including the one you mentioned but no errors. I don’t know where the compilation result is so I can’t check is it actually work or not.

Should I create a new bug and land a fix in it?

[Vsevolod Vlasov]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 166530 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=166530&action=review
> > 
> > > Source/WebCore/inspector/front-end/TextPrompt.js:1142
> > > +     * @param {number} selectedIndex
> > 
> > Optional parameters should follow the required ones. This breaks front-end compilation.
> 
> How do I compile front-end? I mean, without compiling the whole WebKit.
> 
> I’ve found ./Source/WebCore/inspector/compile-front-end.py. I downloaded the latest Closure Compiler into ~/closure/compiler.jar to make it work. compile-front-end.py shows lots of warnings including the one you mentioned but no errors. I don’t know where the compilation result is so I can’t check is it actually work or not.
> 
> Should I create a new bug and land a fix in it?

We are currently migrating to the new compiler version. Right now it could be compiled with an older version only. 
You could try this one now: http://mvnrepository.com/artifact/com.google.javascript/closure-compiler/r1810

[Nikita Vasilyev]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > (From update of attachment 166530 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=166530&action=review
> > > 
> > > > Source/WebCore/inspector/front-end/TextPrompt.js:1142
> > > > +     * @param {number} selectedIndex
> > > 
> > > Optional parameters should follow the required ones. This breaks front-end compilation.
> > 
> > How do I compile front-end? I mean, without compiling the whole WebKit.
> > 
> > I’ve found ./Source/WebCore/inspector/compile-front-end.py. I downloaded the latest Closure Compiler into ~/closure/compiler.jar to make it work. compile-front-end.py shows lots of warnings including the one you mentioned but no errors. I don’t know where the compilation result is so I can’t check is it actually work or not.
> > 
> > Should I create a new bug and land a fix in it?
> 
> We are currently migrating to the new compiler version. Right now it could be compiled with an older version only. 
> You could try this one now: http://mvnrepository.com/artifact/com.google.javascript/closure-compiler/r1810

I put http://repo1.maven.org/maven2/com/google/javascript/closure-compiler/r1810/closure-compiler-r1810.jar into ~/closure/compiler.jar. Now it doens’t even work:

WebKit ➤ ./Source/WebCore/inspector/compile-front-end.py 
Failed to load Main-Class manifest attribute from
/Users/nv/closure/compiler.jar

[Vsevolod Vlasov]

Try r1810 here http://code.google.com/p/closure-compiler/downloads/list

[Nikita Vasilyev]

It works. It doesn’t show any errors on the ToT:

➤ ./Source/WebCore/inspector/compile-front-end.py 
Module 'jsmodule_scripts' depends on unknown module 'jsmodule_workers'. Be sure to list modules in dependency order.
Compiling InjectedScriptSource.js...
0 error(s), 0 warning(s), 79.3% typed
Compiling InjectedScriptCanvasModuleSource.js...
0 error(s), 0 warning(s), 76.1% typed

It doesn’t seem to run compilation on TextPrompt.js at all.

[Vsevolod Vlasov]

> Module 'jsmodule_scripts' depends on unknown module 'jsmodule_workers'. Be sure to list modules in dependency order.
This should be fixed now.

[Pavel Feldman]

Comment on attachment 166530 [details]
Add a comment

View in context: https://bugs.webkit.org/attachment.cgi?id=166530&action=review

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2668
> +        completionsReadyCallback(results, selectedIndex);

You should also annotate that completionsReadyCallback now accepts two parameters.

[Nikita Vasilyev]

What should I do to see all these warnings/errors? This is what I just ran on ToT and didn’t see any of the warnings/errors above:

➤ ./Source/WebCore/inspector/compile-front-end.py
0 error(s), 0 warning(s), 79.3% typed
Compiling InjectedScriptSource.js...
0 error(s), 0 warning(s), 89.3% typed
Compiling InjectedScriptCanvasModuleSource.js...
Source/WebCore/inspector/InjectedScriptCanvasModuleSourceTmp.js:656: WARNING - inconsistent return type
found   : ReplayableResource
required: (Resource|null)
            return resource;
                   ^

Source/WebCore/inspector/InjectedScriptCanvasModuleSourceTmp.js:1592: WARNING - actual parameter 1 of Resource.prototype.setWrappedObject does not match formal parameter
found   : *
required: (Object|null)
        this.setWrappedObject(gl);
                              ^

Source/WebCore/inspector/InjectedScriptCanvasModuleSourceTmp.js:1900: WARNING - actual parameter 1 of Resource.prototype.setWrappedObject does not match formal parameter
found   : *
required: (Object|null)
        this.setWrappedObject(ctx);
                              ^

0 error(s), 3 warning(s), 84.2% typed

[Vsevolod Vlasov]

What you see there now is correct.
These are compiler errors that are not fixed yet.

I believe compiler errors caused by this patch were batch-fixed in http://trac.webkit.org/changeset/130154

[Nikita Vasilyev]

How do I collect the stats?

[Nikita Vasilyev]

(In reply to comment #33)
> How do I collect the stats?

Whoops, old browser session. Ignore that.