Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(130)

Issue 2517203002: Reland of Add GN build rules to allow java_assertion_enabler to enable Java asserts. (Closed)

Created:
4 years, 1 month ago by F
Modified:
4 years ago
CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland #2 of Add GN build rules to allow java_assertion_enabler to enable Java asserts. Reverted by: https://codereview.chromium.org/2506263003/ Reason for reland: Fixed failing tests. TBR=jbudorick@chromium.org,agrieve@chromium.org,toyoshim@chromium.org,qinmin@chromium.org,yolandyan@chromium.org BUG=666193, 667337, 462676, 665157, 665478, 667437 Committed: https://crrev.com/00a360e5ed7d790433b0be74169afa7643324d0d Cr-Commit-Position: refs/heads/master@{#433681}

Patch Set 1 : Reland of Add GN build rules to allow java_assertion_enabler to enable Java asserts. (patchset #1 i… #

Patch Set 2 : Fixing assert error #

Patch Set 3 : format nit #

Total comments: 3

Messages

Total messages: 29 (20 generated)
F
Created Reland of Add GN build rules to allow java_assertion_enabler to enable Java asserts.
4 years, 1 month ago (2016-11-21 16:34:08 UTC) #1
agrieve
On 2016/11/21 16:34:08, F wrote: > Created Reland of Add GN build rules to allow ...
4 years, 1 month ago (2016-11-21 19:53:25 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2517203002/250001
4 years, 1 month ago (2016-11-21 20:00:28 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:250001)
4 years, 1 month ago (2016-11-21 23:05:34 UTC) #22
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/00a360e5ed7d790433b0be74169afa7643324d0d Cr-Commit-Position: refs/heads/master@{#433681}
4 years, 1 month ago (2016-11-21 23:08:24 UTC) #24
boliu
this CL should *not* have been tbr-ed. https://codereview.chromium.org/2517203002/diff/250001/android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewThreadTest.java File android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewThreadTest.java (right): https://codereview.chromium.org/2517203002/diff/250001/android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewThreadTest.java#newcode21 android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewThreadTest.java:21: private static ...
4 years ago (2016-11-23 16:50:39 UTC) #26
boliu
https://codereview.chromium.org/2517203002/diff/250001/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java File content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java (right): https://codereview.chromium.org/2517203002/diff/250001/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java#newcode252 content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:252: // assert mObservers.isEmpty(); also is this being fixed?
4 years ago (2016-11-23 17:20:23 UTC) #27
agrieve
Sorry, I think tbr was appropriate since the change was 99% the same as last ...
4 years ago (2016-11-23 17:55:39 UTC) #28
boliu
4 years ago (2016-11-23 18:03:36 UTC) #29
Message was sent while issue was closed.
On 2016/11/23 17:55:39, agrieve wrote:
> Sorry, I think tbr was appropriate since the change was 99% the same as last
> time, and I reviewed the different parts. 

Additional changes still needs to be reviewed by owners

> 
> The timeout was causing the test to fail for zpeng@ locally when running that
> test. Admittedly, bumping that should have been a separate change.
> 
>
https://codereview.chromium.org/2517203002/diff/250001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
> (right):
> 
>
https://codereview.chromium.org/2517203002/diff/250001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:252:
> // assert mObservers.isEmpty();
> On 2016/11/23 17:20:22, boliu_back_dec wrote:
> > also is this being fixed?
> 
> Looks like there's action on the bug.

Powered by Google App Engine
This is Rietveld 408576698