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

Issue 2233023002: Adding BlimpNavigationController to Tab (Closed)

Created:
4 years, 4 months ago by shaktisahu
Modified:
4 years, 4 months ago
CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@nav_handler_remove
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Adding BlimpNavigationController to Tab Added blimp navigation controller to tab so that tab can route the navigation calls to the correct navigation controller depending on blimp mode. A TabObserver was created to be notified about the blimp navigation events. Also changed ToolbarModelImpl to reflect the correct url in blimp mode. Also moved the blimp client context delegate initialization to an early life cycle method in ChromeActivity. Added a method to get the Java object from native BlimpContents. BUG=611100 Committed: https://crrev.com/26a1d3f241bfaeaf32db9a3b10d3b10a0d9817b4 Cr-Commit-Position: refs/heads/master@{#413924}

Patch Set 1 #

Total comments: 16

Patch Set 2 : initial comments #

Total comments: 21

Patch Set 3 : @nyquist feedback #

Total comments: 18

Patch Set 4 : David's comments #

Patch Set 5 : Passing Profile from Java #

Patch Set 6 : Nits from David #

Patch Set 7 : merge origin/master #

Patch Set 8 : Fixing unit tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+229 lines, -30 lines) Patch
M blimp/client/core/android/blimp_client_context_impl_android.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M blimp/client/core/android/java/src/org/chromium/blimp/core/BlimpClientContextImpl.java View 1 2 3 4 5 6 1 chunk +7 lines, -2 lines 0 comments Download
M blimp/client/core/android/java/src/org/chromium/blimp/core/DummyBlimpClientContext.java View 1 2 3 4 5 6 7 1 chunk +6 lines, -2 lines 0 comments Download
M blimp/client/core/contents/android/blimp_navigation_controller_impl_android.h View 1 chunk +2 lines, -0 lines 0 comments Download
M blimp/client/core/contents/android/blimp_navigation_controller_impl_android.cc View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
M blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/BlimpNavigationControllerImpl.java View 1 3 chunks +8 lines, -1 line 0 comments Download
M blimp/client/core/contents/blimp_contents_impl.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M blimp/client/core/contents/blimp_contents_impl.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M blimp/client/core/contents/blimp_navigation_controller_impl.h View 1 2 chunks +4 lines, -0 lines 0 comments Download
M blimp/client/core/contents/blimp_navigation_controller_impl.cc View 3 chunks +8 lines, -2 lines 0 comments Download
M blimp/client/core/contents/blimp_navigation_controller_impl_unittest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M blimp/client/public/android/java/src/org/chromium/blimp_public/contents/BlimpNavigationController.java View 1 2 chunks +7 lines, -5 lines 0 comments Download
M blimp/client/public/contents/blimp_contents.h View 1 2 2 chunks +10 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java View 1 2 3 4 5 6 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java View 1 2 3 4 5 6 7 15 chunks +80 lines, -10 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/tab/TabBlimpContentsObserver.java View 1 2 3 1 chunk +31 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarModelImpl.java View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/android/blimp/blimp_client_context_factory.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/android/blimp/blimp_client_context_factory.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/android/tab_android.h View 1 2 3 4 5 6 4 chunks +17 lines, -0 lines 0 comments Download
M chrome/browser/android/tab_android.cc View 1 2 3 4 5 6 4 chunks +24 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 38 (15 generated)
shaktisahu
4 years, 4 months ago (2016-08-10 17:52:16 UTC) #3
David Trainor- moved to gerrit
haven't had a chance to get to this yet. adding nyquist@ to take a look ...
4 years, 4 months ago (2016-08-11 15:51:23 UTC) #5
David Trainor- moved to gerrit
initial comments! https://codereview.chromium.org/2233023002/diff/1/blimp/client/core/contents/blimp_navigation_controller_impl.h File blimp/client/core/contents/blimp_navigation_controller_impl.h (right): https://codereview.chromium.org/2233023002/diff/1/blimp/client/core/contents/blimp_navigation_controller_impl.h#newcode62 blimp/client/core/contents/blimp_navigation_controller_impl.h:62: std::string current_title_; Maybe comment? https://codereview.chromium.org/2233023002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java ...
4 years, 4 months ago (2016-08-11 21:40:19 UTC) #6
xingliu
https://codereview.chromium.org/2233023002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2233023002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode454 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:454: mBlimpClientContextDelegate = On 2016/08/11 21:40:19, David Trainor wrote: > ...
4 years, 4 months ago (2016-08-11 23:26:25 UTC) #8
nyquist
https://codereview.chromium.org/2233023002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2233023002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode454 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:454: mBlimpClientContextDelegate = On 2016/08/11 23:26:25, xingliu wrote: > On ...
4 years, 4 months ago (2016-08-12 05:57:18 UTC) #9
shaktisahu
https://codereview.chromium.org/2233023002/diff/1/blimp/client/core/contents/blimp_navigation_controller_impl.h File blimp/client/core/contents/blimp_navigation_controller_impl.h (right): https://codereview.chromium.org/2233023002/diff/1/blimp/client/core/contents/blimp_navigation_controller_impl.h#newcode62 blimp/client/core/contents/blimp_navigation_controller_impl.h:62: std::string current_title_; On 2016/08/11 21:40:19, David Trainor wrote: > ...
4 years, 4 months ago (2016-08-12 22:11:49 UTC) #10
nyquist
https://codereview.chromium.org/2233023002/diff/20001/blimp/client/core/android/blimp_client_context_impl_android.cc File blimp/client/core/android/blimp_client_context_impl_android.cc (right): https://codereview.chromium.org/2233023002/diff/20001/blimp/client/core/android/blimp_client_context_impl_android.cc#newcode64 blimp/client/core/android/blimp_client_context_impl_android.cc:64: BlimpClientContextImplAndroid::CreateBlimpContentsJava(JNIEnv* env, Do we still need this, or could ...
4 years, 4 months ago (2016-08-16 19:09:21 UTC) #11
nyquist
https://codereview.chromium.org/2233023002/diff/20001/chrome/browser/android/tab_android.cc File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2233023002/diff/20001/chrome/browser/android/tab_android.cc#newcode430 chrome/browser/android/tab_android.cc:430: void TabAndroid::InitBlimpContents(JNIEnv* env, Do we want to ensure that ...
4 years, 4 months ago (2016-08-16 19:17:11 UTC) #12
nyquist
https://codereview.chromium.org/2233023002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2233023002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode1514 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1514: if (isBlimpTab() && getBlimpContents() == null) { Maybe ask ...
4 years, 4 months ago (2016-08-16 21:42:25 UTC) #13
shaktisahu
https://codereview.chromium.org/2233023002/diff/20001/blimp/client/core/android/blimp_client_context_impl_android.cc File blimp/client/core/android/blimp_client_context_impl_android.cc (right): https://codereview.chromium.org/2233023002/diff/20001/blimp/client/core/android/blimp_client_context_impl_android.cc#newcode64 blimp/client/core/android/blimp_client_context_impl_android.cc:64: BlimpClientContextImplAndroid::CreateBlimpContentsJava(JNIEnv* env, On 2016/08/16 19:09:20, nyquist wrote: > Do ...
4 years, 4 months ago (2016-08-16 23:46:13 UTC) #14
David Trainor- moved to gerrit
https://codereview.chromium.org/2233023002/diff/40001/blimp/client/core/contents/blimp_navigation_controller_impl.cc File blimp/client/core/contents/blimp_navigation_controller_impl.cc (right): https://codereview.chromium.org/2233023002/diff/40001/blimp/client/core/contents/blimp_navigation_controller_impl.cc#newcode70 blimp/client/core/contents/blimp_navigation_controller_impl.cc:70: current_url_ = url; Do we have unit tests for ...
4 years, 4 months ago (2016-08-18 17:06:30 UTC) #15
David Trainor- moved to gerrit
https://codereview.chromium.org/2233023002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2233023002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode660 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:660: .isBlimpEnabled(); && !mIsIncognito https://codereview.chromium.org/2233023002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode3321 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3321: private native void nativeInitBlimpContents(long ...
4 years, 4 months ago (2016-08-18 18:28:19 UTC) #16
shaktisahu
David, PTAL https://codereview.chromium.org/2233023002/diff/40001/blimp/client/core/contents/blimp_navigation_controller_impl.cc File blimp/client/core/contents/blimp_navigation_controller_impl.cc (right): https://codereview.chromium.org/2233023002/diff/40001/blimp/client/core/contents/blimp_navigation_controller_impl.cc#newcode70 blimp/client/core/contents/blimp_navigation_controller_impl.cc:70: current_url_ = url; On 2016/08/18 17:06:30, David ...
4 years, 4 months ago (2016-08-18 19:36:06 UTC) #17
David Trainor- moved to gerrit
lgtm % nit https://codereview.chromium.org/2233023002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2233023002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode2078 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2078: mBlimpContents = null; On 2016/08/18 19:36:06, ...
4 years, 4 months ago (2016-08-22 21:30:00 UTC) #18
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/2233023002/100001
4 years, 4 months ago (2016-08-23 00:54:03 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/56433) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, ...
4 years, 4 months ago (2016-08-23 00:56:26 UTC) #23
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/2233023002/120001
4 years, 4 months ago (2016-08-23 16:36:09 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/127947)
4 years, 4 months ago (2016-08-23 18:08:46 UTC) #28
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/2233023002/140001
4 years, 4 months ago (2016-08-23 19:05:52 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/279306)
4 years, 4 months ago (2016-08-23 21:33:30 UTC) #33
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/2233023002/140001
4 years, 4 months ago (2016-08-24 00:19:31 UTC) #35
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 4 months ago (2016-08-24 01:24:56 UTC) #36
commit-bot: I haz the power
4 years, 4 months ago (2016-08-24 01:27:06 UTC) #38
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/26a1d3f241bfaeaf32db9a3b10d3b10a0d9817b4
Cr-Commit-Position: refs/heads/master@{#413924}

Powered by Google App Engine
This is Rietveld 408576698