Bug 135690

Summary: Web Inspector: DebuggerManager commands should return promises
Product: WebKit Reporter: Brian Burg <burg>
Component: Web InspectorAssignee: Brian Burg <burg>
Status: RESOLVED FIXED    
Severity: Normal CC: graouts, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 135772    
Bug Blocks: 135663    
Attachments:
Description Flags
Patch timothy: review+, burg: commit-queue-

Brian Burg
Reported 2014-08-06 23:13:29 PDT
This could include pause, resume, stepOver, stepInto, stepOut. Note that these should add one-time listeners to the relevant DebuggerManager events, rather than returning the same promise returned by InspectorBackend commands. The latter may fire before the debugger manager has actually received the relevant event from the backend.
Attachments
Patch (28.59 KB, patch)
2014-08-08 15:50 PDT, Brian Burg 		timothy: review+
burg: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2014-08-06 23:13:40 PDT
<rdar://problem/17942179>
Brian Burg
Comment 2 2014-08-08 15:50:02 PDT
Created attachment 236318 [details] Patch
Brian Burg
Comment 3 2014-08-08 15:51:03 PDT
Comment on attachment 236318 [details] Patch This needs to be tested manually once debugging works again. But, feel free to review before then.
Timothy Hatcher
Comment 4 2014-08-09 23:26:26 PDT
Comment on attachment 236318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236318&action=review > Source/WebInspectorUI/UserInterface/Base/EventListenerSet.js:27 > +WebInspector.EventListener = function(thisObject, fireOnce) Should be a seperate file. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:158 > + throw error; Why throw?
Brian Burg
Comment 5 2014-08-10 08:32:01 PDT
Comment on attachment 236318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236318&action=review >> Source/WebInspectorUI/UserInterface/Base/EventListenerSet.js:27 >> +WebInspector.EventListener = function(thisObject, fireOnce) > > Should be a seperate file. ok >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:158 >> + throw error; > > Why throw? From a catch block, you need to re-throw an exception. Otherwise, the overall chain will be resolved instead of rejected. We want the promise returned by this method to cause a larger chain to be rejected if any step fails.
Timothy Hatcher
Comment 6 2014-08-10 09:36:55 PDT
I thought that might be the case. Still a promise noob here.
Brian Burg
Comment 7 2014-08-10 16:37:50 PDT
(In reply to comment #6) > I thought that might be the case. Still a promise noob here. I just wrote up some such Promise gotchas on the Inspector style guide page.
Timothy Hatcher
Comment 8 2014-08-11 10:21:03 PDT
Comment on attachment 236318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236318&action=review > Source/WebInspectorUI/ChangeLog:26 > + Web Inspector: Add a helper to avoid leaking single-fire event listeners in Promise chains > + https://bugs.webkit.org/show_bug.cgi?id=135772 Should remove this from this patch, which I think you already noticed.
Joseph Pecoraro
Comment 9 2014-08-11 12:32:27 PDT
Comment on attachment 236318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236318&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:148 > + listener = new WebInspector.EventListener(this, true); Missing "var" for listener, otherwise this leaks into global scope. [Applies to multiple places in this patch].
Brian Burg
Comment 10 2014-08-11 14:26:40 PDT
Committed r172412: <http://trac.webkit.org/changeset/172412>
Note You need to log in before you can comment on or make changes to this bug.