Bug 151848 – Web Inspector: Remove untested and unused Worker inspection

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Remove untested and unused Worker inspection.

* NOTES
The inspector frontend currently doesn't expose UI for debugging workers, so this code has been maintained for a few years without any way for a user to access it. Likewise, some pieces were implemented in V8 and not implemented in JavaScriptCore so it is possible that if tested some pieces would not be fully functional. Rather then keep the code in half-maintained, lets remove it and re-add it when we will focus on adding proper Worker Inspection.

See:
<https://webkit.org/b/127634> Web Inspector: support debugging web workers

[Radar WebKit Bug Importer]

<rdar://problem/23755748>

[Joseph Pecoraro]

Created attachment 266592 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 266593 [details]
[PATCH] Proposed Fix

Rebased.

[WebKit Commit Bot]

Attachment 266593 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 266596 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 266596 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=266596&action=review

r=me with some nits.

> Source/JavaScriptCore/inspector/protocol/Worker.json:-8
> -        },

Please update the legacy protocol files as well.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:-19059
> -    <ClCompile Include="..\bindings\js\WorkerScriptDebugServer.cpp">

You don't need to modify the vcxproj files, they aren't used by anything.

> Source/WebCore/inspector/InspectorConsoleInstrumentation.h:-47
> -inline void InspectorInstrumentation::addMessageToConsole(WorkerGlobalScope* workerGlobalScope, std::unique_ptr<Inspector::ConsoleMessage> message)

Why not remove this method entirely?

> Source/WebCore/inspector/InspectorTimelineAgent.h:95
> +    enum InspectorType { PageInspector };

Might as well nuke this enum too.

> Source/WebCore/inspector/InspectorWebAgentBase.h:58
>  

Should we sink PageAgentContext into WebAgentContext, or keep this around for the future?

> Source/WebCore/inspector/WorkerDebuggerAgent.h:-42
> -class WorkerDebuggerAgent final : public WebDebuggerAgent {

Let's mark WebDebuggerAgent (as well as other similar sibling classes) as final.

[Joseph Pecoraro]

(In reply to comment #6)
> Comment on attachment 266596 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266596&action=review
> 
> r=me with some nits.

Thanks!

> > Source/JavaScriptCore/inspector/protocol/Worker.json:-8
> > -        },
> 
> Please update the legacy protocol files as well.

Okay.

> > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:-19059
> > -    <ClCompile Include="..\bindings\js\WorkerScriptDebugServer.cpp">
> 
> You don't need to modify the vcxproj files, they aren't used by anything.

Yeah, but as long as the files exist I'm going to update them.


> > Source/WebCore/inspector/InspectorConsoleInstrumentation.h:-47
> > -inline void InspectorInstrumentation::addMessageToConsole(WorkerGlobalScope* workerGlobalScope, std::unique_ptr<Inspector::ConsoleMessage> message)
> 
> Why not remove this method entirely?

I think it would be a good starting point to get console.log working within a Worker without adding complete Worker inspection.


> > Source/WebCore/inspector/InspectorWebAgentBase.h:58
> >  
> 
> Should we sink PageAgentContext into WebAgentContext, or keep this around
> for the future?

Keep it around. If we can't get to Worker support soon, then maybe, but I'm optimistic.


> > Source/WebCore/inspector/WorkerDebuggerAgent.h:-42
> > -class WorkerDebuggerAgent final : public WebDebuggerAgent {
> 
> Let's mark WebDebuggerAgent (as well as other similar sibling classes) as
> final.

Where there was a "Web" there was normally a "Page" and "Worker".

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/193426>

[Brent Fulgham]

*** Bug 148642 has been marked as a duplicate of this bug. ***