Add SyncCustomizationFragmentTest.

Add SyncCustomizationFragmentTest.

This change adds a test class for SyncCustomizationFragment.
Common code between this new class and SyncTest has been split
out into SyncTestBase. Everything else in this CL is fixing various
bugs that popped up when I tried to run the test.

BUG=480604
Committed: https://crrev.com/f9576067c711e9f5cb12f15a378b89a67e7ed9ec
Cr-Commit-Position: refs/heads/master@{#327510}

[maxbogue, 2015-04-23 20:30:45]

maxbogue@chromium.org changed reviewers:
+ pvalenzuela@chromium.org, yfriedman@chromium.org

[maxbogue, 2015-04-23 20:30:47]

Hey Patrick and Yaron, could you please review this change?

[pval...(no longer on Chromium), 2015-04-23 22:37:28]

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:37:
private static final String DEFAULT_URL = "https://www.google.com";
Maybe add a comment noting that this must be https?

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
File
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
ChildAccountService.getInstance(targetContext).checkHasChildAccount(
why is this being added? (just curious)

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:24:
private static final String FOREIGN_SESSION_TEST_MACHINE_ID =
I think this name is a carry over from an old test. Maybe we can just call it
CLIENT_ID and give a more appropriate value here?

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java:44:
// Setup fake mapper from accountNames to Ids.
extract this logic to its own method? 

https://sourcemaking.com/refactoring/comments

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java:60:
// Mock out the account manager on the device.
maybe extract this as well?

[Yaron, 2015-04-24 16:49:59]

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
File
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
ChildAccountService.getInstance(targetContext).checkHasChildAccount(
On 2015/04/23 22:37:28, pvalenzuela wrote:
> why is this being added? (just curious)

Hmm. Can you replace the body of this with
ChromiumApplication.startChromeBrowserProcessesSync. I think it just wasn't
upstreamed when this was originally added.

[maxbogue, 2015-04-24 22:21:51]

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:37:
private static final String DEFAULT_URL = "https://www.google.com";
On 2015/04/23 22:37:28, pvalenzuela wrote:
> Maybe add a comment noting that this must be https?

It's actually a bug that it has to be https: http://crbug.com/480624

Not gonna add a comment since this is just a simple workaround for that.

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
File
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
ChildAccountService.getInstance(targetContext).checkHasChildAccount(
On 2015/04/23 22:37:28, pvalenzuela wrote:
> why is this being added? (just curious)

Because everything breaks without it! :)

It was added to the function this one was modeled off of at some point:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
ChildAccountService.getInstance(targetContext).checkHasChildAccount(
On 2015/04/24 16:49:58, Yaron wrote:
> On 2015/04/23 22:37:28, pvalenzuela wrote:
> > why is this being added? (just curious)
> 
> Hmm. Can you replace the body of this with
> ChromiumApplication.startChromeBrowserProcessesSync. I think it just wasn't
> upstreamed when this was originally added.

Done.

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java:24:
private static final String FOREIGN_SESSION_TEST_MACHINE_ID =
On 2015/04/23 22:37:28, pvalenzuela wrote:
> I think this name is a carry over from an old test. Maybe we can just call it
> CLIENT_ID and give a more appropriate value here?

Done.

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java:44:
// Setup fake mapper from accountNames to Ids.
On 2015/04/23 22:37:28, pvalenzuela wrote:
> extract this logic to its own method? 
> 
> https://sourcemaking.com/refactoring/comments

Done.

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java:60:
// Mock out the account manager on the device.
On 2015/04/23 22:37:28, pvalenzuela wrote:
> maybe extract this as well?

Done.

[Yaron, 2015-04-27 15:28:53]

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
File
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
ChildAccountService.getInstance(targetContext).checkHasChildAccount(
On 2015/04/24 22:21:51, maxbogue wrote:
> On 2015/04/24 16:49:58, Yaron wrote:
> > On 2015/04/23 22:37:28, pvalenzuela wrote:
> > > why is this being added? (just curious)
> > 
> > Hmm. Can you replace the body of this with
> > ChromiumApplication.startChromeBrowserProcessesSync. I think it just wasn't
> > upstreamed when this was originally added.
> 
> Done.

I think it's better to take the context and call getApplicationContext() from
it. You can get the application without hitting an activity so it can make for
faster in some instances. Requiring getActivity means we'll always
instantiate/start the activity here. Also, I think you still want to call
CommandLine.initFromFile (sorry for suggesting to replace the whole file). I
expect this would affect some perf tests which are setting the command-line
file.

[pval...(no longer on Chromium), 2015-04-27 19:19:15]

lgtm

LGTM from me, but you'll need Yaron's approval here

https://codereview.chromium.org/1060373004/diff/140001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/140001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTestBase.java:43:
mapAccountNamesToIds();
this looks really nice!

[maxbogue, 2015-04-27 22:05:40]

On 2015/04/27 15:28:53, Yaron wrote:
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
> File
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
> (right):
> 
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
> ChildAccountService.getInstance(targetContext).checkHasChildAccount(
> On 2015/04/24 22:21:51, maxbogue wrote:
> > On 2015/04/24 16:49:58, Yaron wrote:
> > > On 2015/04/23 22:37:28, pvalenzuela wrote:
> > > > why is this being added? (just curious)
> > > 
> > > Hmm. Can you replace the body of this with
> > > ChromiumApplication.startChromeBrowserProcessesSync. I think it just
wasn't
> > > upstreamed when this was originally added.
> > 
> > Done.
> 
> I think it's better to take the context and call getApplicationContext() from
> it. You can get the application without hitting an activity so it can make for
> faster in some instances. Requiring getActivity means we'll always
> instantiate/start the activity here. Also, I think you still want to call
> CommandLine.initFromFile (sorry for suggesting to replace the whole file). I
> expect this would affect some perf tests which are setting the command-line
> file.

Are the application and the application context the same thing? I was a bit
confused about that and what I saw online seemed to say that technically
they are but they might not always be so you shouldn't cast the Context
to an Application. I couldn't figure out any other way to get the Application
which is why I did it this way.

[Yaron, 2015-04-27 22:14:16]

On 2015/04/27 22:05:40, maxbogue wrote:
> On 2015/04/27 15:28:53, Yaron wrote:
> >
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
> > File
> >
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
> >
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
> > ChildAccountService.getInstance(targetContext).checkHasChildAccount(
> > On 2015/04/24 22:21:51, maxbogue wrote:
> > > On 2015/04/24 16:49:58, Yaron wrote:
> > > > On 2015/04/23 22:37:28, pvalenzuela wrote:
> > > > > why is this being added? (just curious)
> > > > 
> > > > Hmm. Can you replace the body of this with
> > > > ChromiumApplication.startChromeBrowserProcessesSync. I think it just
> wasn't
> > > > upstreamed when this was originally added.
> > > 
> > > Done.
> > 
> > I think it's better to take the context and call getApplicationContext()
from
> > it. You can get the application without hitting an activity so it can make
for
> > faster in some instances. Requiring getActivity means we'll always
> > instantiate/start the activity here. Also, I think you still want to call
> > CommandLine.initFromFile (sorry for suggesting to replace the whole file). I
> > expect this would affect some perf tests which are setting the command-line
> > file.
> 
> Are the application and the application context the same thing? I was a bit
> confused about that and what I saw online seemed to say that technically
> they are but they might not always be so you shouldn't cast the Context
> to an Application. I couldn't figure out any other way to get the Application
> which is why I did it this way.

Application is the application context which is one of the instances of Context.
Activity (and some others) are also Contexts. In this case, if you pass in the
targetContext and call getApplicationContext, you can cast it to Application.

[maxbogue, 2015-04-27 22:44:31]

On 2015/04/27 22:14:16, Yaron wrote:
> On 2015/04/27 22:05:40, maxbogue wrote:
> > On 2015/04/27 15:28:53, Yaron wrote:
> > >
> >
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
> > > File
> > >
> >
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
> > >
> >
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
> > > ChildAccountService.getInstance(targetContext).checkHasChildAccount(
> > > On 2015/04/24 22:21:51, maxbogue wrote:
> > > > On 2015/04/24 16:49:58, Yaron wrote:
> > > > > On 2015/04/23 22:37:28, pvalenzuela wrote:
> > > > > > why is this being added? (just curious)
> > > > > 
> > > > > Hmm. Can you replace the body of this with
> > > > > ChromiumApplication.startChromeBrowserProcessesSync. I think it just
> > wasn't
> > > > > upstreamed when this was originally added.
> > > > 
> > > > Done.
> > > 
> > > I think it's better to take the context and call getApplicationContext()
> from
> > > it. You can get the application without hitting an activity so it can make
> for
> > > faster in some instances. Requiring getActivity means we'll always
> > > instantiate/start the activity here. Also, I think you still want to call
> > > CommandLine.initFromFile (sorry for suggesting to replace the whole file).
I
> > > expect this would affect some perf tests which are setting the
command-line
> > > file.
> > 
> > Are the application and the application context the same thing? I was a bit
> > confused about that and what I saw online seemed to say that technically
> > they are but they might not always be so you shouldn't cast the Context
> > to an Application. I couldn't figure out any other way to get the
Application
> > which is why I did it this way.
> 
> Application is the application context which is one of the instances of
Context.
> Activity (and some others) are also Contexts. In this case, if you pass in the
> targetContext and call getApplicationContext, you can cast it to Application.

Done, PTAL.

[Yaron, 2015-04-28 00:00:10]

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
File
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
(right):

https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
ChildAccountService.getInstance(targetContext).checkHasChildAccount(
On 2015/04/27 15:28:53, Yaron wrote:
> On 2015/04/24 22:21:51, maxbogue wrote:
> > On 2015/04/24 16:49:58, Yaron wrote:
> > > On 2015/04/23 22:37:28, pvalenzuela wrote:
> > > > why is this being added? (just curious)
> > > 
> > > Hmm. Can you replace the body of this with
> > > ChromiumApplication.startChromeBrowserProcessesSync. I think it just
wasn't
> > > upstreamed when this was originally added.
> > 
> > Done.
> 
> I think it's better to take the context and call getApplicationContext() from
> it. You can get the application without hitting an activity so it can make for
> faster in some instances. Requiring getActivity means we'll always
> instantiate/start the activity here. Also, I think you still want to call
> CommandLine.initFromFile (sorry for suggesting to replace the whole file). I
> expect this would affect some perf tests which are setting the command-line
> file.

It still doesn't init CommandLine. did you try that?

[maxbogue, 2015-04-28 18:25:31]

On 2015/04/28 00:00:10, Yaron wrote:
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
> File
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java
> (right):
> 
>
https://codereview.chromium.org/1060373004/diff/100001/chrome/android/shell/j...
>
chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeShellTestBase.java:51:
> ChildAccountService.getInstance(targetContext).checkHasChildAccount(
> On 2015/04/27 15:28:53, Yaron wrote:
> > On 2015/04/24 22:21:51, maxbogue wrote:
> > > On 2015/04/24 16:49:58, Yaron wrote:
> > > > On 2015/04/23 22:37:28, pvalenzuela wrote:
> > > > > why is this being added? (just curious)
> > > > 
> > > > Hmm. Can you replace the body of this with
> > > > ChromiumApplication.startChromeBrowserProcessesSync. I think it just
> wasn't
> > > > upstreamed when this was originally added.
> > > 
> > > Done.
> > 
> > I think it's better to take the context and call getApplicationContext()
from
> > it. You can get the application without hitting an activity so it can make
for
> > faster in some instances. Requiring getActivity means we'll always
> > instantiate/start the activity here. Also, I think you still want to call
> > CommandLine.initFromFile (sorry for suggesting to replace the whole file). I
> > expect this would affect some perf tests which are setting the command-line
> > file.
> 
> It still doesn't init CommandLine. did you try that?

Done.

[Yaron, 2015-04-28 21:31:50]

lgtm

[maxbogue, 2015-04-29 16:45:54]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-04-29 16:45:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
pvalenzuela@chromium.org
Link to the patchset: https://codereview.chromium.org/1060373004/#ps180001
(title: "Use better start browser function.")

[commit-bot: I haz the power, 2015-04-29 16:47:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060373004/180001

[commit-bot: I haz the power, 2015-04-29 16:52:38]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-29 16:53:33]

Patchset 10 (id:??) landed as
https://crrev.com/f9576067c711e9f5cb12f15a378b89a67e7ed9ec
Cr-Commit-Position: refs/heads/master@{#327510}