Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2818643002:
Support ChildProcessAllocatorSettings in JUnit4 tests (Closed)
|
Created:
3 years, 8 months ago by the real yoland Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, Jay Civelli, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport ChildProcessAllocatorSettings in JUnit4 tests
BUG=640116
Review-Url: https://codereview.chromium.org/2818643002
Cr-Commit-Position: refs/heads/master@{#465313}
Committed: https://chromium.googlesource.com/chromium/src/+/86767028eea938a79b9b814ca3fe6f4396e38387
Patch Set 1 #
Total comments: 1
Patch Set 2 : add missing imports #
Total comments: 6
Patch Set 3 : rebase #Patch Set 4 : change to simple synchronized modifier #
Total comments: 2
Patch Set 5 : use lock #Patch Set 6 : rebase #Patch Set 7 : rebase #Messages
Total messages: 45 (28 generated)
yolandyan@chromium.org changed reviewers: + boliu@chromium.org, jbudorick@chromium.org
https://codereview.chromium.org/2818643002/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2818643002/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:39: public class ChromeInstrumentationTestRunner extends BaseChromiumInstrumentationTestRunner { ChromeInstrumentationTestRunner should not inherit ContentInstrumentationTestRunner, inheritance between Chrome and Content does not mean inheritance between there runners
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I'll add you next time for test changes :p cc jcivelli fyi https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:504: synchronized (ChildProcessLauncher.class) { no need to be fancy and prematurely optmize this, just wrap the whole thing in a synchornized block, and don't use volatile also don't use class as synchronization var, use a final static object https://codereview.chromium.org/2818643002/diff/20001/content/public/test/and... File content/public/test/android/BUILD.gn (right): https://codereview.chromium.org/2818643002/diff/20001/content/public/test/and... content/public/test/android/BUILD.gn:34: "javatests/src/org/chromium/content/browser/test/ContentInstrumentationTestRunner.java", is this still in use?
https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:504: synchronized (ChildProcessLauncher.class) { On 2017/04/13 at 01:05:54, boliu wrote: > no need to be fancy and prematurely optmize this, just wrap the whole thing in a synchornized block, and don't use volatile > > also don't use class as synchronization var, use a final static object hmm, I wouldn't call this prematurely though, but then I don't know how often and from what threads is this function or others that uses this function called...but this would not cause problems though.. https://codereview.chromium.org/2818643002/diff/20001/content/public/test/and... File content/public/test/android/BUILD.gn (right): https://codereview.chromium.org/2818643002/diff/20001/content/public/test/and... content/public/test/android/BUILD.gn:34: "javatests/src/org/chromium/content/browser/test/ContentInstrumentationTestRunner.java", On 2017/04/13 at 01:05:54, boliu wrote: > is this still in use? Yes
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:504: synchronized (ChildProcessLauncher.class) { On 2017/04/13 01:15:58, the real yoland wrote: > On 2017/04/13 at 01:05:54, boliu wrote: > > no need to be fancy and prematurely optmize this, just wrap the whole thing in > a synchornized block, and don't use volatile > > > > also don't use class as synchronization var, use a final static object > > hmm, I wouldn't call this prematurely though, but then I don't know how often > and from what threads is this function or others that uses this function > called...but this would not cause problems though.. don't know is exactly premature optmization... And the cost is this is impossible to read and fwiw, I do know when this thing is called, and it's only on certain user actions like closing a tab, or putting chrome into the background, so there is no point in optimization this at all
https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2818643002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:504: synchronized (ChildProcessLauncher.class) { On 2017/04/13 at 01:33:34, boliu wrote: > On 2017/04/13 01:15:58, the real yoland wrote: > > On 2017/04/13 at 01:05:54, boliu wrote: > > > no need to be fancy and prematurely optmize this, just wrap the whole thing in > > a synchornized block, and don't use volatile > > > > > > also don't use class as synchronization var, use a final static object > > > > hmm, I wouldn't call this prematurely though, but then I don't know how often > > and from what threads is this function or others that uses this function > > called...but this would not cause problems though.. > > don't know is exactly premature optmization... And the cost is this is impossible to read > > and fwiw, I do know when this thing is called, and it's only on certain user actions like closing a tab, or putting chrome into the background, so there is no point in optimization this at all alright done
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2818643002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2818643002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:202: public static synchronized BindingManager getBindingManager() { use explicit Object to synchronize
https://codereview.chromium.org/2818643002/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/2818643002/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:202: public static synchronized BindingManager getBindingManager() { On 2017/04/13 at 02:13:08, boliu wrote: > use explicit Object to synchronize Done
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
yolandyan@chromium.org changed reviewers: + agrieve@chromium.org
+agrieve for base/test/android OWNER stamp
On 2017/04/13 03:17:45, the real yoland wrote: > +agrieve for base/test/android OWNER stamp (I can look at this tomorrow, fwiw. dealing w/ proguard atm.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2017/04/13 13:30:20, jbudorick wrote: > lgtm fwiw, lgtm :)
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2818643002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, boliu@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2818643002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492541435425390,
"parent_rev": "7a70faddc69db1f8018d3124d5b7fefbc6b311fd", "commit_rev":
"86767028eea938a79b9b814ca3fe6f4396e38387"}
Message was sent while issue was closed.
Description was changed from ========== Support ChildProcessAllocatorSettings in JUnit4 tests BUG=640116 ========== to ========== Support ChildProcessAllocatorSettings in JUnit4 tests BUG=640116 Review-Url: https://codereview.chromium.org/2818643002 Cr-Commit-Position: refs/heads/master@{#465313} Committed: https://chromium.googlesource.com/chromium/src/+/86767028eea938a79b9b814ca3fe... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/86767028eea938a79b9b814ca3fe... |
