[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





--- Comment #23 from Joseph Pecoraro <joepeck at webkit.org>  2010-05-13 23:54:12 PST ---
(From update of attachment 55551)
I just applied the patch locally and it wasn't working. I suspect
this may be an issue with the recent style panel changes. I
apologize for looking at this so late. I will comment on a few
things. Again, the patch looks great overall. Mostly just some
style issues need fixing, and to make this work again with ToT.

Also, the next patch that you put up, set the "review" flag
to be "?". That marks it for review and it won't get lost =)


> +++ b/WebCore/ChangeLog

Use `prepare-ChangeLog --bug #####`, in this case use the number
is 17374 (the bugzilla number) to generate the ChangeLog entry.
I see you're using git, and probably across multiple commits and
I know there are some options for that. However, your ChangeLog is
fine, it would be nice to have more detail though. Examples I
like are:
http://trac.webkit.org/changeset/59443 (a recently landed patch)
http://trac.webkit.org/changeset/59372 (shamelessly my own)


> +++ b/WebCore/inspector/front-end/CSS.js

I like the idea of a new file, but I think maybe we can make this
a little more generic. Maybe Completion.js so that in the future
we can prepare for HTML/DOM attribute completions as well (when editing
DOM attributes). I think much of the code could be shared between
these.


> @@ -0,0 +1,37 @@
> +WebInspector.CSS = {
> +    properties: (function getCSSProperties() {

`get`CSSProperties doesn't seem accurate. Maybe "build"
or "generate" might be better. We may be looking into
backend solutions for this, but I think your solution
right now is great =).

> +        // Add shorthands.

Since there is a clever trick in the code below this might
want to point that out. Something like:

  // Add shorthands to the end of the properties list.


> +      if (typeof document.documentElement.style[shorthand] !== "undefined" && properties.indexOf(shorthand) < 0) {
> +          properties.push(shorthand);
> +      }

Single statement if's shouldn't have braces.


> +WebInspector.CSS.properties.startsWith = function startsWith(str)
> +{
> +    return this.filter(function(property) {
> +        return property.indexOf(str) === 0;
> +    });
> +};

It might be worth looking into sorting the list, and more quickly
finding the appropriate matches. For a list of ~220 this isn't too
bad but I could see this getting out of hand. For starters, using
a for loop instead of a filter would let you break early. Sorting
would let you do a binary search. This could be improved in the
future.


> +    if (!str)
> +        return "";

Add Newline here.

> +    for (var i = 0; i < this.length; ++i) {
> +        if (this[i].indexOf(str) === 0) {
> +            return this[i];
> +        }
> +    }

if statement doesn't need braces. And Web Inspector code tends to
go with the for loop then not needing braces either.


> +        if (arrowKeyPressed || pageKeyPressed && matches && matches.length) {

Make this clearer so we don't have to remember our || and && ordering:

  if ((arrowKeyPressed || pageKeyPressed) && matches && matches.length) {



> +                        } else {
> +                            return;
> +                        }

No braces for the else. Also, this might read better
as an early bail. It would reduce the nesting and that
tends to be the Web Inspector's usual style.



> +                }

Add newline here.


> +        var name = element.querySelector(".name");
> +        var value = element.querySelector(".value");

I prefer nameElement and valueElement as they point out its
an element. Just name and value sound too much like strings.


> +                if (newProperty && newProperty !== property) {
> +                    name.textContent = newProperty;
> +                }

> +                if (n < 0) {
> +                    name.firstChild.select(n);
> +                }

Remove Braces.

>          } else {
>              // FIXME: this should cycle through known keywords for the current property name.
>              return;

It looks like this could probably be removed. Or the
FIXME could be updated to say we need to autocomplete
the values now.


> +    if (start < 0) {
> +        start = end + start;
> +    }

No Braces.

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