tools/android/loading: Properly detect page loading failures

tools/android/loading: Properly detect page loading failures

Before, one page failures, Blink sided generated response were
triggering assertion in request_track.RequestTrack.

This CL fixes this issue by removing the asserts and instead
implements loading_trace.LoadingTrace.HasLoadingSucceed()
that is based on navigation trace events.

BUG=582080
Committed: https://crrev.com/c44e7bf15904d461676839524c3a850dc6d05da7
Cr-Commit-Position: refs/heads/master@{#402481}

[gabadie, 2016-06-28 09:41:37]

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

[mattcary, 2016-06-28 10:07:06]

Add unittests. Since python isn't compiled, eg, in the request track now that
something nontrivial is being done with the LoadingFailed case we should check
the error message is being added correctly. Also, after moving the
LoadingSucceeded predicate to tracing_track it should be easy to add a test that
will similarly catch stupid errors the next time that method is changed.

https://codereview.chromium.org/2103673003/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_trace.py (right):

https://codereview.chromium.org/2103673003/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_trace.py:71: def HasLoadingSucceed(self):
Move this to the tracing track. Then you can test it easily, and it also avoids
leaking the tracing track into the loading trace.

[gabadie, 2016-06-28 11:42:45]

Uploaded a new revision. PTAL.

https://codereview.chromium.org/2103673003/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_trace.py (right):

https://codereview.chromium.org/2103673003/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_trace.py:71: def HasLoadingSucceed(self):
On 2016/06/28 10:07:06, mattcary wrote:
> Move this to the tracing track. Then you can test it easily, and it also
avoids
> leaking the tracing track into the loading trace.

Makes sens. Done.

[mattcary, 2016-06-28 11:44:41]

and request_track_unitest.py?

[gabadie, 2016-06-28 11:57:10]

On 2016/06/28 11:44:41, mattcary wrote:
> and request_track_unitest.py?

My bad. Did not understand to also add test coverage for removal of untested
asserts. Done. PTAL.

[mattcary, 2016-06-28 12:27:59]

Ah, mostly I meant LoadingFailed and making sure the new error text is handled
correctly. But thanks for the additional test, it is welcome ;)

[gabadie, 2016-06-28 12:47:10]

On 2016/06/28 12:27:59, mattcary wrote:
> Ah, mostly I meant LoadingFailed and making sure the new error text is handled
> correctly. But thanks for the additional test, it is welcome ;)

Oh I see. Done!

[mattcary, 2016-06-28 12:57:12]

lgtm

Thanks!

[gabadie, 2016-06-28 12:57:57]

gabadie@chromium.org changed reviewers:
+ blundell@chromium.org

[gabadie, 2016-06-28 12:57:58]

blundell@chromium.org: PTAL

[blundell, 2016-06-28 13:53:01]

lgtm

https://codereview.chromium.org/2103673003/diff/70001/tools/android/loading/t...
File tools/android/loading/tracing.py (right):

https://codereview.chromium.org/2103673003/diff/70001/tools/android/loading/t...
tools/android/loading/tracing.py:235: def HasLoadingSucceed(self):
nit: HasLoadingSucceeded

[gabadie, 2016-06-28 14:29:54]

Thanks guys for the review! Landing.

https://codereview.chromium.org/2103673003/diff/70001/tools/android/loading/t...
File tools/android/loading/tracing.py (right):

https://codereview.chromium.org/2103673003/diff/70001/tools/android/loading/t...
tools/android/loading/tracing.py:235: def HasLoadingSucceed(self):
On 2016/06/28 13:53:01, blundell wrote:
> nit: HasLoadingSucceeded

Done.

[gabadie, 2016-06-28 14:29:58]

The CQ bit was checked by gabadie@chromium.org

[gabadie, 2016-06-28 14:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2103673003/#ps90001
(title: "Addresses Colin's nit")

[commit-bot: I haz the power, 2016-06-28 14:30:38]

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

[commit-bot: I haz the power, 2016-06-28 15:49:03]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-06-28 15:50:34]

Description was changed from

==========
tools/android/loading: Properly detect page loading failures

Before, one page failures, Blink sided generated response were
triggering assertion in request_track.RequestTrack.

This CL fixes this issue by removing the asserts and instead
implements loading_trace.LoadingTrace.HasLoadingSucceed()
that is based on navigation trace events.

BUG=582080
==========

to

==========
tools/android/loading: Properly detect page loading failures

Before, one page failures, Blink sided generated response were
triggering assertion in request_track.RequestTrack.

This CL fixes this issue by removing the asserts and instead
implements loading_trace.LoadingTrace.HasLoadingSucceed()
that is based on navigation trace events.

BUG=582080

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

[commit-bot: I haz the power, 2016-06-28 15:50:36]

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