Handle DRT crash caused by Android OOM.
Created attachment 134003 [details] Patch
Comment on attachment 134003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134003&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:580 > + if crash and line is not None: > + error.append(line) I'm not sure I quite understand what these lines do. Can you explain a bit more? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:484 > + if drt_return == 137: Can we name this constant?
This looks fine, I'm just unsure about the change to chromium.py.
(In reply to comment #2) > (From update of attachment 134003 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134003&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:580 > > + if crash and line is not None: > > + error.append(line) > > I'm not sure I quite understand what these lines do. Can you explain a bit more? > Sure. This is to append the last output line when crash happens. > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:484 > > + if drt_return == 137: > > Can we name this constant? Will replace with signal.SIGKILL.
Comment on attachment 134003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134003&action=review Ok. >>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:580 >>> + error.append(line) >> >> I'm not sure I quite understand what these lines do. Can you explain a bit more? > > Sure. This is to append the last output line when crash happens. I see. Previously we were truncating.
Created attachment 134009 [details] Patch
Comment on attachment 134009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review Should there be test cases for this? > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551 > + return None if (return_match is None) else int(return_match.group(1)) Nit: This could be "return return_match and int(return_match.group(1))".
Comment on attachment 134009 [details] Patch Rejecting attachment 134009 [details] from commit-queue. zhenghao@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
(In reply to comment #7) > (From update of attachment 134009 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review > > Should there be test cases for this? > As it involves adb command and a recursion of 10 levels, I could not find a good way to test it. Any suggestion on this? I could add tests in https://bugs.webkit.org/show_bug.cgi?id=80852 . > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551 > > + return None if (return_match is None) else int(return_match.group(1)) > > Nit: This could be "return return_match and int(return_match.group(1))". Ternary operator looks more readable to me. Is there any style guideline on this? Tony, as I don't have commit right, could you set commit-queue for me? Thanks!
(In reply to comment #9) > (In reply to comment #7) > > (From update of attachment 134009 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review > > > > Should there be test cases for this? > > > > As it involves adb command and a recursion of 10 levels, I could not find a good way to test it. Any suggestion on this? I could add tests in https://bugs.webkit.org/show_bug.cgi?id=80852 . Can you mock out adb? > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551 > > > + return None if (return_match is None) else int(return_match.group(1)) > > > > Nit: This could be "return return_match and int(return_match.group(1))". > > Ternary operator looks more readable to me. Is there any style guideline on this? No style guide, hence a nit.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #7) > > > (From update of attachment 134009 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=134009&action=review > > > > > > Should there be test cases for this? > > > > > > > As it involves adb command and a recursion of 10 levels, I could not find a good way to test it. Any suggestion on this? I could add tests in https://bugs.webkit.org/show_bug.cgi?id=80852 . > > Can you mock out adb? > Good idea. Need some refactoring. Will do in bug 80852. > > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:551 > > > > + return None if (return_match is None) else int(return_match.group(1)) > > > > > > Nit: This could be "return return_match and int(return_match.group(1))". > > > > Ternary operator looks more readable to me. Is there any style guideline on this? > > No style guide, hence a nit.
Comment on attachment 134009 [details] Patch Clearing flags on attachment: 134009 Committed r112359: <http://trac.webkit.org/changeset/112359>
All reviewed patches have been landed. Closing bug.