New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 678294 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

Blink rename can result in behavior change, unless we are careful around macro stringify operator

Project Member Reported by lukasza@chromium.org, Jan 4 2017

Issue description

Examples, 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).

 
I had trouble finding all cases of macro argument stringification in Blink code base.  I've ended up using the command below (taking care to not match on #include or #ifdef, to ignore URLs or CSS and test code):

find third_party/WebKit/Source -name '*.cpp' -or -name '*.h' | xargs grep '^[^#].*[^#]#[^#0-9]' | grep -v 'https://\|http://\|about:\|querySelector\|\(//.*#\)' | grep -v Test.*cpp:

This gives around 100 hits that I plan to inspect manually.
Labels: -Pri-3 Pri-2
Owner: haraken@chromium.org
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);
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.
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
#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.)


Cc: haraken@chromium.org
Owner: ----
Status: Fixed (was: Available)
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