Web Inspector: unify agent command error messages

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Different agents can sometimes have different error messages for commands that have a similar intended effect.  We should make our error messages more similar.

[Devin Rousso]

Created attachment 376836 [details]
Patch

This will likely require rebasing some tests

[Joseph Pecoraro]

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 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]

Created attachment 377288 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377288 [details]
Patch

Clearing flags on attachment: 377288

Committed r249128: <https://trac.webkit.org/changeset/249128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54730799>