Adding NOTREACHED in Read() in Event StructTraits to add clarity to the code.

ui/events/mojo/event_struct_traits.cc

 // Copyright 2016 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 "ui/events/mojo/event_struct_traits.h"
 
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/dom/keycode_converter.h"
 #include "ui/events/mojo/event_constants.mojom.h"
@@ skipping 290 matching lines @@
         }
         case ui::mojom::PointerKind::PEN:
           NOTIMPLEMENTED();
           return false;
       }
     }
     case ui::mojom::EventType::UNKNOWN:
       return false;
   }
 
+  NOTREACHED();

[sadrul, 2017/03/02 00:00:38]

I am not sure if NOTREACHED() is good here. Because this means a sender can
trigger a crash to a remote app by sending it an invalid event, which I think is
something we should avoid.

[mfomitchev, 2017/03/02 00:03:41]

If the event is invalid, it's type will be UNKNOWN, and we will return false,
no?

[sadrul, 2017/03/02 02:22:31]

I think it's possible for the sender to send an event with an undefined action
(i.e. none of the valid ui::mojom::EventTypes)?

[mfomitchev, 2017/03/02 03:05:45]

Hmm.. at this point we have EventDataView object, and not just some random blob
of binary data. EventDataView::action() is returning a value of type
::ui::mojom::EventType, which I think means that it should be one of the valid
EventType values. If we somehow received an invalid serialized value, I think it
would be handled deeper in the guts of Mojo? Here we have a typed value
already..

As you may have guessed, I've put this up merely as a precursor/sanity check for
https://codereview.chromium.org/2728883002/. If we indeed have a case where we
can legitimately fall through to this return, we should probably structure the
code in that CL a little differently.

[Tom Sepez, 2017/03/02 18:54:37]

Doesn't NOTREACHED() only fire in the debug code?  If so, its fine to leave it
as the release build avoids it, and we don't mind crashing in debug.

   return false;
 }
 
 }  // namespace mojo