[Android] Presubmit check for banned Notification construction

[Android] Presubmit check for banned Notification construction

- This check searches for use of 'new Notification.Builder'
and 'new NotificationCompat.Builder' in changed lines.

- Those constructors should not be used, new notifications should
use NotificationBuilderFactory.createChromeNotificationBuilder instead.

- This check ensures no new notifications can be introduced without
specifying a channel for use on Android O.

BUG=702628
Review-Url: https://codereview.chromium.org/2894643002
Cr-Commit-Position: refs/heads/master@{#473205}
Committed: https://chromium.googlesource.com/chromium/src/+/2c43ab991daa30205059b6d8a92a3857373061ff

[awdf, 2017-05-18 18:48:00]

awdf@chromium.org changed reviewers:
+ nyquist@chromium.org

[awdf, 2017-05-18 18:48:01]

I promised I would do this a while ago but never got round to it. Better late
than never!

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

lgtm

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/PRESUBMIT.py (right):

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
chrome/android/java/src/PRESUBMIT.py:18: r'new\sNotification(Compat)?\.Builder')
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.

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
chrome/android/java/src/PRESUBMIT.py:36:
'''chrome/android/java/src/org/chromium/chrome/browser/notifications/
Why so many 's?

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

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/PRESUBMIT.py (right):

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
chrome/android/java/src/PRESUBMIT.py:18: r'new\sNotification(Compat)?\.Builder')
On 2017/05/18 20:32:36, nyquist (nychthemeron ping) wrote:
> 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.

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...)

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
chrome/android/java/src/PRESUBMIT.py:36:
'''chrome/android/java/src/org/chromium/chrome/browser/notifications/
On 2017/05/18 20:32:37, nyquist (nychthemeron ping) wrote:
> Why so many 's?

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)

[awdf, 2017-05-19 14:42:09]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-05-19 14:42:09]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2894643002/#ps60001
(title: "remove blank lines for peter")

[commit-bot: I haz the power, 2017-05-19 14:42:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/PRESUBMIT.py (right):

https://codereview.chromium.org/2894643002/diff/20001/chrome/android/java/src...
chrome/android/java/src/PRESUBMIT.py:18: r'new\sNotification(Compat)?\.Builder')
On 2017/05/19 14:16:53, awdf wrote:
> On 2017/05/18 20:32:36, nyquist (nychthemeron ping) wrote:
> > 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.
> 
> 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...)

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.

[commit-bot: I haz the power, 2017-05-19 15:55:06]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495204929100240,
"parent_rev": "c9a40db46670d1c37111c0010d36fc290566eaf0", "commit_rev":
"2c43ab991daa30205059b6d8a92a3857373061ff"}

[commit-bot: I haz the power, 2017-05-19 15:55:15]

Description was changed from

==========
[Android] Presubmit check for banned Notification construction

- This check searches for use of 'new Notification.Builder'
and 'new NotificationCompat.Builder' in changed lines.

- Those constructors should not be used, new notifications should
use NotificationBuilderFactory.createChromeNotificationBuilder instead.

- This check ensures no new notifications can be introduced without
specifying a channel for use on Android O.

BUG=702628
==========

to

==========
[Android] Presubmit check for banned Notification construction

- This check searches for use of 'new Notification.Builder'
and 'new NotificationCompat.Builder' in changed lines.

- Those constructors should not be used, new notifications should
use NotificationBuilderFactory.createChromeNotificationBuilder instead.

- This check ensures no new notifications can be introduced without
specifying a channel for use on Android O.

BUG=702628

Review-Url: https://codereview.chromium.org/2894643002
Cr-Commit-Position: refs/heads/master@{#473205}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c43ab991daa30205059b6d8a92a...
==========

[commit-bot: I haz the power, 2017-05-19 15:55:17]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2c43ab991daa30205059b6d8a92a...