Bug 123945 – Cosmetic: rename xxxId to xxxID for ScriptId, SourceId, and BreakpointId

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mark Lam]

Currently, these types and variable sometimes use "Id" in their names, and "ID" at other times.  I'm changing these for Inspector and Debugger related files for consistency.  This will also make it easier to review changes for the patch for https://bugs.webkit.org/show_bug.cgi?id=121796 later.

There is one exception: the Inspector protocol (.json files) and JS code uses "breakpointId" and "scriptId".  For backward compatibility, we should not change the .json protocol files.  So, as a rule, I left these .json and .js files alone, along with their relevant C++ files.

This change is purely cosmetic, and has been run on the layout tests without any regression.  Patch coming soon.

[Mark Lam]

Created attachment 216254 [details]
the patch.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp', u'Source/WebCore/bindings/js/JavaScriptCallFrame.h', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp', u'Source/WebCore/bindings/js/ScriptDebugServer.h', u'Source/WebCore/inspector/InspectorConsoleAgent.cpp', u'Source/WebCore/inspector/InspectorConsoleAgent.h', u'Source/WebCore/inspector/InspectorConsoleInstrumentation.h', u'Source/WebCore/inspector/InspectorDOMAgent.cpp', u'Source/WebCore/inspector/InspectorDebuggerAgent.cpp', u'Source/WebCore/inspector/InspectorDebuggerAgent.h', u'Source/WebCore/inspector/InspectorInstrumentation.cpp', u'Source/WebCore/inspector/InspectorInstrumentation.h', u'Source/WebCore/inspector/ScriptDebugListener.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebScriptDebugger.mm']" exit_code: 1
Source/WebCore/inspector/InspectorDebuggerAgent.cpp:305:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 19 files


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

[Timothy Hatcher]

Comment on attachment 216254 [details]
the patch.

I don't really like "ID", it isn't any better than "Id". Maybe even worse. I would prefer spelling out "Identifier" (WebKit style is to not abbreviate).

[Geoffrey Garen]

Should we go with "Identifier", then? ("Id" seems wrong according to WebKit style, since it's a shortening like an acronym.)

[Timothy Hatcher]

(In reply to comment #4)
> Should we go with "Identifier", then? ("Id" seems wrong according to WebKit style, since it's a shortening like an acronym.)

I agree, "Id" is wrong too.

[Mark Lam]

OK, we have an agreement that "Id" is not the right choice.  However, there are benefits to go with "ID" as a pseudo-acronym instead of "Identifier".  These benefits are:

1. "ID" is short.  This makes for less verbose code which is easier to read.  This is valuable unless it conflicts with 2 below ...

2. It should be easy to understand.  For the cases of ScriptID, SourceID, and BreakpointID, I believe the meaning is clear and anyone who works with this area of the code would commonly interpret "ID" here to mean "Identifier" without needing it to be spelled out.

3. On a more subjective level, in VM / parser / debugger type code, "Identiier" is often associated with a string / symbol value, whereas "ID" is often associated with a numeric cookie / token.  My intent is for SourceID and BreakpointID to become numeric tokens in the patch for https://bugs.webkit.org/show_bug.cgi?id=121796, and this current patch is meant to minimize the cosmetic changes ahead of time.

Per our offline discussion, we'll move forward with "ID".  That said, I do see areas in this patch where I used "ID" where the value is intended to be a string.  I'll upload another patch so that it adheres to reason 3 above.

[Mark Lam]

Created attachment 216317 [details]
patch 2: changed some IDs to Identifiers in InspectorDebuggerAgent because those are meant to be strings.

There are some places where ScriptID or SourceID is a string.  I chose to keep those as "ID"s because they ultimately hold a numeric SourceID value, but just happened to be encoded in a string.  In contrast, the InspectorDebuggerAgent actually has a different representation of breakpoint ids which are string names.  I renamed the "ID"s here to "Identifier".

As a reminder, this patch is meant to reduce the cosmetic changes that will come with https://bugs.webkit.org/show_bug.cgi?id=121796.  With https://bugs.webkit.org/show_bug.cgi?id=121796, all the SourceIDs and BreakpointIDs in JSC and in the ScriptDebugServer will be numeric.  This current patch makes use of that knowledge in choosing whether to use "ID" or "Identifier" in the names.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp', u'Source/WebCore/bindings/js/JavaScriptCallFrame.h', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp', u'Source/WebCore/bindings/js/ScriptDebugServer.h', u'Source/WebCore/inspector/InspectorConsoleAgent.cpp', u'Source/WebCore/inspector/InspectorConsoleAgent.h', u'Source/WebCore/inspector/InspectorConsoleInstrumentation.h', u'Source/WebCore/inspector/InspectorDOMAgent.cpp', u'Source/WebCore/inspector/InspectorDebuggerAgent.cpp', u'Source/WebCore/inspector/InspectorDebuggerAgent.h', u'Source/WebCore/inspector/InspectorInstrumentation.cpp', u'Source/WebCore/inspector/InspectorInstrumentation.h', u'Source/WebCore/inspector/ScriptDebugListener.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebScriptDebugger.mm']" exit_code: 1
Source/WebCore/inspector/InspectorDebuggerAgent.cpp:305:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 19 files


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

[Geoffrey Garen]

Comment on attachment 216317 [details]
patch 2: changed some IDs to Identifiers in InspectorDebuggerAgent because those are meant to be strings.

Looks like Tim signed off on this, so r=me.

[Mark Lam]

Thanks for the review.  Landed in r158862: <http://trac.webkit.org/r158862>.