Add presubmit check for correct MessageLoopProxy usage in src/media/.

Add presubmit check for correct MessageLoopProxy usage in src/media/.

In most cases SingleThreadTaskRunner can be used instead.

BUG=315922
R=dalecurtis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243408

[scherkus (not reviewing), 2014-01-07 19:32:19]

dalecurtis: can you review? you know python 'n stuff

[DaleCurtis, 2014-01-07 20:00:42]

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py
File media/PRESUBMIT.py (left):

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py#oldcode72
media/PRESUBMIT.py:72: 
Python style is two lines between global methods.

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py#newcode67
media/PRESUBMIT.py:67: """Make sure media code only uses MessageLoopProxy for
accessing the current
I'm not sure what you mean here.  Can you elaborate?  There's only a single
static on MessageLoopProxy... current() -- What else do you expect this to
catch?

[scherkus (not reviewing), 2014-01-07 20:06:09]

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py
File media/PRESUBMIT.py (left):

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py#oldcode72
media/PRESUBMIT.py:72: 
On 2014/01/07 20:00:42, DaleCurtis wrote:
> Python style is two lines between global methods.

whoops! done

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py#newcode67
media/PRESUBMIT.py:67: """Make sure media code only uses MessageLoopProxy for
accessing the current
On 2014/01/07 20:00:42, DaleCurtis wrote:
> I'm not sure what you mean here.  Can you elaborate?  There's only a single
> static on MessageLoopProxy... current() -- What else do you expect this to
> catch?

we want to catch any use of MessageLoopProxy that isn't
MessageLoopProxy::current()

For example:

void ShouldBeUsingTaskRunner(const scoped_refptr<MessageLoopProxy>& loop);

class Blah {
  scoped_refptr<MessageLoopProxy> should_be_a_task_runner_;
};

[DaleCurtis, 2014-01-07 20:13:46]

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py#newcode67
media/PRESUBMIT.py:67: """Make sure media code only uses MessageLoopProxy for
accessing the current
On 2014/01/07 20:06:10, scherkus wrote:
> On 2014/01/07 20:00:42, DaleCurtis wrote:
> > I'm not sure what you mean here.  Can you elaborate?  There's only a single
> > static on MessageLoopProxy... current() -- What else do you expect this to
> > catch?
> 
> we want to catch any use of MessageLoopProxy that isn't
> MessageLoopProxy::current()
> 
> For example:
> 
> void ShouldBeUsingTaskRunner(const scoped_refptr<MessageLoopProxy>& loop);
> 
> class Blah {
>   scoped_refptr<MessageLoopProxy> should_be_a_task_runner_;
> };

Are you expecting SingleThreadTaskRunner or TaskRunner?  Either way, I think you
need a more elaborate error message when these cases are detected so users w/
errors have an idea of what to do to fix their problem.

https://codereview.chromium.org/101503007/diff/60001/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/60001/media/PRESUBMIT.py#newco...
media/PRESUBMIT.py:75: contents = input_api.ReadFile(f, 'rb')
Instead of reading the file you should only need to check f.ChangedContents() as
is done on _CheckForUseOfWrongClock().

[scherkus (not reviewing), 2014-01-07 21:40:22]

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/1/media/PRESUBMIT.py#newcode67
media/PRESUBMIT.py:67: """Make sure media code only uses MessageLoopProxy for
accessing the current
On 2014/01/07 20:13:46, DaleCurtis wrote:
> On 2014/01/07 20:06:10, scherkus wrote:
> > On 2014/01/07 20:00:42, DaleCurtis wrote:
> > > I'm not sure what you mean here.  Can you elaborate?  There's only a
single
> > > static on MessageLoopProxy... current() -- What else do you expect this to
> > > catch?
> > 
> > we want to catch any use of MessageLoopProxy that isn't
> > MessageLoopProxy::current()
> > 
> > For example:
> > 
> > void ShouldBeUsingTaskRunner(const scoped_refptr<MessageLoopProxy>& loop);
> > 
> > class Blah {
> >   scoped_refptr<MessageLoopProxy> should_be_a_task_runner_;
> > };
> 
> Are you expecting SingleThreadTaskRunner or TaskRunner?  Either way, I think
you
> need a more elaborate error message when these cases are detected so users w/
> errors have an idea of what to do to fix their problem.

I'm expecting anything other than MLP, but in most cases STTR.

I reworked the message -- let me know what you think.

https://codereview.chromium.org/101503007/diff/60001/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/60001/media/PRESUBMIT.py#newco...
media/PRESUBMIT.py:75: contents = input_api.ReadFile(f, 'rb')
On 2014/01/07 20:13:46, DaleCurtis wrote:
> Instead of reading the file you should only need to check f.ChangedContents()
as
> is done on _CheckForUseOfWrongClock().

Done.

(I initially had it read the whole file to catch instances that weren't included
in the diff, but it looks like my existing CL to fix all instances should do the
trick)

[DaleCurtis, 2014-01-07 21:57:18]

https://codereview.chromium.org/101503007/diff/110001/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/110001/media/PRESUBMIT.py#newc...
media/PRESUBMIT.py:71: message_loop_proxy_re =
r'\bMessageLoopProxy(::current\(\))?'
Use the re.compile syntax like above so you're not recomputing the regex for
every line.  Also instead of checking the lastindex below you can rewrite the
regex w/ a negative lookahead so it matches MessageLoopProxy iff it doesn't
follow with current()

r'\bMessageLoopProxy(?!::current\(\))'

https://codereview.chromium.org/101503007/diff/110001/media/PRESUBMIT.py#newc...
media/PRESUBMIT.py:76: for match in input_api.re.finditer(message_loop_proxy_re,
line):
Since you're breaking after the first match, just use "if not re.search()" (w/
the above regex mod) like the above method does. You can then avoid any
continue/break semantics as well as extra scanning per line.

[scherkus (not reviewing), 2014-01-07 22:05:59]

https://codereview.chromium.org/101503007/diff/110001/media/PRESUBMIT.py
File media/PRESUBMIT.py (right):

https://codereview.chromium.org/101503007/diff/110001/media/PRESUBMIT.py#newc...
media/PRESUBMIT.py:71: message_loop_proxy_re =
r'\bMessageLoopProxy(::current\(\))?'
On 2014/01/07 21:57:19, DaleCurtis wrote:
> Use the re.compile syntax like above so you're not recomputing the regex for
> every line.  Also instead of checking the lastindex below you can rewrite the
> regex w/ a negative lookahead so it matches MessageLoopProxy iff it doesn't
> follow with current()
> 
> r'\bMessageLoopProxy(?!::current\(\))'

Done. Had a feeling there was a way to do this!

https://codereview.chromium.org/101503007/diff/110001/media/PRESUBMIT.py#newc...
media/PRESUBMIT.py:76: for match in input_api.re.finditer(message_loop_proxy_re,
line):
On 2014/01/07 21:57:19, DaleCurtis wrote:
> Since you're breaking after the first match, just use "if not re.search()" (w/
> the above regex mod) like the above method does. You can then avoid any
> continue/break semantics as well as extra scanning per line.

Done.

[DaleCurtis, 2014-01-07 22:12:54]

lgtm

[DaleCurtis, 2014-01-07 22:14:52]

FYI, you can append NOTRY=true to avoid trybots on this change.

[scherkus (not reviewing), 2014-01-07 22:26:48]

Committed patchset #4 manually as r243408 (presubmit successful).