Bug 37625 - [Chromium] Add some notifications and an accessor to WebKit API
Summary: [Chromium] Add some notifications and an accessor to WebKit API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jens Alfke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-14 16:47 PDT by Jens Alfke
Modified: 2010-04-27 18:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.74 KB, patch)
2010-04-15 10:22 PDT, Jens Alfke
fishd: review-
Details | Formatted Diff | Diff
patch, after initial feedback (7.50 KB, patch)
2010-04-23 13:13 PDT, Jens Alfke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jens Alfke 2010-04-14 16:47:31 PDT
For some work I'm doing on Chromium* I need to add a few methods to the WebKit API to expose WebCore functionality:

1. WebElement::isVisible — tells whether a DOM element is visible (defined as 'has a layout with a non-empty bounding box').
2. WebFrame::didFirstLayout and WebFrame::didFirstVisuallyNonEmptyLayout — these are equivalent to the FrameLoaderClient methods of the same names.

* http://code.google.com/p/chromium/issues/detail?id=777
Comment 1 Jens Alfke 2010-04-15 10:22:25 PDT
Created attachment 53447 [details]
Patch
Comment 2 Jens Alfke 2010-04-15 10:28:30 PDT
I actually linked to the wrong Chromium bug, below. The correct one is
"Password manager heuristics fail for sites that keep the login form around after signin"
http://code.google.com/p/chromium/issues/detail?id=28911
Comment 3 Darin Fisher (:fishd, Google) 2010-04-22 14:47:29 PDT
Comment on attachment 53447 [details]
Patch

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.
Comment 4 Jens Alfke 2010-04-23 13:13:34 PDT
Created attachment 54186 [details]
patch, after initial feedback

OK, I have moved the hasNonEmptyBoundingBox method up to Node, and made the other changes you suggested.
Comment 5 Jens Alfke 2010-04-27 10:39:27 PDT
The Chromium patch (fix for 28911) that's depending on this is now approved, and waiting on this patch to get checked in and downstreamed to Chromium first.
Comment 6 Darin Fisher (:fishd, Google) 2010-04-27 10:51:58 PDT
Comment on attachment 54186 [details]
patch, after initial feedback

R=me
Comment 7 WebKit Commit Bot 2010-04-27 17:55:17 PDT
Comment on attachment 54186 [details]
patch, after initial feedback

Clearing flags on attachment: 54186

Committed r58362: <http://trac.webkit.org/changeset/58362>
Comment 8 WebKit Commit Bot 2010-04-27 17:55:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2010-04-27 18:21:47 PDT
http://trac.webkit.org/changeset/58362 might have broken Qt Linux Release