Clovis: update resource sack to use new dependency graph.

Clovis: update resource sack to use new dependency graph.

This is a refactoring with (hopefully) no functional change.

Committed: https://crrev.com/6341256079ecdb84a47165c1fda47ed308ccb87e
Cr-Commit-Position: refs/heads/master@{#383719}

[mattcary, 2016-03-29 13:25:18]

mattcary@chromium.org changed reviewers:
+ lizeb@chromium.org

[mattcary, 2016-03-29 13:25:18]

Benoît---

Here's an update to the Sack code to use your new graph. It drops the
visualization because that's not very useful now, I think, and not worth the
effort to refactor.

[Benoit L, 2016-03-29 13:45:42]

On 2016/03/29 13:25:18, mattcary wrote:
> Benoît---
> 
> Here's an update to the Sack code to use your new graph. It drops the
> visualization because that's not very useful now, I think, and not worth the
> effort to refactor.

Thanks!

Only nits, and a question.

[Benoit L, 2016-03-29 13:45:51]

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
File tools/android/loading/resource_sack.py (right):

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
tools/android/loading/resource_sack.py:58: sorted according to its current
filter.
nit: There are no filters anymore.

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
tools/android/loading/resource_sack.py:63: for n in request_graph.graph.Nodes():
The nodes are no longer sorted, is it an issue? (beyond testing)

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
tools/android/loading/resource_sack.py:162: def _MakeShortname(cls, url):
nit: Add a TODO(lizeb) to move this method elsewhere, while you're touching it.
It's copied in a couple places.

[mattcary, 2016-03-29 13:56:07]

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
File tools/android/loading/resource_sack.py (right):

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
tools/android/loading/resource_sack.py:58: sorted according to its current
filter.
On 2016/03/29 13:45:50, Benoit L wrote:
> nit: There are no filters anymore.

Done.

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
tools/android/loading/resource_sack.py:63: for n in request_graph.graph.Nodes():
On 2016/03/29 13:45:50, Benoit L wrote:
> The nodes are no longer sorted, is it an issue? (beyond testing)

No. The idea was that because adding nodes transitively adds descendants, doing
it over sorted nodes was more efficient. I'm not actually sure that's true, and
anyway since (I think/hope) it doesn't affect correctness it's doesn't really
matter.

https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
tools/android/loading/resource_sack.py:162: def _MakeShortname(cls, url):
On 2016/03/29 13:45:50, Benoit L wrote:
> nit: Add a TODO(lizeb) to move this method elsewhere, while you're touching
it.
> It's copied in a couple places.

Done.

Note it's not exactly copied... but I agree the variations are immaterial and we
should standardize somehow. Note also that in the very new future I think I'll
want to do approximate URL matching, so it might be best to wait for that to
shake out before spending time on this.

[Benoit L, 2016-03-29 14:06:14]

On 2016/03/29 13:56:07, mattcary wrote:
>
https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
> File tools/android/loading/resource_sack.py (right):
> 
>
https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
> tools/android/loading/resource_sack.py:58: sorted according to its current
> filter.
> On 2016/03/29 13:45:50, Benoit L wrote:
> > nit: There are no filters anymore.
> 
> Done.
> 
>
https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
> tools/android/loading/resource_sack.py:63: for n in
request_graph.graph.Nodes():
> On 2016/03/29 13:45:50, Benoit L wrote:
> > The nodes are no longer sorted, is it an issue? (beyond testing)
> 
> No. The idea was that because adding nodes transitively adds descendants,
doing
> it over sorted nodes was more efficient. I'm not actually sure that's true,
and
> anyway since (I think/hope) it doesn't affect correctness it's doesn't really
> matter.
> 
>
https://codereview.chromium.org/1837193002/diff/1/tools/android/loading/resou...
> tools/android/loading/resource_sack.py:162: def _MakeShortname(cls, url):
> On 2016/03/29 13:45:50, Benoit L wrote:
> > nit: Add a TODO(lizeb) to move this method elsewhere, while you're touching
> it.
> > It's copied in a couple places.
> 
> Done.
> 
> Note it's not exactly copied... but I agree the variations are immaterial and
we
> should standardize somehow. Note also that in the very new future I think I'll
> want to do approximate URL matching, so it might be best to wait for that to
> shake out before spending time on this.

Thanks, lgtm.

[mattcary, 2016-03-29 14:08:51]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2016-03-29 14:08:51]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1837193002/#ps10009
(title: "comments")

[commit-bot: I haz the power, 2016-03-29 14:09:03]

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

[commit-bot: I haz the power, 2016-03-29 14:39:08]

Committed patchset #2 (id:10009)

[commit-bot: I haz the power, 2016-03-29 14:40:42]

Description was changed from

==========
Clovis: update resource sack to use new dependency graph.

This is a refactoring with (hopefully) no functional change.
==========

to

==========
Clovis: update resource sack to use new dependency graph.

This is a refactoring with (hopefully) no functional change.

Committed: https://crrev.com/6341256079ecdb84a47165c1fda47ed308ccb87e
Cr-Commit-Position: refs/heads/master@{#383719}
==========

[commit-bot: I haz the power, 2016-03-29 14:40:43]

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