[webkit-reviews] review granted: [Bug 200950] Web Inspector: unify agent command error messages : [Attachment 376836] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200950: Web Inspector: unify agent command error messages
https://bugs.webkit.org/show_bug.cgi?id=200950

Attachment 376836: Patch

https://bugs.webkit.org/attachment.cgi?id=376836&action=review




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 376836
  --> https://bugs.webkit.org/attachment.cgi?id=376836
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.