[webkit-reviews] review denied: [Bug 18224] Adding new CSS style properties to HTML nodes using Web Inspector : [Attachment 20219] Tabs were removed.

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Markus Knaup
<mk at digitaleprodukte.de>'s request for review:
Bug 18224: Adding new CSS style properties to HTML nodes using Web Inspector
http://bugs.webkit.org/show_bug.cgi?id=18224

Attachment 20219: Tabs were removed.
http://bugs.webkit.org/attachment.cgi?id=20219&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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!