Bug 42304

Summary:

DeviceOrientation event listeners should never be called synchronously from addEventListener()

Product:

WebKit

Reporter:

Steve Block <steveblock>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hans, jorlow, ojan, steveblock

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

30335

Attachments:

Description	Flags
Patch	none
Patch	jorlow: review+, jorlow: commit-queue-

Steve Block

Reported 2010-07-14 16:49:09 PDT

See Bug 39588 for background.

Attachments
Patch (4.84 KB, patch) 2010-07-16 02:42 PDT, Steve Block	no flags	Details Formatted Diff Diff
Patch (4.94 KB, patch) 2010-07-23 01:04 PDT, Steve Block	jorlow: review+ jorlow: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Steve Block

Comment 1 2010-07-16 02:42:05 PDT

Created attachment 61785 [details] Patch

Ojan Vafai

Comment 2 2010-07-22 17:05:46 PDT

Comment on attachment 61785 [details] Patch Even if you can't add layout tests you should add manual tests that can be converted to layout tests once LayoutTestController supports DeviceOrientation. Also, it would be good if the ChangeLog description could point to the bugs for adding the necessary support to LayoutTestController.

Steve Block

Comment 3 2010-07-23 00:53:01 PDT

> Even if you can't add layout tests you should add manual tests that can be converted to layout tests once > LayoutTestController supports DeviceOrientation. I don't think this is possible. Currently, no platforms enable DeviceOrientation, so there's no real implementation to test. Addition of a mock implementation is underway at in Bug 39589. I already have Bug 39590 to track adding DeviceOrientation layout tests and assure you that I'll add a test for this case as soon as the mock is in place! > Also, it would be good if the ChangeLog description could point to the bugs for adding the necessary > support to LayoutTestController. Will upload a patch to do this.

Steve Block

Comment 4 2010-07-23 01:04:41 PDT

Created attachment 62390 [details] Patch

Jeremy Orlow

Comment 5 2010-07-26 08:18:13 PDT

Comment on attachment 62390 [details] Patch WebCore/dom/DeviceOrientationController.cpp:48 + m_timer.stop(); why is this necessary? WebCore/dom/DeviceOrientationController.cpp:51 + m_client->lastOrientation() : DeviceOrientation::create(); I'd lean towards not wrapping these. WebCore/dom/DeviceOrientationController.cpp:65 + // immediately trigger an asynchronous response. This seems to imply that if it's not present, it would fire whenever it becomes so...which does't seem to be true. Also "immediately" and "asynchronous" kind of conflict. WebCore/dom/DeviceOrientationController.h:57 + typedef HashSet<DOMWindow*> ListenersSet; I'd lean towards putting newlines between the pairs of typedef+declarations...but it's more personal preference. r=me, but please consider making the above changes

Steve Block

Comment 6 2010-07-26 08:47:02 PDT

> WebCore/dom/DeviceOrientationController.cpp:48 > + m_timer.stop(); > why is this necessary? It's not, will remove. > WebCore/dom/DeviceOrientationController.cpp:51 > + m_client->lastOrientation() : DeviceOrientation::create(); > I'd lean towards not wrapping these. Will fix. > WebCore/dom/DeviceOrientationController.cpp:65 > + // immediately trigger an asynchronous response. I was trying to describe that the event is async, but triggered right now, without waiting for the client to get new data. Will update the comment.

Steve Block

Comment 7 2010-07-26 09:03:51 PDT

Committed r64048: <http://trac.webkit.org/changeset/64048>

Note You need to log in before you can comment on or make changes to this bug.