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.
Created attachment 216254 [details] the patch.
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.
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).
Should we go with "Identifier", then? ("Id" seems wrong according to WebKit style, since it's a shortening like an acronym.)
(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.
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.
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.
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.
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.
Thanks for the review. Landed in r158862: <http://trac.webkit.org/r158862>.