Bug 200950

Summary: Web Inspector: unify agent command error messages
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 200854    
Attachments:
Description Flags
Patch
none
Patch none

Devin Rousso
Reported 2019-08-20 16:22:21 PDT
Different agents can sometimes have different error messages for commands that have a similar intended effect. We should make our error messages more similar.
Attachments
Patch (96.75 KB, patch)
2019-08-20 18:11 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (119.23 KB, patch)
2019-08-26 16:36 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2019-08-20 18:11:58 PDT
Created attachment 376836 [details] Patch This will likely require rebasing some tests
Joseph Pecoraro
Comment 2 2019-08-24 11:17:09 PDT
Comment on attachment 376836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376836&action=review rs=me > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:544 > -void InspectorDOMDebuggerAgent::removeURLBreakpoint(ErrorString&, const String& url) > +void InspectorDOMDebuggerAgent::removeURLBreakpoint(ErrorString& errorString, const String& url) > { > if (url.isEmpty()) { > + if (!m_pauseOnAllURLsEnabled) > + errorString = "Breakpoint for all URLs missing"_s; > m_pauseOnAllURLsEnabled = false; > return; > } > > - m_urlBreakpoints.remove(url); > + auto result = m_urlBreakpoints.remove(url); > + if (!result) > + errorString = "Breakpoint for given url missing"_s; > } I don't like the trend of sending back errors even though no error actually happened. Example: calling enable twice is harmless. That said, sending an error in these cases might help the frontend catch an error where it is doing something unbalanced.
Devin Rousso
Comment 3 2019-08-26 14:19:51 PDT
Comment on attachment 376836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376836&action=review >> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:544 >> } > > I don't like the trend of sending back errors even though no error actually happened. Example: calling enable twice is harmless. That said, sending an error in these cases might help the frontend catch an error where it is doing something unbalanced. Yeah, I think sending an error can help the frontend know when it's getting out of sync. For most "additive" things (e.g. setting a breakpoint), I'd agree that two calls is kinda harmless, unless there's some configuration change. That having been said, these errors are things that shouldn't be seen, as otherwise it means that we've gotten out of sync somehow, so I think the more errors the better. For what it's worth, `enable` is probably a bad example because most agents actually do a lot of things inside their `enable` that can do serious havoc for the inspector frontend.
Devin Rousso
Comment 4 2019-08-26 16:36:43 PDT
Created attachment 377288 [details] Patch
WebKit Commit Bot
Comment 5 2019-08-26 18:02:28 PDT
Comment on attachment 377288 [details] Patch Clearing flags on attachment: 377288 Committed r249128: <https://trac.webkit.org/changeset/249128>
WebKit Commit Bot
Comment 6 2019-08-26 18:02:29 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 7 2019-08-26 18:03:20 PDT
<rdar://problem/54730799>
Note You need to log in before you can comment on or make changes to this bug.