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

Issue 381593002: Removing ContentView dependencies for few functions which acts as WebContents wrapper. (Closed)

Created:
6 years, 5 months ago by AKVT
Modified:
6 years, 5 months ago
Reviewers:
Ted C, Yaron, no sievers
CC:
chromium-reviews, darin-cc_chromium.org, jam, jdduke (slow)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Removing ContentView dependencies for few functions which acts as WebContents wrapper. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284334

Patch Set 1 #

Patch Set 2 : Fixed nits and white space issues. #

Patch Set 3 : Fixed white space issues. #

Total comments: 4

Patch Set 4 : Fixed review comments #

Total comments: 2

Patch Set 5 : Fixed review comments. #

Total comments: 14

Patch Set 6 : Fixed review comments #

Total comments: 4

Patch Set 7 : Removed unnecessary web_contents_ NULL check #

Total comments: 4

Patch Set 8 : Fixed review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+327 lines, -163 lines) Patch
M content/browser/android/content_view_core_impl.h View 1 2 3 4 5 6 7 3 chunks +0 lines, -21 lines 0 comments Download
M content/browser/android/content_view_core_impl.cc View 1 2 3 4 5 6 7 3 chunks +0 lines, -89 lines 0 comments Download
M content/browser/web_contents/web_contents_android.h View 1 2 3 4 5 6 7 2 chunks +23 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_android.cc View 1 2 3 4 5 6 7 2 chunks +124 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 2 3 4 5 6 7 16 chunks +25 lines, -53 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java View 1 2 3 4 5 1 chunk +81 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content_public/browser/WebContents.java View 1 2 3 4 5 1 chunk +74 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (0 generated)
AKVT
PTAL this change
6 years, 5 months ago (2014-07-09 11:34:32 UTC) #1
AKVT
PTAL
6 years, 5 months ago (2014-07-09 11:44:27 UTC) #2
AKVT
@Yaron and jdduke - PTAL
6 years, 5 months ago (2014-07-09 16:48:39 UTC) #3
jdduke (slow)
https://codereview.chromium.org/381593002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/381593002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode853 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853: if (mWebContents != null) return mWebContents.getBackgroundColor(); I wonder if ...
6 years, 5 months ago (2014-07-09 17:13:14 UTC) #4
Yaron
https://codereview.chromium.org/381593002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/381593002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode853 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853: if (mWebContents != null) return mWebContents.getBackgroundColor(); On 2014/07/09 17:13:14, ...
6 years, 5 months ago (2014-07-09 17:34:02 UTC) #5
AKVT
@jdduke and Yaron PTAL https://codereview.chromium.org/381593002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/381593002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode853 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:853: if (mWebContents != null) return ...
6 years, 5 months ago (2014-07-11 12:59:42 UTC) #6
AKVT
On 2014/07/11 12:59:42, AKV wrote: > @jdduke and Yaron PTAL > > https://codereview.chromium.org/381593002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > File ...
6 years, 5 months ago (2014-07-11 16:42:27 UTC) #7
jdduke (slow)
https://codereview.chromium.org/381593002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/381593002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode856 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:856: assert mWebContents == null; Have you done at least ...
6 years, 5 months ago (2014-07-11 16:51:25 UTC) #8
AKVT
@jdduke & Yaron PTAL https://codereview.chromium.org/381593002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/381593002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode856 content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:856: assert mWebContents == null; On ...
6 years, 5 months ago (2014-07-12 08:46:12 UTC) #9
AKVT
On 2014/07/12 08:46:12, AKV wrote: > @jdduke & Yaron PTAL > > https://codereview.chromium.org/381593002/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > File ...
6 years, 5 months ago (2014-07-14 18:52:27 UTC) #10
AKVT
On 2014/07/14 18:52:27, AKV wrote: > On 2014/07/12 08:46:12, AKV wrote: > > @jdduke & ...
6 years, 5 months ago (2014-07-15 05:05:07 UTC) #11
jdduke (slow)
On 2014/07/15 05:05:07, AKV wrote: > On 2014/07/14 18:52:27, AKV wrote: > > @jdduke and ...
6 years, 5 months ago (2014-07-15 15:32:45 UTC) #12
Yaron
https://codereview.chromium.org/381593002/diff/80001/content/browser/android/content_view_core_impl.cc File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/381593002/diff/80001/content/browser/android/content_view_core_impl.cc#oldcode1368 content/browser/android/content_view_core_impl.cc:1368: long ContentViewCoreImpl::GetNativeImeAdapter(JNIEnv* env, jobject obj) { I don't think ...
6 years, 5 months ago (2014-07-15 21:23:49 UTC) #13
Yaron
as an aside: I thought of another way of looking at this general issue. We ...
6 years, 5 months ago (2014-07-15 21:28:07 UTC) #14
AKVT
PTAL https://codereview.chromium.org/381593002/diff/80001/content/browser/android/content_view_core_impl.cc File content/browser/android/content_view_core_impl.cc (left): https://codereview.chromium.org/381593002/diff/80001/content/browser/android/content_view_core_impl.cc#oldcode1368 content/browser/android/content_view_core_impl.cc:1368: long ContentViewCoreImpl::GetNativeImeAdapter(JNIEnv* env, jobject obj) { On 2014/07/15 ...
6 years, 5 months ago (2014-07-16 09:53:47 UTC) #15
Yaron
https://codereview.chromium.org/381593002/diff/100001/content/browser/web_contents/web_contents_android.cc File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/381593002/diff/100001/content/browser/web_contents/web_contents_android.cc#newcode91 content/browser/web_contents/web_contents_android.cc:91: if (web_contents_) { remove if-check. This is constructed with ...
6 years, 5 months ago (2014-07-16 17:14:54 UTC) #16
AKVT
PTAL https://codereview.chromium.org/381593002/diff/100001/content/browser/web_contents/web_contents_android.cc File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/381593002/diff/100001/content/browser/web_contents/web_contents_android.cc#newcode91 content/browser/web_contents/web_contents_android.cc:91: if (web_contents_) { On 2014/07/16 17:14:54, Yaron wrote: ...
6 years, 5 months ago (2014-07-17 03:08:32 UTC) #17
Yaron
lgtm +tedchoc for web_contents_android
6 years, 5 months ago (2014-07-18 00:39:40 UTC) #18
AKVT
@Ted C and sievers PTAL.
6 years, 5 months ago (2014-07-18 15:25:16 UTC) #19
no sievers
lgtm, but you might want to TBR Ted at least (he's OOTO). https://codereview.chromium.org/381593002/diff/120001/content/browser/web_contents/web_contents_android.cc File content/browser/web_contents/web_contents_android.cc ...
6 years, 5 months ago (2014-07-18 19:42:03 UTC) #20
AKVT
PTAL https://codereview.chromium.org/381593002/diff/120001/content/browser/web_contents/web_contents_android.cc File content/browser/web_contents/web_contents_android.cc (right): https://codereview.chromium.org/381593002/diff/120001/content/browser/web_contents/web_contents_android.cc#newcode188 content/browser/web_contents/web_contents_android.cc:188: RenderViewHost* host = web_contents_->GetRenderViewHost(); On 2014/07/18 19:42:03, sievers ...
6 years, 5 months ago (2014-07-19 10:39:33 UTC) #21
AKVT
The CQ bit was checked by ajith.v@samsung.com
6 years, 5 months ago (2014-07-19 10:39:53 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajith.v@samsung.com/381593002/140001
6 years, 5 months ago (2014-07-19 10:40:30 UTC) #23
commit-bot: I haz the power
6 years, 5 months ago (2014-07-19 12:55:08 UTC) #24
Message was sent while issue was closed.
Change committed as 284334

Powered by Google App Engine
This is Rietveld 408576698