[Webkit-unassigned] [Bug 18224] Adding new CSS style properties to HTML nodes using Web Inspector

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=18224


aroben at apple.com changed:

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




------- Comment #6 from aroben at apple.com  2008-04-02 08:23 PDT -------
(From update of attachment 20219)
Thanks for the patch! I'm impressed by how closely it matches the style of the
existing Inspector code. A couple of comments:

You ChangeLog should explain the changes you made to each function.

+    this.addNewPropertyElement.title = "Add property";

All strings displayed in the Inspector's UI need to be obtained through the
WebInspector.UIString function. You'll have to add your new string to
WebCore/English.lproj/InspectorLocalizedStrings.js and call UIString here.

Your changes in PropertiesSection.js break the "Properties" pane.
PropertiesSection is used by both the Styles pane and the Properties pane. Your
changes should move to StylePropertiesSection in StylesSidebarPane.js

I'm confused by the name of the "this.addNewInlineStyle" property added to
StylesSidebarPane. The name is confusing because it's a verb phrase, so to me
it sounds more like a function name.

I actually think this.addNewInlineStyle doesn't need to be a property at all,
and can just be another optional argument to the update function, called
something like "alwaysShowInlineStyle".

The "newInlineStyle" function should probably be called "addNewInlineStyle",
since that's a verb phrase. I also think it should be bound to the
StylesSidebarPane object, instead of to the newInlineStyleElement. So your
event listener would change to this.addNewInlineStyle.bind(this).

 220                 if (editable && !(section.styleRule.parentStyleSheet &&
!section.styleRule.parentStyleSheet.ownerNode &&
!section.styleRule.parentStyleSheet.href))

It think we need to move "parentStyleSheet && !ownerNode && !href" into a
helper function so we're not duplicating the logic so much in this file.

I think it would be better for the new property to start out completely blank,
instead of starting out as ": ;". I just find that confusing and it gets in the
way.

584          parseElement.setAttribute("style", userInput);
 627         parseElement.setAttribute("style", userInput.replace(/;$/g,
"").trimWhitespace() + ";");

What's the purpose of this change? Again, things like this should be mentioned
in the ChangeLog.

 690     isPropertyEmpty: function(propertyString)
 691     {
 692         if (!propertyString.match(/([a-zA-Z]+)/g)) {
 693             if (this.treeOutline.section && this.treeOutline.section.pane)
 694                 this.treeOutline.section.pane.update();
 695             this.parent.removeChild(this);
 696         }

"isPropertyEmpty" is not a good name for a function that removes an empty
property. Something like "removeIfEmpty" would be better. I'm also not sure
what the purpose of the regular expression is. And why is this needed? As far
as I can tell we already remove properties if they are empty when you finish
editing.

 2204 .section.expanded .hidden {
 2205     display: none;
 2206 }
21812207 \ No newline at end of file

You should put a newline at the end of this file.

I know it sounds like a lot, but this patch is in really good shape! Thanks
again!


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