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

Issue 1687393002: Add assigner support to Blimp (Closed)

Created:
4 years, 10 months ago by David Trainor- moved to gerrit
Modified:
4 years, 10 months ago
Reviewers:
nyquist, Kevin M, mmenke
CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add assigner support to Blimp - Add code to contact the assigner for an engine instance. - Add plumbing to pipe the request from Java to native. - Add plumbing to pipe the response back to Java to provide a UI indication of the success or failure of the request. - Connect the assignment response with the BlimpClientSession. BUG=582668 Committed: https://crrev.com/6df6164eda86c8ccdb3a0aad8025d9d2a827ee10 Cr-Commit-Position: refs/heads/master@{#376447}

Patch Set 1 #

Total comments: 49

Patch Set 2 : Added tests and addressed comments #

Total comments: 14

Patch Set 3 : Added more tests and addressed command line arg including scheme. #

Total comments: 3

Patch Set 4 : Fix redundant if check #

Patch Set 5 : Build the ProxyConfigService on the main thread. #

Total comments: 42

Patch Set 6 : Addressed nyquist@ comments #

Patch Set 7 : Tommy was right. We already have a thread check in GetAssignment. Added to callback. #

Patch Set 8 : Address more comments :) #

Total comments: 1

Patch Set 9 : Nits, the final battle! #

Patch Set 10 : Fix evil build break? #

