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.
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.
(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?
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 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?
> 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.
(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).
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.
Created attachment 164763 [details] WIP
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.
(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.
Created attachment 164996 [details] Implement selection of non-first item in WebInspector.TextPrompt.SuggestBox
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.
Created attachment 164998 [details] Implement selection of non-first item in WebInspector.TextPrompt.SuggestBox
Created attachment 165000 [details] After the patch #164998
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)
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.
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.
> I should guarantee a single repaint. I mean, "It should".
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.
Created attachment 166530 [details] Add a comment
Comment on attachment 166530 [details] Add a comment Clearing flags on attachment: 166530 Committed r130140: <http://trac.webkit.org/changeset/130140>
All reviewed patches have been landed. Closing bug.
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.
(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?
(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
(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
Try r1810 here http://code.google.com/p/closure-compiler/downloads/list
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.
> Module 'jsmodule_scripts' depends on unknown module 'jsmodule_workers'. Be sure to list modules in dependency order. This should be fixed now.
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.
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
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
How do I collect the stats?
(In reply to comment #33) > How do I collect the stats? Whoops, old browser session. Ignore that.