Issue 11926028: Remove ContentViewCore.enableMultiProcess etc.

Issue 11926028: Remove ContentViewCore.enableMultiProcess etc. (Closed)

Created:
7 years, 11 months ago by michaelbai

Modified:
7 years, 10 months ago

Reviewers:
joth, Yaron

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org, jochen+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Remove ContentViewCore.enableMultiProcess etc. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181765

Patch Set 3 : combine 2 init methods to 1 #

Total comments: 2

Patch Set 5 : Remove R #

Created: 7 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+18 lines, -142 lines)			Patch
M	android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java	View	1 2 3	2 chunks	+3 lines, -8 lines	0 comments	Download
M	chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java	View	1 2	2 chunks	+2 lines, -1 line	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java	View	1 2 3 4	4 chunks	+10 lines, -49 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/ContentView.java	View	1 2 3	1 chunk	+0 lines, -48 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java	View	1 2 3	1 chunk	+0 lines, -34 lines	0 comments	Download
M	content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java	View	1 2 3	2 chunks	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

Yaron

lgtm Please land the downstream one firs tto avoid a temporary breakage

7 years, 11 months ago (2013-01-19 00:39:01 UTC) #2

joth

https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode19 android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:19: import org.chromium.content.browser.ContentView; you should be able to remove ContentView ...

7 years, 11 months ago (2013-01-20 22:03:54 UTC) #3

https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java
(right):

https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:19:
import org.chromium.content.browser.ContentView;
you should be able to remove ContentView import.

https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:57:
AndroidBrowserProcess.initChromiumBrowserProcess(
interesting. I think this should really be using initContentViewProcess. 

initChromiumBrowserProcess() is supposed to be only for when running as "the
platform browser" (i.e. chrome)


but while you're here... AwBrowserProcess.start(context) would be an even better
choice, now we have that.

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java
(right):

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:89:
* @param maxRendererProcesses See ContentView.initContentViewProcess().
ContentView => AndroidBrowserProcess

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:92:
public static boolean initChromiumBrowserProcess(Context context, int
maxRendererProcesses)
so looking through this, the only difference between 'content' and 'chromium'
process is whether the R.array.official_command_line  is processed below.
I don't know anyone uses that anymore? If not, we could remove this version of
the method altogether, and just have a single BrowserProcess.init method.
(or even if it is needed, we can do just it unconditionally, as
R.array.official_command_line will be empty string on webview it really won't
hurt)

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:59:
android.R.attr.webViewStyle, personality);
(aside - the |personality| param and associated constants are spurious here.
ContentView should only ever be used in "chrome" personality.)

Yaron

https://codereview.chromium.org/11926028/diff/3001/content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java File content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java (right): https://codereview.chromium.org/11926028/diff/3001/content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java#newcode92 content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:92: public static boolean initChromiumBrowserProcess(Context context, int maxRendererProcesses) On 2013/01/20 ...

7 years, 11 months ago (2013-01-22 18:09:47 UTC) #4

michaelbai

PTAL https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java File android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java (right): https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java#newcode19 android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:19: import org.chromium.content.browser.ContentView; On 2013/01/20 22:03:54, joth wrote: > ...

7 years, 11 months ago (2013-01-24 22:29:59 UTC) #5

PTAL

https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java
(right):

https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:19:
import org.chromium.content.browser.ContentView;
On 2013/01/20 22:03:54, joth wrote:
> you should be able to remove ContentView import.

Done.

https://codereview.chromium.org/11926028/diff/3001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidWebViewTestBase.java:57:
AndroidBrowserProcess.initChromiumBrowserProcess(
On 2013/01/20 22:03:54, joth wrote:
> interesting. I think this should really be using initContentViewProcess. 
> 
> initChromiumBrowserProcess() is supposed to be only for when running as "the
> platform browser" (i.e. chrome)
> 
> 
> but while you're here... AwBrowserProcess.start(context) would be an even
better
> choice, now we have that.

Done.

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java
(right):

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:89:
* @param maxRendererProcesses See ContentView.initContentViewProcess().
On 2013/01/20 22:03:54, joth wrote:
> ContentView => AndroidBrowserProcess

Done.

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:92:
public static boolean initChromiumBrowserProcess(Context context, int
maxRendererProcesses)
On 2013/01/22 18:09:47, Yaron wrote:
> On 2013/01/20 22:03:54, joth wrote:
> > so looking through this, the only difference between 'content' and
'chromium'
> > process is whether the R.array.official_command_line  is processed below.
> > I don't know anyone uses that anymore? If not, we could remove this version
of
> > the method altogether, and just have a single BrowserProcess.init method.
> > (or even if it is needed, we can do just it unconditionally, as
> > R.array.official_command_line will be empty string on webview it really
won't
> > hurt)
> 
> I agree that we should remove it. I'm pretty sure official_command_line was
> temporary

Done.

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentView.java
(right):

https://codereview.chromium.org/11926028/diff/3001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentView.java:59:
android.R.attr.webViewStyle, personality);
I am not sure I fully understand the issue, Could you file a bug if you think
there is problem here?

Thanks


On 2013/01/20 22:03:54, joth wrote:
> (aside - the |personality| param and associated constants are spurious here.
> ContentView should only ever be used in "chrome" personality.)

joth

lgtm https://codereview.chromium.org/11926028/diff/7002/content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java File content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java (right): https://codereview.chromium.org/11926028/diff/7002/content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java#newcode20 content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:20: import org.chromium.content.R; you can probably remove R now?

7 years, 10 months ago (2013-02-04 21:05:52 UTC) #6

michaelbai

https://codereview.chromium.org/11926028/diff/7002/content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java File content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java (right): https://codereview.chromium.org/11926028/diff/7002/content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java#newcode20 content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:20: import org.chromium.content.R; On 2013/02/04 21:05:52, joth wrote: > you ...

7 years, 10 months ago (2013-02-11 18:58:48 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/11926028/17006

7 years, 10 months ago (2013-02-11 18:59:19 UTC) #8

Message was sent while issue was closed.

Change committed as 181765

Expand Messages | Collapse Messages