[Webkit-unassigned] [Bug 17374] Inspector should support tab completion while editing CSS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=17374


Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59166|review?                     |review-
               Flag|                            |




--- Comment #38 from Joseph Pecoraro <joepeck at webkit.org>  2010-06-19 01:03:19 PST ---
(From update of attachment 59166)
> diff --git a/WebCore/inspector/front-end/CSS.js b/WebCore/inspector/front-end/CSS.js

I still don't like this file being called CSS.js. Its purpose
is more about autocompletion. I still like "Completion.js"
or something along those lines.

Also, when you add a file you should update a number of build files.
You can see the following for an example of most of them:
http://trac.webkit.org/changeset/60530

  - [easy] WebCore/inspector/front-end/WebKit.qrc
  - [easy] WebCore/WebCore.gypi
  - [optional, if you have Windows handy] WebCore/WebCore.vcproj/WebCore.vcproj
  - [optional, if you have XCode handy] WebCore/WebCore.xcodeproj/project.pbxproj


> +WebInspector.CSS = {
> +    properties: (function buildCSSProperties() {
> +        var properties = Array.convert(document.defaultView.getComputedStyle(document.documentElement, ""));
> +        var length = properties.length;
> +        // Add shorthands to the end of the properties list.
> +        for (var i = 0; i < length; ++i) {
> +            var propertyWords = properties[i].split("-");
> +            var j = propertyWords.length;
> +            while (--j) {
> +                var shorthand = propertyWords.slice(0, j).join("-");
> +                if (typeof document.documentElement.style[shorthand] !== "undefined" && properties.indexOf(shorthand) < 0)
> +                    properties.push(shorthand);
> +            }
> +        }
> +        return properties.sort();
> +    })()
> +}

The implementation of this function is okay But it can
be improved quite a bit. I played around and implemented
a version twice a fast. However, this already takes ~2ms
so its not really that important to improve. Some simple
things I think would be worth doing that keep the
readability while not going overboard are:

  - cache `document.documentElement.style` into a variable
    and remove the typeof. Resulting in:

    // Early on...
    var style = document.documentElement.style;

    // In the condition...
    style[shorthand] !== undefined

  - change the "slice().join()" call to a "pop(); join()"

    propertyWords.pop();
    var shorthand = propertyWords.join("-");

These few changes along should really speed things up
without affecting the readability. Other improvements
are mentioned here:
http://gist.github.com/444648

Ultimately I think the engine should provide this list,
but that is beyond the scope of this bug.

Also, after some further thought, I think this should
be an unnamed anonymous function. Giving it a name
makes it an unnecessary global.



> +WebInspector.CSS.properties.startsWith = function startsWith(prefix)
> +{
> +    // FIXME: Use binary search.
> +    return this.filter(function(property) {
> +        return property.indexOf(prefix) === 0;
> +    });
> +}

With a bit of refactoring, this could just use the binary search
you implemented in firstStartsWith. And, doing so you can achieve
a pretty quick ~20x speedup because of the nature of the data.
Again, not something particularly necessary to speedup, but since
you already have it!

Make a helper function, like firstStartsWith, that returns the
index of the first starting item (and -1 if not found). Then
its trivial to implement firstStartsWith and startsWith on top
of the binary search implementation.

To see what I mean:
http://gist.github.com/444674


> +        var middleIndex = (maxIndex + minIndex) >> 1; // Same as Math.floor((maxIndex + minIndex) / 2), but twice faster.

You can remove this comment, or just shorten
it to note that this handles the flooring.


> +    index = (index + this.length + shift) % this.length;
> +
> +    if (!prefix)
> +        return this[index];

The top statement can go inside the if block. Its not
necessary otherwise.


> \ No newline at end of file

We want that newline!


> +        // Restore <span class="webkit-css-property"> if it doesn't yet exist or was actidentally deleted.

Typo: actidentally => accidentally


> +Array.convert = function convert(list)
> +{
> +    // Cast array-like object to an array.
> +    return Array.prototype.slice.call(list);
> +}
> +

Again, I think this should be unnamed to prevent an
extra global reference. I know this helps JSC find the
function name in debugging, but I don't think its
that big of a deal in this case. Also, the engine
should be improved to infer the name. I think v8 does.



I hope this review didn't come across as bad. I really
liked the patch. I just think it could use a little more
polish! Cheers!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.