[Android] Presubmit check for banned Notification construction

chrome/android/java/src/PRESUBMIT.py

+# Copyright (c) 2017 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.
+
+"""Presubmit script for Android Java code.
+
+See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
+for more details about the presubmit API built into depot_tools.
+
+This presubmit checks for the following:
+  - No new calls to Notification.Builder or NotificationCompat.Builder
+    constructors. Callers should use ChromeNotificationBuilder instead.
+"""
+
+import re
+
+NEW_NOTIFICATION_BUILDER_RE = re.compile(
+  r'new\sNotification(Compat)?\.Builder')

[nyquist, 2017/05/18 20:32:36]

I don't know if that's even OK, but does the \s here go across multiple lines? I
mean, if you do something weird like:

Object foo =  new
  Notification.Builder

Also, do we need to track comments? I guess we usually don't write the text:
"This will definitely show a new Notification.Builder pattern has been used to
make the API simple.
So... I think what you have is fine.

[awdf, 2017/05/19 14:16:53]

I took out the '\s+'s from the regex when I realised it was pointless because I
only run the regex on one line at a time (Affectedfile.ChangedContents returns
an iterator over lines). 

It is possible to split the banned call over multiple lines (I came up with a
very construed example by indenting about 20 times), if we want to be 100% safe
I could run the regex over the whole file contents instead (maybe using mmap -
http://stackoverflow.com/questions/454456/how-do-i-re-search-or-re-match-on-a...)

[awdf, 2017/05/19 14:48:44]

Dismissing that idea (& the more straightforward one of just joining all the
changed lines together since they're in memory anyway) because any naive
implementation would lose the benefit of printing out the line number. Chance of
anyone splitting the call over multiple lines I reckon is negligible.

+
+def CheckChangeOnUpload(input_api, output_api):
+  return _CommonChecks(input_api, output_api)
+
+
+def CheckChangeOnCommit(input_api, output_api):
+  return _CommonChecks(input_api, output_api)
+
+def _CommonChecks(input_api, output_api):
+  """Checks common to both upload and commit."""
+  result = []
+  result.extend(_CheckNotificationConstructors(input_api, output_api))
+  # Add more checks here
+  return result
+
+def _CheckNotificationConstructors(input_api, output_api):
+  whitelist = [
+      '''chrome/android/java/src/org/chromium/chrome/browser/notifications/

[nyquist, 2017/05/18 20:32:37]

Why so many 's?

[awdf, 2017/05/19 14:16:53]

Ah you caught a bug, thank you! (Triple quotes meant it included the linebreak &
indentation in the path, better to just break the string  with single quotes)

+          NotificationBuilder.java''',
+    '''chrome/android/java/src/org/chromium/chrome/browser/notifications/
+        NotificationCompatBuilder.java'''
+  ]
+  problems = []
+  for f in input_api.AffectedFiles():
+    if f.LocalPath() in whitelist:
+      continue
+    for line_number, line in f.ChangedContents():
+      if NEW_NOTIFICATION_BUILDER_RE.search(line):
+        problems.append(
+          '  %s:%d\n    \t%s' % (f.LocalPath(), line_number, line.strip()))
+  if problems:
+    return [output_api.PresubmitError(
+'''
+Android Notification Construction Check failed:
+  Your new code added one or more calls to the Notification.Builder and/or
+  NotificationCompat.Builder constructors, listed below.
+
+  This is banned, please construct notifications using
+  NotificationBuilderFactory.createChromeNotificationBuilder instead,
+  specifying a channel for use on Android O.
+
+  See https://crbug.com/678670 for more information.
+''',
+      problems)]
+  return []
+
+
+
+