Telemetry android backend: re-raise BrowserGoneException instead of calling exit() when failing to …

Telemetry android backend: re-raise BrowserGoneException instead of calling exit() when failing to start browser.

This addresses a review comment from:
https://codereview.chromium.org/1252253003/

BUG=None

Committed: https://crrev.com/7a684ed7872978ab1c54afe24c9264c3d9ad89e7
Cr-Commit-Position: refs/heads/master@{#341982}

[yzshen1, 2015-08-05 16:42:10]

yzshen@chromium.org changed reviewers:
+ nednguyen@chromium.org, nednguyen@google.com

[yzshen1, 2015-08-05 16:42:11]

Hi, Ned.

Would you please take a look? Thanks!

[nednguyen, 2015-08-05 17:23:40]

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:120:
self.Close()
It looks to me that line 120 -> 126 can be simplified to:

except exceptions.BrowserGoneException:
   if not (self.device.HasRoot()...):
      raise exceptions.BrowserGone(msg=...)  # Add useful message
finally:
   self.Close()

[yzshen1, 2015-08-05 17:31:32]

Thanks Ned!

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:120:
self.Close()
On 2015/08/05 17:23:39, nednguyen wrote:
> It looks to me that line 120 -> 126 can be simplified to:
> 
> except exceptions.BrowserGoneException:
>    if not (self.device.HasRoot()...):
>       raise exceptions.BrowserGone(msg=...)  # Add useful message

I am not super familiar with this, but "raise" without argument re-raise the
latest exception with the original information (such as the stack trace). I
think that is useful, what do you think?

 
> finally:
>    self.Close()

self.Close() is not supposed to be called if the try block completes
successfully.

[nednguyen, 2015-08-05 19:07:58]

Thanks Yuzhu! lgtm

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:120:
self.Close()
On 2015/08/05 17:31:32, yzshen1 wrote:
> On 2015/08/05 17:23:39, nednguyen wrote:
> > It looks to me that line 120 -> 126 can be simplified to:
> > 
> > except exceptions.BrowserGoneException:
> >    if not (self.device.HasRoot()...):
> >       raise exceptions.BrowserGone(msg=...)  # Add useful message
> 
> I am not super familiar with this, but "raise" without argument re-raise the
> latest exception with the original information (such as the stack trace). I
> think that is useful, what do you think?
> 
>  
> > finally:
> >    self.Close()
> 
> self.Close() is not supposed to be called if the try block completes
> successfully.

Err, you are right. Can you remove the traceback print in line 123 -> 125?

[yzshen1, 2015-08-05 20:17:42]

Thanks, Ned!

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/1275613002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:120:
self.Close()
On 2015/08/05 19:07:58, nednguyen wrote:
> On 2015/08/05 17:31:32, yzshen1 wrote:
> > On 2015/08/05 17:23:39, nednguyen wrote:
> > > It looks to me that line 120 -> 126 can be simplified to:
> > > 
> > > except exceptions.BrowserGoneException:
> > >    if not (self.device.HasRoot()...):
> > >       raise exceptions.BrowserGone(msg=...)  # Add useful message
> > 
> > I am not super familiar with this, but "raise" without argument re-raise the
> > latest exception with the original information (such as the stack trace). I
> > think that is useful, what do you think?
> > 
> >  
> > > finally:
> > >    self.Close()
> > 
> > self.Close() is not supposed to be called if the try block completes
> > successfully.
> 
> Err, you are right. Can you remove the traceback print in line 123 -> 125? 

Done.

[yzshen1, 2015-08-05 20:17:44]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-08-05 20:17:45]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1275613002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-08-05 20:18:49]

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

[commit-bot: I haz the power, 2015-08-05 21:44:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-05 21:44:51]

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