Summary: | UI-side scripts in WebKitTestRunner should wait until event handling completes before finishing | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Component: | Tools / Tests | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, lforschler, rniwa, simon.fraser, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | iPhone / iPad | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2015-11-10 09:00:09 PST
Created attachment 265200 [details]
Patch
Comment on attachment 265200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265200&action=review > Tools/WebKitTestRunner/UIScriptContext/Bindings/UIScriptController.idl:53 > - void uiScriptComplete(DOMString result); > + void uiScriptCompleteAfterHandlingEvents(DOMString result); This probably could have been an implementation detail of uiScriptComplete instead of requiring a rename. We don't list all the other things that happen between calling this and actually being /done/. > Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:53 > unsigned prepareForAsyncTask(JSValueRef taskCallback); > + unsigned prepareForAsyncTaskBlockingScriptCompletion(JSValueRef taskCallback); Do we ever want async tasks *not* to block uiScriptComplete? Is it ever bad if they do? It would be nice to only have one that always does the right thing, to make it harder to write a broken test (and not change the name like above). Comment on attachment 265200 [details] Patch Attachment 265200 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/411197 New failing tests: fast/harness/concurrent-ui-side-scripts.html fast/harness/ui-side-scripts.html Created attachment 265208 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 265272 [details]
Patch
Comment on attachment 265272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265272&action=review > Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:49 > +const unsigned minimumPersistentCallbackID = 1000; > +typedef enum { > + CallbackTypeNonPersistent, > + CallbackTypeWillBeginZooming = minimumPersistentCallbackID, > + CallbackTypeDidEndZooming = minimumPersistentCallbackID + 1, > + CallbackTypeDidShowKeyboard = minimumPersistentCallbackID + 2, > + CallbackTypeDidHideKeyboard = minimumPersistentCallbackID + 3 > +} CallbackType; I think it's better to reserve the low range for persistent callbacks; that way, the upper bound for other callbacks is much higher. Comment on attachment 265272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265272&action=review >> Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:49 >> +} CallbackType; > > I think it's better to reserve the low range for persistent callbacks; that way, the upper bound for other callbacks is much higher. That makes sense -- changed to use minimumNonPersistentCallbackID. Created attachment 265286 [details]
Patch
Comment on attachment 265286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265286&action=review > Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.cpp:158 > + m_uiScriptResultsPendingCompletion.add(m_currentScriptCallbackID, result ? JSStringRetain(result) : result); : nullptr I think. > Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.cpp:188 > + for (WTF::KeyValuePair<unsigned, Task> entry : m_callbacks) { auto > Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:42 > +const unsigned minimumNonPersistentCallbackID = 1000; How about firstNonPersistentCallbackID and a blank line after this. Committed r192314: <http://trac.webkit.org/changeset/192314> |