Summary: | Web Inspector: Breakpoint Actions | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, eflews.bot, graouts, gyuyoung.kim, joepeck, mkwst, rniwa, timothy, webkit-bug-importer, webkit-ews | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2013-09-01 02:16:38 PDT
Created attachment 210234 [details]
[PATCH] 1 - Backend and Protocol Tests
Created attachment 210235 [details]
[PATCH] 2 - Frontend
Created attachment 210236 [details]
[PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor
Created attachment 210237 [details]
[IMAGE] Popover - No Actions
Created attachment 210238 [details]
[IMAGE] Popover - All Actions
Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests View in context: https://bugs.webkit.org/attachment.cgi?id=210234&action=review > Source/WebCore/bindings/js/ScriptDebugServer.cpp:445 > + // ASSERT(m_currentCallFrame); So, I have to debug this. It happens almost 100% running LayoutTest/inspector-protocol/debugger/setBreakpoint-options-exception.html. But things seem to work as expected. Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests Attachment 210234 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1656607 Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests Attachment 210234 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1656608 Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests Attachment 210234 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1641640 *** Bug 66867 has been marked as a duplicate of this bug. *** Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests Attachment 210234 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1609550 Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests Attachment 210234 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1657569 Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests Attachment 210234 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1675455 Comment on attachment 210234 [details] [PATCH] 1 - Backend and Protocol Tests View in context: https://bugs.webkit.org/attachment.cgi?id=210234&action=review > Source/WebCore/bindings/js/ScriptDebugServer.cpp:205 > + case ScriptBreakpointActionTypeSound: { > + systemBeep(); > + break; > + } No need for the extra braces here. >> Source/WebCore/bindings/js/ScriptDebugServer.cpp:445 >> + // ASSERT(m_currentCallFrame); > > So, I have to debug this. It happens almost 100% running LayoutTest/inspector-protocol/debugger/setBreakpoint-options-exception.html. But things seem to work as expected. Perhaps the nested evaluate or reportException calls are confusing the debugger? > Source/WebCore/inspector/Inspector.json:2667 > + { "name": "type", "type": "string", "enum": ["log", "eval", "sound"], "description": "Different kinds of breakpoint actions." }, "evaluate"? It would match the others better and other protocol methods that spell it out. Comment on attachment 210235 [details] [PATCH] 2 - Frontend View in context: https://bugs.webkit.org/attachment.cgi?id=210235&action=review Nice an clean! > Source/WebInspectorUI/UserInterface/Breakpoint.js:43 > + actions[i] = WebInspector.BreakpointAction.fromProtocol(this, actions[i]); I wouldn't call this "fromProtocol". It is really about save and restore from localStorage. So it is really "fromInfo" given the rename I also suggest for the protocolObject getter. I would prefer to special case the WebInspector.BreakpointAction constructor to check the type of the second argument and do either normal or info object construction. I'm not a fan of separate "from" constructor helpers if they can be avoided. (Call me jaded by the old way "fromPayload" helpers leaked protocol data all over the Inspector code.) > Source/WebInspectorUI/UserInterface/BreakpointAction.js:41 > +WebInspector.BreakpointAction.fromProtocol = function(breakpoint, breakpointActionObject) > +{ > + return new WebInspector.BreakpointAction(breakpoint, breakpointActionObject.type, breakpointActionObject.data); > +} See previous comment. > Source/WebInspectorUI/UserInterface/BreakpointAction.js:73 > + get protocolObject() I'd call this info() to match Breakpoint since this is used for both the protocol and saving to localStorage and add a comment here that it is used for those. > Source/WebInspectorUI/UserInterface/BreakpointActionView.css:49 > + background-image: url(Images/BreakpointActionAdd.svg), -webkit-linear-gradient(top, rgb(250, 250, 250), rgb(200, 200, 200)); Why does the gradient need to be here? Can it be moved to the SVG? I'm not sure how this clips the gradient to the SVG circle! > Source/WebInspectorUI/UserInterface/BreakpointActionView.css:72 > + -webkit-appearance: textfield; Clever! But this adds some padding at the top and bottom that push the line gutter down when it is showing. Not an issue in this patch since it is disabled, but something to think about. The CodeMirror instance might be able to set negative top and bottom margins to compensate. > Source/WebInspectorUI/UserInterface/BreakpointActionView.css:78 > + width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */ calc()? > Source/WebInspectorUI/UserInterface/BreakpointActionView.css:83 > + width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */ calc() here too? > Source/WebInspectorUI/UserInterface/BreakpointActionView.js:128 > + case DebuggerAgent.BreakpointActionType.Log: Yay enums! > Source/WebInspectorUI/UserInterface/BreakpointActionView.js:152 > + indentWithTabs: true, > + indentUnit: 4, > + matchBrackets: true, I wonder if there is a way to make these default so we don't need to set them everywhere? > Source/WebInspectorUI/UserInterface/Images/BreakpointActionAdd.svg:5 > + <path stroke="#F1F2F3" stroke-width="2" d="M 4 9 L 12 9 M 8 5 L 8 13"/> > + <path stroke="#818181" stroke-width="2" d="M 4 8 L 12 8 M 8 4 L 8 12"/> I like rgb() in the SVGs. Comment on attachment 210236 [details] [PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor View in context: https://bugs.webkit.org/attachment.cgi?id=210236&action=review > Source/WebInspectorUI/ChangeLog:13 > + The logic was inside of JavaScriptLogViewController but was already > + entirely independent. Factor it out into its own class and plug it into > + CodeMirrorCompletionController as a "CompletionsProvider". Nice! > Source/WebInspectorUI/ChangeLog:56 > +2013-09-01 Joseph Pecoraro <pecoraro@apple.com> > + > + Web Inspector: Breakpoint Actions > + https://bugs.webkit.org/show_bug.cgi?id=120576 > + > + Reviewed by NOBODY (OOPS!). > + Oops! > Source/WebInspectorUI/UserInterface/BreakpointActionView.js:160 > + completionController.setExtendedCompletionProvider("javascript", WebInspector.JavaScriptRuntimeCompletionProvider.instance); I think WebInspector.JavaScriptRuntimeCompletionProvider.instance is odd. I would expect WebInspector.globalJavaScriptRuntimeCompletionProvider or something. > Source/WebInspectorUI/UserInterface/CodeMirrorCompletionController.js:95 > + setExtendedCompletionProvider: function(modeName, provider) > + { > + this._extendedCompletionProviders[modeName] = provider; > + }, I would almost call this "addExtendedCompletionProvider" so it can be multiple. But that is just me wanting to avoid "set" which makes me want this to be a property. Comment on attachment 210236 [details] [PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor View in context: https://bugs.webkit.org/attachment.cgi?id=210236&action=review >> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:160 >> + completionController.setExtendedCompletionProvider("javascript", WebInspector.JavaScriptRuntimeCompletionProvider.instance); > > I think WebInspector.JavaScriptRuntimeCompletionProvider.instance is odd. I would expect WebInspector.globalJavaScriptRuntimeCompletionProvider or something. I guess "global" is redundant since it is on the global namespace. WebInspector.javaScriptRuntimeCompletionProvider? Comment on attachment 210235 [details] [PATCH] 2 - Frontend View in context: https://bugs.webkit.org/attachment.cgi?id=210235&action=review > Source/WebInspectorUI/UserInterface/BreakpointAction.js:75 > + var obj = {type: this._type}; I'm kind of bothered the type comes from the protocol and not another enum from the frontend. This can lead to a compatibility issue since this is stored in localStorage for an indefinite time and the enums can never change then. Ideally the DebuggerManager would be the gatekeeper for all the Breakpoint protocol translation. Comment on attachment 210235 [details] [PATCH] 2 - Frontend View in context: https://bugs.webkit.org/attachment.cgi?id=210235&action=review >> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:49 >> + background-image: url(Images/BreakpointActionAdd.svg), -webkit-linear-gradient(top, rgb(250, 250, 250), rgb(200, 200, 200)); > > Why does the gradient need to be here? Can it be moved to the SVG? I'm not sure how this clips the gradient to the SVG circle! I didn't want to duplicate the gradient in both SVG files. I was hoping I could share the gradient here. Though it isn't completely shared here at least they are close enough together you can compare at a glance. And finally, gradients in CSS are much shorter then their SVG counterparts. >> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:78 >> + width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */ > > calc()? Ahh, apparently width: 100% does what I want if I put it on both. Much easier! (In reply to comment #19) > (From update of attachment 210235 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210235&action=review > > > Source/WebInspectorUI/UserInterface/BreakpointAction.js:75 > > + var obj = {type: this._type}; > > I'm kind of bothered the type comes from the protocol and not another enum from the frontend. This can lead to a compatibility issue since this is stored in localStorage for an indefinite time and the enums can never change then. > > Ideally the DebuggerManager would be the gatekeeper for all the Breakpoint protocol translation. So I went with a patch that makes the frontend use only its own enum: WebInspector.BreakpointAction.Type = { Log: "log", … } And DebuggerManager converts it to the protocol enum when sending (setBreakpoint). I'm going to rebase and re-run tests. If it all checks out I will land. Committed: <http://trac.webkit.org/changeset/155132> <http://trac.webkit.org/changeset/155133> <http://trac.webkit.org/changeset/155134> Build fix follow-up: <http://trac.webkit.org/changeset/155136> Comment on attachment 210235 [details] [PATCH] 2 - Frontend View in context: https://bugs.webkit.org/attachment.cgi?id=210235&action=review >>> Source/WebInspectorUI/UserInterface/BreakpointActionView.css:78 >>> + width: 336px; /* NOTE: Fixed value, manually tuned to .edit-breakpoint-popover-content.wide width */ >> >> calc()? > > Ahh, apparently width: 100% does what I want if I put it on both. Much easier! Ahh, I found the case where this doesn't work. 1 long line of text wraps incorrectly and even expands if width is ever % based (e.g. 100% or calc(100%-30px)). I don't yet know if this is an engine bug or behaving correctly. I'll address this issue in a follow-up bug. |