Unified diffs Side-by-side diffs Delta from patch set Stats (+944 lines, -104 lines) Patch
M blimp/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M blimp/client/BUILD.gn View 1 2 3 4 5 6 7 3 chunks +26 lines, -0 lines 0 comments Download
M blimp/client/DEPS View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M blimp/client/app/android/blimp_client_session_android.h View 1 2 3 4 5 6 7 1 chunk +10 lines, -3 lines 0 comments Download
M blimp/client/app/android/blimp_client_session_android.cc View 1 2 3 4 5 5 chunks +23 lines, -9 lines 0 comments Download
M blimp/client/app/android/blimp_library_loader.cc View 2 chunks +4 lines, -0 lines 0 comments Download
M blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java View 1 2 3 4 5 5 chunks +21 lines, -8 lines 0 comments Download
M blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java View 1 2 3 4 5 3 chunks +68 lines, -4 lines 0 comments Download
M blimp/client/app/android/java/strings/android_blimp_strings.grd View 1 chunk +33 lines, -0 lines 0 comments Download
M blimp/client/app/blimp_client_switches.h View 1 1 chunk +1 line, -2 lines 0 comments Download
M blimp/client/app/blimp_client_switches.cc View 1 2 1 chunk +4 lines, -7 lines 0 comments Download
M blimp/client/app/linux/blimp_client_session_linux.h View 1 chunk +1 line, -2 lines 0 comments Download
M blimp/client/app/linux/blimp_client_session_linux.cc View 1 chunk +2 lines, -4 lines 0 comments Download
M blimp/client/app/linux/blimp_main.cc View 2 chunks +3 lines, -5 lines 0 comments Download
M blimp/client/session/assignment_source.h View 1 2 3 4 5 6 7 8 9 1 chunk +70 lines, -6 lines 0 comments Download
M blimp/client/session/assignment_source.cc View 1 2 3 4 5 6 7 8 1 chunk +272 lines, -35 lines 0 comments Download
A blimp/client/session/assignment_source_unittest.cc View 1 2 3 4 5 1 chunk +355 lines, -0 lines 0 comments Download
M blimp/client/session/blimp_client_session.h View 1 2 3 4 5 2 chunks +18 lines, -10 lines 0 comments Download
M blimp/client/session/blimp_client_session.cc View 1 2 3 4 5 6 7 4 chunks +22 lines, -8 lines 0 comments Download
M blimp/common/protocol_version.h View 1 chunk +3 lines, -0 lines 0 comments Download
M blimp/docs/running.md View 1 2 3 4 5 2 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 32 (5 generated)
David Trainor- moved to gerrit
Hey Kevin! Can you ptal? I'm going to add some unit tests shortly, but I ...
4 years, 10 months ago (2016-02-12 16:30:20 UTC) #2
Kevin M
https://codereview.chromium.org/1687393002/diff/1/blimp/client/app/android/blimp_client_session_android.cc File blimp/client/app/android/blimp_client_session_android.cc (right): https://codereview.chromium.org/1687393002/diff/1/blimp/client/app/android/blimp_client_session_android.cc#newcode64 blimp/client/app/android/blimp_client_session_android.cc:64: void BlimpClientSessionAndroid::ConnectWithAssignment( I think that subclassing this method obfuscates ...
4 years, 10 months ago (2016-02-12 19:45:27 UTC) #3
David Trainor- moved to gerrit
addressed comments. ptal! https://codereview.chromium.org/1687393002/diff/1/blimp/client/app/android/blimp_client_session_android.cc File blimp/client/app/android/blimp_client_session_android.cc (right): https://codereview.chromium.org/1687393002/diff/1/blimp/client/app/android/blimp_client_session_android.cc#newcode64 blimp/client/app/android/blimp_client_session_android.cc:64: void BlimpClientSessionAndroid::ConnectWithAssignment( On 2016/02/12 19:45:26, Kevin ...
4 years, 10 months ago (2016-02-17 21:09:48 UTC) #4
David Trainor- moved to gerrit
mmenke@: Can you look at blimp/client/DEPS? We're eventually going to integrate this into chrome/, so ...
4 years, 10 months ago (2016-02-17 21:12:55 UTC) #6
Kevin M
This is looking pretty good. https://codereview.chromium.org/1687393002/diff/20001/blimp/client/app/blimp_client_switches.cc File blimp/client/app/blimp_client_switches.cc (right): https://codereview.chromium.org/1687393002/diff/20001/blimp/client/app/blimp_client_switches.cc#newcode11 blimp/client/app/blimp_client_switches.cc:11: // --blimplet-host="127.0.0.1:25467". Ooh, this ...
4 years, 10 months ago (2016-02-17 22:00:15 UTC) #7
David Trainor- moved to gerrit
addressed comments thanks! ptal :). https://codereview.chromium.org/1687393002/diff/20001/blimp/client/app/blimp_client_switches.cc File blimp/client/app/blimp_client_switches.cc (right): https://codereview.chromium.org/1687393002/diff/20001/blimp/client/app/blimp_client_switches.cc#newcode11 blimp/client/app/blimp_client_switches.cc:11: // --blimplet-host="127.0.0.1:25467". On 2016/02/17 ...
4 years, 10 months ago (2016-02-17 23:02:32 UTC) #8
David Trainor- moved to gerrit
https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc#newcode54 blimp/client/session/assignment_source.cc:54: if (url.is_empty() || !url.is_valid() || oops will fix.
4 years, 10 months ago (2016-02-17 23:05:19 UTC) #9
mmenke
https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc#newcode106 blimp/client/session/assignment_source.cc:106: io_loop_task_runner_, base::ThreadTaskRunnerHandle::Get())); On Android, this has to be created ...
4 years, 10 months ago (2016-02-17 23:12:19 UTC) #10
David Trainor- moved to gerrit
https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc#newcode106 blimp/client/session/assignment_source.cc:106: io_loop_task_runner_, base::ThreadTaskRunnerHandle::Get())); On 2016/02/17 23:12:19, mmenke wrote: > On ...
4 years, 10 months ago (2016-02-17 23:28:11 UTC) #11
mmenke
On 2016/02/17 23:28:11, David Trainor wrote: > https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc > File blimp/client/session/assignment_source.cc (right): > > https://codereview.chromium.org/1687393002/diff/40001/blimp/client/session/assignment_source.cc#newcode106 ...
4 years, 10 months ago (2016-02-17 23:30:56 UTC) #12
David Trainor- moved to gerrit
On 2016/02/17 23:30:56, mmenke wrote: > On 2016/02/17 23:28:11, David Trainor wrote: > > > ...
4 years, 10 months ago (2016-02-17 23:35:06 UTC) #13
David Trainor- moved to gerrit
On 2016/02/17 23:30:56, mmenke wrote: > On 2016/02/17 23:28:11, David Trainor wrote: > > > ...
4 years, 10 months ago (2016-02-17 23:35:07 UTC) #14
mmenke
On 2016/02/17 23:35:07, David Trainor wrote: > On 2016/02/17 23:30:56, mmenke wrote: > > On ...
4 years, 10 months ago (2016-02-18 00:12:34 UTC) #15
nyquist
https://codereview.chromium.org/1687393002/diff/80001/blimp/client/app/android/blimp_client_session_android.cc File blimp/client/app/android/blimp_client_session_android.cc (right): https://codereview.chromium.org/1687393002/diff/80001/blimp/client/app/android/blimp_client_session_android.cc#newcode48 blimp/client/app/android/blimp_client_session_android.cc:48: const base::android::JavaParamRef<jobject>& jobj, Nit: Should we include base/android/scoped_java_ref.h by ...
4 years, 10 months ago (2016-02-18 01:58:40 UTC) #16
David Trainor- moved to gerrit
On 2016/02/18 00:12:34, mmenke wrote: > On 2016/02/17 23:35:07, David Trainor wrote: > > On ...
4 years, 10 months ago (2016-02-18 04:09:59 UTC) #17
David Trainor- moved to gerrit
addressed comments! thanks :). https://codereview.chromium.org/1687393002/diff/80001/blimp/client/app/android/blimp_client_session_android.cc File blimp/client/app/android/blimp_client_session_android.cc (right): https://codereview.chromium.org/1687393002/diff/80001/blimp/client/app/android/blimp_client_session_android.cc#newcode48 blimp/client/app/android/blimp_client_session_android.cc:48: const base::android::JavaParamRef<jobject>& jobj, On 2016/02/18 ...
4 years, 10 months ago (2016-02-18 16:01:55 UTC) #18
mmenke
This LGTM. A couple more quick comments. You may want to add me as a ...
4 years, 10 months ago (2016-02-18 16:06:12 UTC) #19
nyquist
lgtm
4 years, 10 months ago (2016-02-18 17:10:21 UTC) #20
mmenke
Also worth noting that URLFetcher has no timeouts. So if, for instance, you're being a ...
4 years, 10 months ago (2016-02-18 17:26:49 UTC) #21
mmenke
On 2016/02/18 17:26:49, mmenke wrote: > Also worth noting that URLFetcher has no timeouts. So ...
4 years, 10 months ago (2016-02-18 17:28:02 UTC) #22
David Trainor- moved to gerrit
https://codereview.chromium.org/1687393002/diff/80001/blimp/client/session/assignment_source.cc File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1687393002/diff/80001/blimp/client/session/assignment_source.cc#newcode175 blimp/client/session/assignment_source.cc:175: url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(0); On 2016/02/18 16:06:11, mmenke wrote: > optional: Suggest ...
4 years, 10 months ago (2016-02-18 17:38:26 UTC) #23
David Trainor- moved to gerrit
On 2016/02/18 17:28:02, mmenke wrote: > On 2016/02/18 17:26:49, mmenke wrote: > > Also worth ...
4 years, 10 months ago (2016-02-18 17:43:45 UTC) #24
mmenke
On 2016/02/18 17:43:45, David Trainor wrote: > On 2016/02/18 17:28:02, mmenke wrote: > > On ...
4 years, 10 months ago (2016-02-18 18:26:03 UTC) #25
Kevin M
lgtm https://codereview.chromium.org/1687393002/diff/140001/blimp/client/session/assignment_source.cc File blimp/client/session/assignment_source.cc (right): https://codereview.chromium.org/1687393002/diff/140001/blimp/client/session/assignment_source.cc#newcode69 blimp/client/session/assignment_source.cc:69: if (url.SchemeIs("ssl")) { Can you make these into ...
4 years, 10 months ago (2016-02-18 22:14:14 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687393002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687393002/180001
4 years, 10 months ago (2016-02-19 15:24:29 UTC) #29
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 10 months ago (2016-02-19 16:10:34 UTC) #30
commit-bot: I haz the power
4 years, 10 months ago (2016-02-19 16:11:47 UTC) #32
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/6df6164eda86c8ccdb3a0aad8025d9d2a827ee10
Cr-Commit-Position: refs/heads/master@{#376447}

Powered by Google App Engine
This is Rietveld 408576698