Add leakcanary to DEPS and third_party

Add leakcanary to DEPS and third_party

BUG=505909
Committed: https://crrev.com/a1b62008719deafac9384edcada8743464658c54
Cr-Commit-Position: refs/heads/master@{#377201}

[agrieve, 2016-02-12 01:57:24]

Description was changed from

==========
Add leakcanary to DEPS and third_party

BUG=505909
==========

to

==========
Add leakcanary to DEPS and third_party

BUG=505909
==========

[agrieve, 2016-02-12 01:57:24]

agrieve@chromium.org changed reviewers:
+ klobag@chromium.org, yfriedman@chromium.org

[agrieve, 2016-02-12 01:59:18]

On 2016/02/12 01:57:24, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:klobag@chromium.org, mailto:yfriedman@chromium.org

🕊🕊🕊

Yaron - plz review
Grace - plz give third_party stamp

[agrieve, 2016-02-12 02:30:48]

On 2016/02/12 01:59:18, agrieve wrote:
> On 2016/02/12 01:57:24, agrieve wrote:
> > mailto:agrieve@chromium.org changed reviewers:
> > + mailto:klobag@chromium.org, mailto:yfriedman@chromium.org
> 
> 🕊🕊🕊
> 
> Yaron - plz review
> Grace - plz give third_party stamp

For reference, here's the review that hooks it up:
https://codereview.chromium.org/1691993003/

[Yaron, 2016-02-12 02:50:27]

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
File third_party/leakcanary/BUILD.gn (right):

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
third_party/leakcanary/BUILD.gn:56: # This is required to remove:
awkward. So what happens with gradle? It uses aapt a different way or not at
all?

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/leak...
File third_party/leakcanary/leakcanary.gyp (right):

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/leak...
third_party/leakcanary/leakcanary.gyp:8: # Leak Canary added to GN only.
Why include this at all?

[Yaron, 2016-02-12 02:56:12]

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/leak...
File third_party/leakcanary/leakcanary.gyp (right):

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/leak...
third_party/leakcanary/leakcanary.gyp:8: # Leak Canary added to GN only.
On 2016/02/12 02:50:27, Yaron wrote:
> Why include this at all?

nvm - i see why. it's so the java dep works from CBI

[agrieve, 2016-02-12 19:30:23]

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
File third_party/leakcanary/BUILD.gn (right):

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
third_party/leakcanary/BUILD.gn:56: # This is required to remove:
On 2016/02/12 02:50:27, Yaron wrote:
> awkward. So what happens with gradle? It uses aapt a different way or not at
> all?

Awkward is how I interpret this as well. From Googling around, public.xml files
used to just be for libraries that distributed as binary to have stable IDs.
This is how aapt interprets <public>, and it dies if no id="" attribute exists.

There's a flag for aapt to generated public.xml files, but I couldn't find a
flag to have it ignore them.

I think this new form of public.xml (without ids) was introduced for .aar
libraries, and is mostly just a hint to AndroidStudio to not auto-complete
resource IDs from dependencies.

[Yaron, 2016-02-12 19:51:49]

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
File third_party/leakcanary/BUILD.gn (right):

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
third_party/leakcanary/BUILD.gn:56: # This is required to remove:
On 2016/02/12 19:30:23, agrieve wrote:
> On 2016/02/12 02:50:27, Yaron wrote:
> > awkward. So what happens with gradle? It uses aapt a different way or not at
> > all?
> 
> Awkward is how I interpret this as well. From Googling around, public.xml
files
> used to just be for libraries that distributed as binary to have stable IDs.
> This is how aapt interprets <public>, and it dies if no id="" attribute
exists.
> 
> There's a flag for aapt to generated public.xml files, but I couldn't find a
> flag to have it ignore them.
> 
> I think this new form of public.xml (without ids) was introduced for .aar
> libraries, and is mostly just a hint to AndroidStudio to not auto-complete
> resource IDs from dependencies. 

Can you update the README.chromium to mention verifying this again. I assume
that the next you roll leakcanary it's gonna fail due to them adding new
resources. In fact, perhaps this is better as a blacklist? Of course you'd have
to glob or walk the directory which probably isn't ideal but would make it
easier to roll in the future.

[agrieve, 2016-02-12 20:24:53]

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
File third_party/leakcanary/BUILD.gn (right):

https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
third_party/leakcanary/BUILD.gn:56: # This is required to remove:
On 2016/02/12 19:51:49, Yaron wrote:
> On 2016/02/12 19:30:23, agrieve wrote:
> > On 2016/02/12 02:50:27, Yaron wrote:
> > > awkward. So what happens with gradle? It uses aapt a different way or not
at
> > > all?
> > 
> > Awkward is how I interpret this as well. From Googling around, public.xml
> files
> > used to just be for libraries that distributed as binary to have stable IDs.
> > This is how aapt interprets <public>, and it dies if no id="" attribute
> exists.
> > 
> > There's a flag for aapt to generated public.xml files, but I couldn't find a
> > flag to have it ignore them.
> > 
> > I think this new form of public.xml (without ids) was introduced for .aar
> > libraries, and is mostly just a hint to AndroidStudio to not auto-complete
> > resource IDs from dependencies. 
> 
> Can you update the README.chromium to mention verifying this again. I assume
> that the next you roll leakcanary it's gonna fail due to them adding new
> resources. In fact, perhaps this is better as a blacklist? Of course you'd
have
> to glob or walk the directory which probably isn't ideal but would make it
> easier to roll in the future.

Added a note saying what needs updating when rolling.

[Yaron, 2016-02-12 21:23:21]

On 2016/02/12 20:24:53, agrieve wrote:
>
https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
> File third_party/leakcanary/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
> third_party/leakcanary/BUILD.gn:56: # This is required to remove:
> On 2016/02/12 19:51:49, Yaron wrote:
> > On 2016/02/12 19:30:23, agrieve wrote:
> > > On 2016/02/12 02:50:27, Yaron wrote:
> > > > awkward. So what happens with gradle? It uses aapt a different way or
not
> at
> > > > all?
> > > 
> > > Awkward is how I interpret this as well. From Googling around, public.xml
> > files
> > > used to just be for libraries that distributed as binary to have stable
IDs.
> > > This is how aapt interprets <public>, and it dies if no id="" attribute
> > exists.
> > > 
> > > There's a flag for aapt to generated public.xml files, but I couldn't find
a
> > > flag to have it ignore them.
> > > 
> > > I think this new form of public.xml (without ids) was introduced for .aar
> > > libraries, and is mostly just a hint to AndroidStudio to not auto-complete
> > > resource IDs from dependencies. 
> > 
> > Can you update the README.chromium to mention verifying this again. I assume
> > that the next you roll leakcanary it's gonna fail due to them adding new
> > resources. In fact, perhaps this is better as a blacklist? Of course you'd
> have
> > to glob or walk the directory which probably isn't ideal but would make it
> > easier to roll in the future.
> 
> Added a note saying what needs updating when rolling.

lgtm - fyi Grace is out until Thursday-ish (i think)

[agrieve, 2016-02-12 21:24:23]

agrieve@chromium.org changed reviewers:
+ thakis@chromium.org
- klobag@chromium.org

[agrieve, 2016-02-12 21:24:38]

On 2016/02/12 21:23:21, Yaron wrote:
> On 2016/02/12 20:24:53, agrieve wrote:
> >
>
https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
> > File third_party/leakcanary/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1693463004/diff/1/third_party/leakcanary/BUIL...
> > third_party/leakcanary/BUILD.gn:56: # This is required to remove:
> > On 2016/02/12 19:51:49, Yaron wrote:
> > > On 2016/02/12 19:30:23, agrieve wrote:
> > > > On 2016/02/12 02:50:27, Yaron wrote:
> > > > > awkward. So what happens with gradle? It uses aapt a different way or
> not
> > at
> > > > > all?
> > > > 
> > > > Awkward is how I interpret this as well. From Googling around,
public.xml
> > > files
> > > > used to just be for libraries that distributed as binary to have stable
> IDs.
> > > > This is how aapt interprets <public>, and it dies if no id="" attribute
> > > exists.
> > > > 
> > > > There's a flag for aapt to generated public.xml files, but I couldn't
find
> a
> > > > flag to have it ignore them.
> > > > 
> > > > I think this new form of public.xml (without ids) was introduced for
.aar
> > > > libraries, and is mostly just a hint to AndroidStudio to not
auto-complete
> > > > resource IDs from dependencies. 
> > > 
> > > Can you update the README.chromium to mention verifying this again. I
assume
> > > that the next you roll leakcanary it's gonna fail due to them adding new
> > > resources. In fact, perhaps this is better as a blacklist? Of course you'd
> > have
> > > to glob or walk the directory which probably isn't ideal but would make it
> > > easier to roll in the future.
> > 
> > Added a note saying what needs updating when rolling.
> 
> lgtm - fyi Grace is out until Thursday-ish (i think)

Doh! Thanks for the heads up. +thakis

[Nico, 2016-02-12 21:26:23]

https://codereview.chromium.org/1693463004/diff/20001/third_party/leakcanary/...
File third_party/leakcanary/README.chromium (right):

https://codereview.chromium.org/1693463004/diff/20001/third_party/leakcanary/...
third_party/leakcanary/README.chromium:8: 
did you do the stuff in
https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review
?

[agrieve, 2016-02-12 21:28:56]

On 2016/02/12 21:26:23, Nico wrote:
>
https://codereview.chromium.org/1693463004/diff/20001/third_party/leakcanary/...
> File third_party/leakcanary/README.chromium (right):
> 
>
https://codereview.chromium.org/1693463004/diff/20001/third_party/leakcanary/...
> third_party/leakcanary/README.chromium:8: 
> did you do the stuff in
>
https://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Get-a-Review
> ?

I believe so (although ~6 months ago). Just forwarded you the chrome-eng-review
thread for it.

[Nico, 2016-02-12 21:30:26]

license review is what i'm most curious about...but "apache" + "i believe so" is
good enough, so lgtm if you're fairly confident you emailed
open-source-third-party-reviews@google.com

[agrieve, 2016-02-12 21:33:27]

On 2016/02/12 21:30:26, Nico wrote:
> license review is what i'm most curious about...but "apache" + "i believe so"
is
> good enough, so lgtm if you're fairly confident you emailed
> mailto:open-source-third-party-reviews@google.com

Alright, thanks for being explicit here. Looks like I did so for "haha", but not
for this one. Will do so before submitting.

[agrieve, 2016-02-22 20:42:33]

agrieve@chromium.org changed reviewers:
+ dannyb@google.com

[agrieve, 2016-02-22 20:42:48]

On 2016/02/12 21:33:27, agrieve wrote:
> On 2016/02/12 21:30:26, Nico wrote:
> > license review is what i'm most curious about...but "apache" + "i believe
so"
> is
> > good enough, so lgtm if you're fairly confident you emailed
> > mailto:open-source-third-party-reviews@google.com
> 
> Alright, thanks for being explicit here. Looks like I did so for "haha", but
not
> for this one. Will do so before submitting.

+dannyb for OSTPR

[agrieve, 2016-02-24 01:08:22]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-02-24 01:12:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693463004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693463004/20001

[commit-bot: I haz the power, 2016-02-24 03:08:41]

Description was changed from

==========
Add leakcanary to DEPS and third_party

BUG=505909
==========

to

==========
Add leakcanary to DEPS and third_party

BUG=505909
==========

[commit-bot: I haz the power, 2016-02-24 03:08:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-24 03:09:34]

Description was changed from

==========
Add leakcanary to DEPS and third_party

BUG=505909
==========

to

==========
Add leakcanary to DEPS and third_party

BUG=505909

Committed: https://crrev.com/a1b62008719deafac9384edcada8743464658c54
Cr-Commit-Position: refs/heads/master@{#377201}
==========

[commit-bot: I haz the power, 2016-02-24 03:09:35]

Patchset 2 (id:??) landed as
https://crrev.com/a1b62008719deafac9384edcada8743464658c54
Cr-Commit-Position: refs/heads/master@{#377201}