[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 #17 from Joseph Pecoraro <joepeck at webkit.org>  2010-05-04 14:09:29 PST ---
> While <li> is contentEditable, <span class="name"> might be deleted. If it
> happens, error occurs:

I'd have to look into this deeper if you want a good comment. I think this
might be expected and you could just handle that with an if statement =)


(In reply to comment #16)
> I'll keep developing in
> http://github.com/NV/web-inspector/tree/css_autocomplete , until I've got a
> solid solution.

Very Nice!! I'll put some comments here (and some in GitHub). I think you're
heading in the right direction.

There isn't anything particularly wrong with your patch, but I feel like I
should raise some concerns so you can provide a high quality patch. WebKit is
very keen on keeping a consistent style, and the Web Inspector has had an
exceptional record in this regard. Here are some style issues that would should
address.

> properties: (function getCSSProperties(){
Space between () and {. There are a few of these in your patch.

> +        // Add shorthands
> +    // convert array-like object to array
Comments should be sentences starting with a capital and ending in a period.

> + var s = ...
> + var j = ...
> + var prop = ...
Use full names except in rare cases.  It really helps readability and readers
like me!

> +                var prop = s.slice(0, j).join('-');
Unless there is a really good reason, always use double quoted strings ("-").

> +                if (typeof document.documentElement.style[prop] !== 'undefined' && properties.indexOf(prop) < 0) {
> +                    properties.push(prop);
> +                }
Single line if statements should not have braces. There are a few of these in
your patch.

> +        return prop.indexOf(str) === 0
Add a semicolon. I think there are few exceptions to the semicolon rule.

> if (!str) return '';
Don't single line this. Make it two lines and add a blank line after it.

> +    for (var i=0; i<this.length; i++) {
Should be => for (var i = 0; i < this.length; ++i) {

> \ No newline at end of file
We don't like that!

> +                var new_property = WebInspector.CSS.properties.firstStartsWith(property);
We prefer camelCase => newProperty

> if (new_property != property) {
Unless there is a really good reason, always use the strict equality
comparisons (=== and !==)

> +Array.convert = function convert(list)
> + ...
> +};
Here would be an exception, I don't think we normally put the semicolon here. 


You extend a number of core types:

    KeyboardEvent.prototype.character (getter)
    Text.prototype.select
    Array.convert
    Array.prototype.last (getter/setter)

I think some of these are nice but unnecessary. For instance the use of
Array.convert in your patch is mostly just extra processing. I'll hold off on
commenting on the others. I'm a fan, but we typically hold off on this.

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