Add an explicit way of making IDs local or global

base/trace_event/trace_event_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/trace_event/trace_event_impl.h"
 
 #include <stddef.h>
 
 #include "base/format_macros.h"
 #include "base/json/string_escape.h"
@@ skipping 340 matching lines @@
     StringAppendF(out, ",\"tts\":%" PRId64, thread_time_int64);
   }
 
   // Output async tts marker field if flag is set.
   if (flags_ & TRACE_EVENT_FLAG_ASYNC_TTS) {
     StringAppendF(out, ", \"use_async_tts\":1");
   }
 
   // If id_ is set, print it out as a hex string so we don't loose any
   // bits (it might be a 64-bit pointer).
-  if (flags_ & TRACE_EVENT_FLAG_HAS_ID) {
+  unsigned int id_flags = flags_ & TRACE_EVENT_FLAG_ID_MASK;
+  if (id_flags != TRACE_EVENT_FLAG_NONE) {
     if (scope_ != trace_event_internal::kGlobalScope)
       StringAppendF(out, ",\"scope\":\"%s\"", scope_);
-    StringAppendF(out, ",\"id\":\"0x%" PRIx64 "\"", static_cast<uint64_t>(id_));
+    std::string id_field_name = "id";
+    if (id_flags == TRACE_EVENT_FLAG_HAS_LOCAL_ID)
+      id_field_name = "local_id";

[Primiano Tucci (use gerrit), 2016/08/31 19:11:22]

instead of changing the field name, which feels quite magical, can we just make
it so that:

the old-style id becomes just id="..."
LOCAL_ID turns into id="local:..."
GLOBAL_ID turns into id="global:..."

so we can use the same pattern consistently for bind_id, without having to
create bind_global_id, bind_local_id etc? I guess that would make the logic in
the importer more reusable (I can imagine some helper LookupObjectById)

The TraceEventFormat[1] doc doesn't seem to mandate that id have to be 0x1234
hex parsable, and they are strings so ... :)

[1]
https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...

[chiniforooshan, 2016/08/31 21:28:45]

Done.

Note that this was originally due to Andrey's suggestion that if we use a
different field here for the new IDs, then old trace consumers will break
explicitly when they read an event with a new ID as opposed to reading the ID
field as usual but handling it incorrectly.

But, since we are going to change the two most important consumers (trace viewer
and devtools) soon, and probably before we actually use these macros, I'm in
favour of the cleaner choice :) (i.e. "id": "local:0x1234").

[chiniforooshan, 2016/09/02 19:37:37]

Nat, Ben, can one of you please tell us your preference about the JSON format
here. The options discussed so far are:

1- ("id":"0x1000", ...), ("id":"0x1000", "id_is_global":false, ...), and
("id":"0x1000", "id_is_global":true, ...)

vs

2- ("id":"0x1000", ...), ("local_id":"0x1000", ...), and ("global_id":"0x1000",
...)

vs

3- ("id":"0x1000", ...), ("id":"local:0x1000", ...), and ("id":"global:0x1000",
...)

Thanks a lot!

[chiniforooshan, 2016/09/09 14:40:37]

+petrcermak maybe?

+    if (id_flags == TRACE_EVENT_FLAG_HAS_GLOBAL_ID)
+      id_field_name = "global_id";
+    StringAppendF(out, ",\"%s\":\"0x%" PRIx64 "\"",
+                  id_field_name.c_str(), static_cast<uint64_t>(id_));
   }
 
   if (flags_ & TRACE_EVENT_FLAG_BIND_TO_ENCLOSING)
     StringAppendF(out, ",\"bp\":\"e\"");
 
   if ((flags_ & TRACE_EVENT_FLAG_FLOW_OUT) ||
       (flags_ & TRACE_EVENT_FLAG_FLOW_IN) ||
       phase_ == TRACE_EVENT_PHASE_BIND_IDS) {
     StringAppendF(out, ",\"bind_id\":\"0x%" PRIx64 "\"",
                   static_cast<uint64_t>(bind_id_));
@@ skipping 43 matching lines @@
         AppendValueAsJSON(arg_types_[i], arg_values_[i], &value_as_text);
 
       *out << value_as_text;
     }
     *out << "}";
   }
 }
 
 }  // namespace trace_event
 }  // namespace base