Enable '-Wimplicit-fallthrough' and fix issues

Change-Id: I9e1e09f3924dcd8f59af2fbc952456b81e2d7c4e

packager/app/stream_descriptor.h

@@ -12,7 +12,7 @@
 #include <set>
 #include <string>
 
-#include "media/base/container_names.h"
+#include "packager/media/base/container_names.h"
 
 namespace edash_packager {
 namespace media {

packager/app/widevine_encryption_flags.cc

@@ -10,6 +10,7 @@
 
 #include "packager/app/validate_flag.h"
 #include "packager/base/logging.h"
+#include "packager/base/strings/string_piece.h"
 #include "packager/base/strings/string_util.h"
 
 DEFINE_bool(enable_widevine_encryption,
@@ -76,7 +77,7 @@ bool ValidateWidevineCryptoFlags() {
     success = false;
   }
   if (widevine_crypto && FLAGS_signer.empty() &&
-      base::StartsWith(FLAGS_key_server_url, "http",
+      base::StartsWith(base::StringPiece(FLAGS_key_server_url), "http",
                        base::CompareCase::INSENSITIVE_ASCII)) {
     LOG(WARNING) << "--signer is likely required with "
                     "--enable_widevine_encryption/decryption.";

packager/common.gypi

@@ -19,8 +19,7 @@
     'conditions': [
       ['clang==1', {
         'cflags': [
-          # Temporary workaround a gtest bug on ImplicitCast_.
+          '-Wimplicit-fallthrough',
-          '-Wno-pessimizing-move',
         ],
         # Revert the relevant settings in Chromium's common.gypi.
         'cflags!': [

packager/media/base/http_key_fetcher.cc

@@ -7,6 +7,8 @@
 #include "packager/media/base/http_key_fetcher.h"
 
 #include <curl/curl.h>
+
+#include "packager/base/logging.h"
 #include "packager/base/strings/stringprintf.h"
 #include "packager/base/synchronization/lock.h"
 

packager/media/base/macros.h

@@ -0,0 +1,54 @@
+// Copyright 2016 Google Inc. All rights reserved.
+//
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file or at
+// https://developers.google.com/open-source/licenses/bsd
+
+#ifndef MEDIA_BASE_MACROS_H_
+#define MEDIA_BASE_MACROS_H_
+
+// The FALLTHROUGH_INTENDED macro can be used to annotate implicit fall-through
+// between switch labels:
+//  switch (x) {
+//    case 40:
+//    case 41:
+//      if (truth_is_out_there) {
+//        ++x;
+//        FALLTHROUGH_INTENDED;  // Use instead of/along with annotations in
+//                               // comments.
+//      } else {
+//        return x;
+//      }
+//    case 42:
+//      ...
+//
+//  As shown in the example above, the FALLTHROUGH_INTENDED macro should be
+//  followed by a semicolon. It is designed to mimic control-flow statements
+//  like 'break;', so it can be placed in most places where 'break;' can, but
+//  only if there are no statements on the execution path between it and the
+//  next switch label.
+//
+//  When compiled with clang in C++11 mode, the FALLTHROUGH_INTENDED macro is
+//  expanded to [[clang::fallthrough]] attribute, which is analysed when
+//  performing switch labels fall-through diagnostic ('-Wimplicit-fallthrough').
+//  See clang documentation on language extensions for details:
+//  http://clang.llvm.org/docs/AttributeReference.html#fallthrough-clang-fallthrough
+//
+//  When used with unsupported compilers, the FALLTHROUGH_INTENDED macro has no
+//  effect on diagnostics.
+//
+//  In either case this macro has no effect on runtime behavior and performance
+//  of code.
+#if defined(__clang__) && __cplusplus >= 201103L && defined(__has_warning)
+#if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")
+#define FALLTHROUGH_INTENDED [[clang::fallthrough]]  // NOLINT
+#endif
+#endif
+
+#ifndef FALLTHROUGH_INTENDED
+#define FALLTHROUGH_INTENDED \
+  do {                       \
+  } while (0)
+#endif
+
+#endif  // MEDIA_BASE_MACROS_H_

packager/media/base/media_base.gyp

@@ -43,6 +43,7 @@
         'key_source.cc',
         'key_source.h',
         'limits.h',
+        'macros.h',
         'media_parser.h',
         'media_sample.cc',
         'media_sample.h',

packager/media/formats/mp4/mp4_media_parser.cc

@@ -16,6 +16,7 @@
 #include "packager/media/base/buffer_reader.h"
 #include "packager/media/base/decrypt_config.h"
 #include "packager/media/base/key_source.h"
+#include "packager/media/base/macros.h"
 #include "packager/media/base/media_sample.h"
 #include "packager/media/base/video_stream_info.h"
 #include "packager/media/file/file.h"
@@ -393,10 +394,15 @@ bool MP4MediaParser::ParseMoov(BoxReader* reader) {
                        << actual_format << " in stsd box.";
             return false;
           }
+          break;
         case FOURCC_DTSC:
+          FALLTHROUGH_INTENDED;
         case FOURCC_DTSH:
+          FALLTHROUGH_INTENDED;
         case FOURCC_DTSL:
+          FALLTHROUGH_INTENDED;
         case FOURCC_DTSE:
+          FALLTHROUGH_INTENDED;
         case FOURCC_DTSM:
           extra_data = entry.ddts.extra_data;
           max_bitrate = entry.ddts.max_bitrate;

packager/media/formats/webvtt/webvtt_media_parser.cc

@@ -13,6 +13,7 @@
 #include "packager/base/strings/string_number_conversions.h"
 #include "packager/base/strings/string_split.h"
 #include "packager/base/strings/string_util.h"
+#include "packager/media/base/macros.h"
 #include "packager/media/base/media_sample.h"
 #include "packager/media/base/text_stream_info.h"
 
@@ -67,6 +68,7 @@ bool ReadLine(std::string* data, std::string* line) {
 
 bool TimestampToMilliseconds(const std::string& original_str,
                              uint64_t* time_ms) {
+  const size_t kMinimalHoursLength = 2;
   const size_t kMinutesLength = 2;
   const size_t kSecondsLength = 2;
   const size_t kMillisecondsLength = 3;
@@ -90,6 +92,8 @@ bool TimestampToMilliseconds(const std::string& original_str,
     // Check if hours is in the right format, if so get the number.
     // -1 for excluding colon for splitting hours and minutes.
     const size_t hours_length = str.size() - kMinimalLength - 1;
+    if (hours_length < kMinimalHoursLength)
+      return false;
     if (!base::StringToInt(str.substr(0, hours_length), &hours))
       return false;
     str_index += hours_length;
@@ -333,6 +337,7 @@ bool WebVttMediaParser::Parse(const uint8_t* buf, int size) {
         // timing, so fall thru. Setting state_ to kCueTiming so that the state
         // always matches the case.
         state_ = kCueTiming;
+        FALLTHROUGH_INTENDED;
       }
       case kCueTiming: {
         DCHECK(has_arrow);

packager/media/formats/webvtt/webvtt_media_parser_unittest.cc

@@ -146,7 +146,7 @@ TEST_F(WebVttMediaParserTest, MalformedHourTimestamp) {
 TEST_F(WebVttMediaParserTest, SpacesInTimestamp) {
   EXPECT_CALL(new_sample_callback_, Call(_, _)).Times(0);
 
-  const char kHourStringTooShort[] =
+  const char kSpacesInTimestamp[] =
       "WEBVTT\n"
       "\n"
       "0:01: 1.004 --> 0 :04:25.092\n"
@@ -154,8 +154,8 @@ TEST_F(WebVttMediaParserTest, SpacesInTimestamp) {
   InitializeParser();
 
   EXPECT_FALSE(
-      parser_.Parse(reinterpret_cast<const uint8_t*>(kHourStringTooShort),
+      parser_.Parse(reinterpret_cast<const uint8_t*>(kSpacesInTimestamp),
-                    arraysize(kHourStringTooShort) - 1));
+                    arraysize(kSpacesInTimestamp) - 1));
 }
 
 MATCHER_P(MatchesPayload, data, "") {

packager/packager.gyp

@@ -89,20 +89,6 @@
         'testing/gtest.gyp:gtest',
       ],
     },
-    {
-      'target_name': 'All',
-      'type': 'none',
-      'dependencies': [
-        'media/base/media_base.gyp:*',
-        'media/event/media_event.gyp:*',
-        'media/file/file.gyp:*',
-        'media/formats/mp2t/mp2t.gyp:*',
-        'media/formats/mp4/mp4.gyp:*',
-        'media/formats/webm/webm.gyp:*',
-        'media/formats/wvm/wvm.gyp:*',
-        'mpd/mpd.gyp:*',
-      ],
-    },
     {
       'target_name': 'packager_builder_tests',
       'type': 'none',
@@ -114,6 +100,7 @@
         'media/formats/mp2t/mp2t.gyp:mp2t_unittest',
         'media/formats/mp4/mp4.gyp:mp4_unittest',
         'media/formats/webm/webm.gyp:webm_unittest',
+        'media/formats/webvtt/webvtt.gyp:webvtt_unittest',
         'media/formats/wvm/wvm.gyp:wvm_unittest',
         'mpd/mpd.gyp:mpd_unittest',
         'packager_test',