Bug 73259 - [chromium] add event methods to WebFrame
Summary: [chromium] add event methods to WebFrame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Karl Koscher
URL:
Keywords:
Depends on:
Blocks: 73337
  Show dependency treegraph
 
Reported: 2011-11-28 15:01 PST by Karl Koscher
Modified: 2011-12-09 17:09 PST (History)
3 users (show)

See Also:


Attachments
Patch (11.26 KB, patch)
2011-11-28 16:00 PST, Karl Koscher
no flags Details | Formatted Diff | Diff
Patch (11.44 KB, patch)
2011-11-28 17:38 PST, Karl Koscher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Karl Koscher 2011-11-28 15:01:23 PST
In preparation for getting cross-process postMessage working in Chromium, we need an API to send message events to frames. We also need an API for adding event handlers to frames so that we can intercept message events to our proxy frames. While we could dispatch the message event to the document and let it bubble, we can't capture message events on the document (they are directly dispatched to the DOMWindow), so we at least need methods to add and remove event handlers. While we're at it, we might as well add an API so we can dispatch messages directly as well to be consistent with the same-process behavior.
Comment 1 Karl Koscher 2011-11-28 16:00:30 PST
Created attachment 116845 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-28 16:04:34 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-11-28 16:31:13 PST
Comment on attachment 116845 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116845&action=review

> Source/WebKit/chromium/public/WebFrame.h:546
> +    // Events --------------------------------------------------------------

nit: please preserve two new lines above section headers and one new line below.

> Source/WebKit/chromium/public/WebFrame.h:547
> +    virtual void addEventListener(const WebString& eventType,

Please add a comment indicating that these operate on the DOM Window.

Do you care if web content is able to generate synthetic DOM events that you
intercept?  Do we need to worry about that introducing security holes through
privilege escalation?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1861
> +    if (!event.isNull())

I think you should be able to assume that you are given a non-null event.  I would just ASSERT(!event.isNull())
Comment 4 Karl Koscher 2011-11-28 17:38:46 PST
Created attachment 116857 [details]
Patch
Comment 5 Karl Koscher 2011-11-28 17:41:58 PST
Comment on attachment 116845 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116845&action=review

>> Source/WebKit/chromium/public/WebFrame.h:546
>> +    // Events --------------------------------------------------------------
> 
> nit: please preserve two new lines above section headers and one new line below.

Fixed.

>> Source/WebKit/chromium/public/WebFrame.h:547
>> +    virtual void addEventListener(const WebString& eventType,
> 
> Please add a comment indicating that these operate on the DOM Window.
> 
> Do you care if web content is able to generate synthetic DOM events that you
> intercept?  Do we need to worry about that introducing security holes through
> privilege escalation?

Comment added.

I talked with Charlie and we both agree that it's an acceptable risk. WebKit already exposes these functions internally. Any users of this API will need to consider the fact that these events might be synthetic, though. I added a comment to that effect.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1861
>> +    if (!event.isNull())
> 
> I think you should be able to assume that you are given a non-null event.  I would just ASSERT(!event.isNull())

Fixed.
Comment 6 WebKit Review Bot 2011-11-29 22:54:57 PST
Comment on attachment 116857 [details]
Patch

Rejecting attachment 116857 [details] from commit-queue.

New failing tests:
svg/transforms/text-with-pattern-with-svg-transform.svg
Full output: http://queues.webkit.org/results/10696086
Comment 7 WebKit Review Bot 2011-12-09 17:09:24 PST
Comment on attachment 116857 [details]
Patch

Clearing flags on attachment: 116857

Committed r102495: <http://trac.webkit.org/changeset/102495>
Comment 8 WebKit Review Bot 2011-12-09 17:09:29 PST
All reviewed patches have been landed.  Closing bug.