Change cc::ElementId to be a uint64_t

third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.h

 // Copyright 2015 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.
 
 #ifndef OffscreenCanvas_h
 #define OffscreenCanvas_h
 
 #include <memory>
 #include "bindings/core/v8/ScriptPromise.h"
 #include "core/events/EventTarget.h"
@@ skipping 41 matching lines @@
 
   // API Methods
   ImageBitmap* transferToImageBitmap(ScriptState*, ExceptionState&);
   ScriptPromise convertToBlob(ScriptState*,
                               const ImageEncodeOptions&,
                               ExceptionState&);
 
   const IntSize& Size() const override { return size_; }
   void SetSize(const IntSize&);
 
-  void SetPlaceholderCanvasId(int canvas_id) {
+  void SetPlaceholderCanvasId(unsigned canvas_id) {

[suzyh_UTC10 (ex-contributor), 2017/05/05 04:27:30]

Would it be worth typedef-ing canvas ID as well, to aid in having
self-documenting code?

[chrishtr, 2017/05/05 05:34:52]

Sure - done.

     placeholder_canvas_id_ = canvas_id;
   }
-  int PlaceholderCanvasId() const { return placeholder_canvas_id_; }
+  unsigned PlaceholderCanvasId() const { return placeholder_canvas_id_; }
   bool HasPlaceholderCanvas() {
     return placeholder_canvas_id_ != kNoPlaceholderCanvas;
   }
   bool IsNeutered() const { return is_neutered_; }
   void SetNeutered();
   CanvasRenderingContext* GetCanvasRenderingContext(
       ScriptState*,
       const String&,
       const CanvasContextCreationAttributes&);
   CanvasRenderingContext* RenderingContext() { return context_; }
@@ skipping 84 matching lines @@
   void DoCommit(RefPtr<StaticBitmapImage>, bool is_web_gl_software_rendering);
   using ContextFactoryVector =
       Vector<std::unique_ptr<CanvasRenderingContextFactory>>;
   static ContextFactoryVector& RenderingContextFactories();
   static CanvasRenderingContextFactory* GetRenderingContextFactory(int);
 
   Member<CanvasRenderingContext> context_;
   WeakMember<ExecutionContext> execution_context_;
 
   enum {
-    kNoPlaceholderCanvas = -1,  // DOMNodeIds starts from 0, using -1 to
-                                // indicate no associated canvas element.
+    kNoPlaceholderCanvas = 0,  // DOMNodeIds starts from 1, using 0 to

[chrishtr, 2017/05/05 03:40:11]

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/WeakI...

always returns a value > 0.

[suzyh_UTC10 (ex-contributor), 2017/05/05 04:27:30]

What is the practical result of changing this from -1 to 0? Are there any tests
that need to be changed (or added) to ensure that this change has not got out of
sync with the assumptions baked into any other part of the code?

[chrishtr, 2017/05/05 05:34:52]

I made this change because bit-shifting signed integers is weird.
I think the practical change should be nothing, because
kNoPlaceholderCanvas was just the initialized value of
placeholder_canvas_id_ before being set. So either way the
code is able to detect whether the placeholder_canvas_id was set.
Re tests changed: I guess not, since they seem to pass.
Re added: I think existing tests should exercise the code,
as well as the DCHECK in DomNodeIds. I'll wait for feedback from
junov@ on this also.

+                               // indicate no associated canvas element.
   };
-  int placeholder_canvas_id_ = kNoPlaceholderCanvas;
+  unsigned placeholder_canvas_id_ = kNoPlaceholderCanvas;
 
   IntSize size_;
   bool is_neutered_ = false;
 
   bool origin_clean_ = true;
   bool disable_reading_from_canvas_ = false;
 
   bool IsPaintable() const;
 
   std::unique_ptr<OffscreenCanvasFrameDispatcher> frame_dispatcher_;
@@ skipping 10 matching lines @@
   // If this object is not created via
   // HTMLCanvasElement.transferControlToOffscreen(),
   // then the following members would remain as initialized zero values.
   uint32_t client_id_ = 0;
   uint32_t sink_id_ = 0;
 };
 
 }  // namespace blink
 
 #endif  // OffscreenCanvas_h