Different agents can sometimes have different error messages for commands that have a similar intended effect. We should make our error messages more similar.
Created attachment 376836 [details] Patch This will likely require rebasing some tests
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.
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.
Created attachment 377288 [details] Patch
Comment on attachment 377288 [details] Patch Clearing flags on attachment: 377288 Committed r249128: <https://trac.webkit.org/changeset/249128>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54730799>