Bug 34481 – Web Inspector: Start unforking profiler and debugger stuff

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Mikhail Naganov]

The goal is to enable "JAVASCRIPT_DEBUGGER" in Chromium port and have profiler / debugger stuff compiled --- this requires not to depend directly on JSC, for example.

As a first step, I'm removing custom implementations of Console, and migrating InspectorController towards using 'Script...' types instead of 'JSC::' ones.

Temporarily, there will be stuff under "#if ENABLE(JAVASCRIPT_DEBUGGER) || USE(V8)" conditionals. The second part will be removed back after migration will have been finished, and thus JAVASCRIPT_DEBUGGER will be enabled in Chromium.

[Mikhail Naganov]

Created attachment 47935 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/Console.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Skipping input 'WebCore/bindings/v8/custom/V8ConsoleCustom.cpp': Can't open for reading
WebCore/bindings/js/ScriptProfiler.cpp:31:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/bindings/v8/ScriptProfile.h:40:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/ScriptProfiler.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/bindings/v8/ScriptProfiler.cpp:40:  context_scope is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5


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

[Mikhail Naganov]

Created attachment 47941 [details]
proposed patch - fixed style violations

[Pavel Feldman]

Comment on attachment 47941 [details]
proposed patch - fixed style violations

r- for no comments on the migration scenario.

> +
> +    const String& title() const { return m_title; }

Return copy in order not to deal with the ownership.

> -    JSC::StringBuilder message;
> -    message.append("Profile \"webkit-profile://");
> -    message.append((UString)encodeWithURLEscapeSequences(CPUProfileType));
> -    message.append("/");
> -    message.append((UString)encodeWithURLEscapeSequences(profile->title()));
> -    message.append("#");
> -    message.append(UString::from(profile->uid()));
> -    message.append("\" finished.");
> -    addMessageToConsole(JSMessageSource, LogMessageType, LogMessageLevel, message.release(), lineNumber, sourceURL);
> +    String message;
> +    message += "Profile \"webkit-profile://";
> +    message += encodeWithURLEscapeSequences(CPUProfileType);
> +    message += "/";
> +    message += encodeWithURLEscapeSequences(profile->title());
> +    message += "#";
> +    message += String::number(profile->uid());
> +    message += "\" finished.";
> +    addMessageToConsole(JSMessageSource, LogMessageType, LogMessageLevel, message, lineNumber, sourceURL);
>  }
>  

Why not to use String::format?


> -void InspectorController::addStartProfilingMessageToConsole(const UString& title, unsigned lineNumber, const UString& sourceURL)
> +void InspectorController::addStartProfilingMessageToConsole(const String& title, unsigned lineNumber, const String& sourceURL)
>  {
> -    JSC::StringBuilder message;
> -    message.append("Profile \"webkit-profile://");
> -    message.append(encodeWithURLEscapeSequences(CPUProfileType));
> -    message.append("/");
> -    message.append(encodeWithURLEscapeSequences(title));
> -    message.append("#0\" started.");
> -    addMessageToConsole(JSMessageSource, LogMessageType, LogMessageLevel, message.release(), lineNumber, sourceURL);
> +    String message;
> +    message += "Profile \"webkit-profile://";
> +    message += encodeWithURLEscapeSequences(CPUProfileType);
> +    message += "/";
> +    message += encodeWithURLEscapeSequences(title);
> +    message += "#0\" started.";
> +    addMessageToConsole(JSMessageSource, LogMessageType, LogMessageLevel, message, lineNumber, sourceURL);
>  }
>  

ditto

> +#if ENABLE(JAVASCRIPT_DEBUGGER) || USE(V8)
>  

> -#if ENABLE(INSPECTOR)
> +#if ENABLE(INSPECTOR) && USE(JSC)
>      InspectorController* controller = page->inspectorController();
>      // FIXME: log a console message when profiling is disabled.

> -#if ENABLE(INSPECTOR)
> +#if ENABLE(INSPECTOR) && USE(JSC)
>          resolvedTitle = controller->getCurrentUserInitiatedProfileName(true);
>  #else

Could you comment on the fact that this is a temporary measure that will exist until JAVASCRIPT_DEBUGGER is enabled in V8 case?

[Mikhail Naganov]

Created attachment 48003 [details]
comments addressed, fixed style violations found by an updated version of style checking script

There can be an error from style checker concerning absence of `#include "config.h"' in JSBindingsAllInOne.cpp. I'm considering this as a problem in the style checker, not my fault.

[WebKit Commit Bot]

Comment on attachment 48003 [details]
comments addressed, fixed style violations found by an updated version of style checking script

Clearing flags on attachment: 48003

Committed r54277: <http://trac.webkit.org/changeset/54277>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.