Bug 184839

Summary:

Web Inspector: remove some dead code in IdentifiersFactory

Product:

WebKit

Reporter:

Blaze Burg <bburg>

Component:

Web Inspector

Assignee:

Blaze Burg <bburg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, darin, ews-watchlist, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none

Description Blaze Burg 2018-04-20 14:36:46 PDT

This code seems to only have been used by Chromium, and doesn't really make sense given the current architecture.

Comment 1 Blaze Burg 2018-04-20 16:53:52 PDT

Created attachment 338492 [details]
Patch

Comment 2 WebKit Commit Bot 2018-04-20 18:12:49 PDT

Comment on attachment 338492 [details]
Patch

Clearing flags on attachment: 338492

Committed r230870: <https://trac.webkit.org/changeset/230870>

Comment 3 WebKit Commit Bot 2018-04-20 18:12:51 PDT

All reviewed patches have been landed.  Closing bug.

Comment 4 Radar WebKit Bug Importer 2018-04-20 18:13:16 PDT

<rdar://problem/39615471>

Comment 5 Darin Adler 2018-04-30 09:54:23 PDT

Comment on attachment 338492 [details]
Patch

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

> Source/JavaScriptCore/inspector/IdentifiersFactory.cpp:39
> +static String addPrefixToIdentifier(const String& identifier)
> +{
> +    return makeString("0.", identifier);
> +}

If this code was at all performance critical, we should fix it to take an integer rather than a string, since makeString can convert a number to a string in place, avoiding allocating and destroying an additional string.

> Source/JavaScriptCore/inspector/IdentifiersFactory.h:35
>      static String requestId(unsigned long identifier);

The use of "unsigned long" here is peculiar. We almost never use the "long" types in WebKit code rather than the "int" types or types like "uint32_t". unless there is a special reason. What is the reason here?