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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 2 08:23:26 PDT 2008


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 &&

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

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

 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

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.

More information about the webkit-unassigned mailing list