[webkit-reviews] review denied: [Bug 34481] Web Inspector: Start unforking profiler and debugger stuff : [Attachment 47941] proposed patch - fixed style violations

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Mikhail Naganov
<mnaganov at chromium.org>'s request for review:
Bug 34481: Web Inspector: Start unforking profiler and debugger stuff
https://bugs.webkit.org/show_bug.cgi?id=34481

Attachment 47941: proposed patch - fixed style violations
https://bugs.webkit.org/attachment.cgi?id=47941&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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?