Blink rename can result in behavior change, unless we are careful around macro stringify operator |
|||
Issue descriptionExamples, where unit tests have caught behavior changes: Source/modules/notifications/NotificationImageLoader.cpp: - NOTIFICATION_PER_TYPE_HISTOGRAM_COUNTS stringifies #type_name - After the rename we need to manually rename back "calls" to the macro to use Image rather than kImage (similarily for Icon, Badge, ActionIcon) and prepend k## inside the macro. Source/core/editing/commands/EditorCommand.cpp: - V macro stringifies #name - After the rename we need to manually rename back "calls" to the macro (in Source/core/editing/commands/EditorCommandNames.h, in FOR_EACH_BLINK_EDITING_COMMAND_NAME macro) to use AlignCenter rather than kAlignCenter (similarily for AlignJustified, AlignLeft, and other ~130 command names) and prepend k## inside the V macro in Source/core/editing/commands/EditorCommand.cpp and in Source/core/editing/commands/EditorCommandTest.cpp. Source/web/WebInputEvent.cpp: - CASE_TYPE macro stringifies #t - After the rename we need to manually rename back "calls" to the macro to use Undefined rather than kUndefined (and similarily for MouseDown, MouseUp, and other ~20 names). We also need to prepend k## inside the CASE_TYPE macro. The changes above can be rolled into https://codereview.chromium.org/2329463004 (see "Apply stashed manual fixes" step in go/blink-rename-checklist).
,
Jan 4 2017
haraken@, could you please help us determine/confirm if it is okay to change the #name string (i.e. from "foo" to "Foo") in third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp: haraken@chromium.org c568952 2014-03-06 22:11:41 15 { \ 16 V8HiddenValue* hiddenValue = V8PerIsolateData::from(isolate)->hiddenValue(); \ 17 if (hiddenValue->m_##name.isEmpty()) { \ 18 HERE -> hiddenValue->m_##name.set(isolate, v8AtomicString(isolate, #name)); \ <- HERE 19 } \ 20 return hiddenValue->m_##name.newLocal(isolate); \ 21 } 22 23 V8_HIDDEN_VALUES(V8_DEFINE_METHOD);
,
Jan 4 2017
We should break these out into sub-bugs. For comment 2, it should be OK - these are used as keys into the hidden values map (see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp?rcl=0&l=29, and https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp?rcl=1483532378&l=36 for an example invocation), so as long as we use a consistent name for setting / retrieving keys, there's no problem.
,
Jan 4 2017
Other things requiring manual intervention after the rename: - Source/core/inspector/InspectorTraceEvents.cpp stringifies #pseudoType. After the rename we need to manually rename back "calls" to the macro to use PseudoFirstChild rather than kPseudoFirstChild (similarily for PseudoUnknown, PseudoEmpty, PseudoLink, etc.) and prepend k## inside the macro where needed. - Source/core/fetch/ResourceFetcher.cpp stringifies #name. After the rename we need to manually rename back "calls" to the macro to use CSSStyleSheet rather than kCSSStyleSheet (similarily for Font, Image, Media, etc.) and prepend k## inside the macro where needed. - Source/platform/heap/ThreadState.cpp stringifies #GCReason. After the rename we need to manually rename back "calls" to the macro to use IdleGC rather than kIdleGC (similarily for PreciseGC, ForcedGC, etc.) and prepend k## inside the macro where needed. Things that seem safe: - Source/wtf/Allocator.h stringifies #type, but this should be okay, because the argument is a type name (which won't be affected by the Big Blink Rename). - Source/core/dom/DocumentLifecycle.cpp stringifies #StateName, but this should be okay, because this is in a DEBUG_STRING_CASE macro - Source/core/html/parser/HTMLTreeBuilder.cpp stringifies #mode, but this should be okay, because this is used in a debug-only method. - Source/core/html/parser/AtomicHTMLToken.cpp stringifies #type, but this should be okay, because this is used in a debug-only method. - Source/modules/screen_orientation/ScreenOrientation.cpp and Source/platform/network/mime/MIMETypeRegistry.cpp stringify #a, but this is only used in a static_assert - Source/platform/graphics/paint/DisplayItem.h stringifies macro args in 2 locations, but resulting strings are only used in static_asserts - Source/platform/graphics/paint/DisplayItem.cpp stringifies macro args in 2 locations, but this should be okay, because this is used in a debug-only method. - Source/platform/cpu/mips/CommonMacrosMSA.h stringifies macro args, but the arguments are asm instruction mnemonics are therefore are not subject to the Big Blink Rename - Source/web/WebFrameWidgetBase.cpp, Source/web/AssertMatchingEnums.cpp and Source/web/WebAssociatedURLLoaderImpl.cpp are okay, because they only use the stringified macro arg in a static_assert
,
Jan 4 2017
#c2: That's fine. Actually I don't really care about naming. (I think it's more important to avoid bike-shedding and finish the big rename asap.)
,
Jan 5 2017
Thanks for the answer. Marking the bug as fixed - the known issues should be covered by https://codereview.chromium.org/2611823003. |
|||
►
Sign in to add a comment |
|||
Comment 1 by lukasza@chromium.org
, Jan 4 2017