[webkit-reviews] review denied: [Bug 37625] [Chromium] Add some notifications and an accessor to WebKit API : [Attachment 53447] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jens Alfke
<snej at chromium.org>'s request for review:
Bug 37625: [Chromium] Add some notifications and an accessor to WebKit API
https://bugs.webkit.org/show_bug.cgi?id=37625

Attachment 53447: Patch
https://bugs.webkit.org/attachment.cgi?id=53447&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/src/WebElement.cpp:76
 +  // Note: This method only works properly after layout has occurred.
This comment would be helpful in the public header file.

WebKit/chromium/src/WebElement.cpp:79
 +	// (Copied from HTMLAnchorElement::isKeyboardFocusable)
It seems rather unfortunate to duplicate this code.  Can it be shared somehow?

WebKit/chromium/public/WebFrameClient.h:198
 +	// The frame's document finished the initial layout of a page.
This would probably be better in the "Geometry notifications" section instead
of the "Navigational notifications" section.

WebKit/chromium/public/WebElement.h:60
 +	    WEBKIT_API bool isVisible() const;
how about naming this method hasNonEmptyBoundingBox?  that way the method name
is self documenting and more accurate.	if a consumer wants to use that to test
visibility, then the fact that it is not a 100% guarantee is an issue for the
user instead of a problem with the implementation of this method.