[webkit-reviews] review granted: [Bug 6571] New Web (DOM) Inspector : [Attachment 5707] Inspector Patch (first cut)

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Sun Jan 15 17:37:13 PST 2006

Darin Adler <darin at apple.com> has granted Timothy Hatcher
<timothy at hatcher.name>'s request for review:
Bug 6571: New Web (DOM) Inspector

Attachment 5707: Inspector Patch (first cut)

------- Additional Comments from Darin Adler <darin at apple.com>
+  scrollIntoViewIfNeeded	DOMElement::ScrollIntoView     
DontDelete|Function 1

Bug on this line. Needs to say DOMElement::ScrollIntoViewIfNeeded.

+	 return rects;
+    } else if(!firstChild())
+	 return QValueList<IntRect>();

No need for else after return. Need space after if.

+    RenderObject *child = firstChild();
+    while (child) {

More elegant to write the above as a for loop.

+	 return IntRect(0,0,0,0);

Should use spaces after commas.

- at interface DOMElement (DOMElementExtensions)
-- (void)focus;
-- (void)blur;
- at end
Not good to move public API out of a public header DOMExtensions.h. Should put
the new stuff in a new category temporarily and leave the already-public stuff

Are we going to make this localizable?

+	     return [_private->searchResults count];
+	 else if (!item)
+	     return 1;

No need for else after return.

+	     return [_private->searchResults objectAtIndex:index];
+	 else if (!item)
+	     return _private->rootNode;


+    DOMNode *currentNode = [node parentNode];
+    while (currentNode) {
+	 if ([self isSameNode:currentNode])
+	     return YES;
+	 currentNode = [currentNode parentNode];
+    }

Above looks like it should be a for loop.

Looks like a great start. At least that very first thing I mentioned should be
fixed before landing, but I'm going to be loose here and say review+. Lets get
that first cut in there!


More information about the webkit-reviews mailing list