Include v2 packaged apps in sync.

Include v2 packaged apps in sync.

BUG=161489
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168631

[Marijn Kruisselbrink, 2012-11-16 18:31:58]

asargent for chrome/common/extensions
akalin for chrome/browser/sync

[akalin, 2012-11-16 18:38:53]

few comments/questions.  nice tests!

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/apps_helper.cc (right):

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/apps_helper.cc:53:
CreateFakeAppName(index),
can you add a CreateFakePlatformAppName() function?  Otherwise bad things might
happen if for some reason a fake app and a fake platform app for the same index
are installed.

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/single_client_apps_sync_test.cc
(right):

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/single_client_apps_sync_test.cc:55:
ASSERT_TRUE(AllProfilesHaveSameAppsAsVerifier());
Doesn't AllProfileHaveSameAppsAsVerifier() have to be modified to look for
platform apps? (It may be that the answer is 'no' if it just looks at the sync
type and not the extension type)

(Or perhaps have an AllProfilesHaveSamePlatformAppsAsVerifier() function)

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/single_client_apps_sync_test.cc:58:
IN_PROC_BROWSER_TEST_F(SingleClientAppsSyncTest, InstallSomeApps) {
can you add an InstallSomePlatformApps test also?

perhaps also another test that installs a mixture of both.

[Marijn Kruisselbrink, 2012-11-16 18:54:30]

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/apps_helper.cc (right):

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/apps_helper.cc:53:
CreateFakeAppName(index),
On 2012/11/16 18:38:53, akalin wrote:
> can you add a CreateFakePlatformAppName() function?  Otherwise bad things
might
> happen if for some reason a fake app and a fake platform app for the same
index
> are installed.

Done.

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/single_client_apps_sync_test.cc
(right):

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/single_client_apps_sync_test.cc:55:
ASSERT_TRUE(AllProfilesHaveSameAppsAsVerifier());
On 2012/11/16 18:38:53, akalin wrote:
> Doesn't AllProfileHaveSameAppsAsVerifier() have to be modified to look for
> platform apps? (It may be that the answer is 'no' if it just looks at the sync
> type and not the extension type)
As far as I could tell AllProfileHaveSameAppsAsVerifier indeed only looks at the
sync type (so the tests I added would have passed also without the
Extension::GetSyncType change, since then it would just not include them in the
verification either...).

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/single_client_apps_sync_test.cc:58:
IN_PROC_BROWSER_TEST_F(SingleClientAppsSyncTest, InstallSomeApps) {
On 2012/11/16 18:38:53, akalin wrote:
> can you add an InstallSomePlatformApps test also?
> 
> perhaps also another test that installs a mixture of both.

Done.

[akalin, 2012-11-16 20:09:06]

lgtm

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/single_client_apps_sync_test.cc
(right):

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/single_client_apps_sync_test.cc:55:
ASSERT_TRUE(AllProfilesHaveSameAppsAsVerifier());
On 2012/11/16 18:54:30, Marijn Kruisselbrink wrote:
> On 2012/11/16 18:38:53, akalin wrote:
> > Doesn't AllProfileHaveSameAppsAsVerifier() have to be modified to look for
> > platform apps? (It may be that the answer is 'no' if it just looks at the
sync
> > type and not the extension type)
> As far as I could tell AllProfileHaveSameAppsAsVerifier indeed only looks at
the
> sync type (so the tests I added would have passed also without the
> Extension::GetSyncType change, since then it would just not include them in
the
> verification either...).

Okay, I think you're right.

Can you update the comment to AllProfilesHaveSameAppsAsVerifier() to mention
platform apps?

[asargent_no_longer_on_chrome, 2012-11-16 20:52:28]

LGTM

[commit-bot: I haz the power, 2012-11-16 23:46:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11308072/7001

[commit-bot: I haz the power, 2012-11-17 00:55:57]

Retried try job too often for step(s) sync_integration_tests

[Marijn Kruisselbrink, 2012-11-19 18:43:01]

akalin: I made a couple more changes/bug fixes to the tests, can you take
another look?

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
File chrome/browser/sync/test/integration/apps_helper.cc (right):

https://codereview.chromium.org/11308072/diff/8/chrome/browser/sync/test/inte...
chrome/browser/sync/test/integration/apps_helper.cc:53:
CreateFakeAppName(index),
On 2012/11/16 18:54:30, Marijn Kruisselbrink wrote:
> On 2012/11/16 18:38:53, akalin wrote:
> > can you add a CreateFakePlatformAppName() function?  Otherwise bad things
> might
> > happen if for some reason a fake app and a fake platform app for the same
> index
> > are installed.
> 
> Done.
Actually, I decided to undo this again (since this change was what caused the
test to not pass). There are just too many places in apps_helper where it
assumes it can determine the name from just an index (the alternative would be
to have 2 copies of almost every method or pass an extension type to almost
every method).

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (left):

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/sync_extension_helper.cc:293:
!file_util::CreateTemporaryDirInDir(
I think this if statement doesn't do what it was supposed to do (only if
base_dir doesn't exist and couldn't be created does it try to create a
subdirectory in base_dir?) so I split it up into two if statements
(alternatively the last && could have been changed into ||, but I don't see the
point in making the code more complicated then necessary.

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/sync_extension_helper.cc:143: void
SyncExtensionHelper::InstallExtensionsPendingForSync(Profile* profile) {
To properly support different types of extensions in one test I changed this to
not get the extension type passed here, but instead I handle it similar to how
the name of an extension is associated with an id.

[akalin, 2012-11-19 19:28:59]

lgtm after nit below

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/apps_helper.h (right):

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/apps_helper.h:30: // extension ID of the
new app.
perhaps add a comment about how one shouldn't install a "regular" app and a
platform app for the same index.

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/sync_test.cc:364: // around, so run
messages both before and after closing all browsers.
whoa, what did you run into that needed this?

[Marijn Kruisselbrink, 2012-11-19 19:39:59]

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/apps_helper.h (right):

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/apps_helper.h:30: // extension ID of the
new app.
On 2012/11/19 19:28:59, akalin wrote:
> perhaps add a comment about how one shouldn't install a "regular" app and a
> platform app for the same index.

Done.

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/11308072/diff/14001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/sync_test.cc:364: // around, so run
messages both before and after closing all browsers.
On 2012/11/19 19:28:59, akalin wrote:
> whoa, what did you run into that needed this?
Without that line, I got the following stack trace, which as far as I can tell
is caused by the loading of the background pages of platform apps after their
browser processes have quit.

[6687:6687:1119/102634:1206708479018:FATAL:browser_process_impl.cc(511)] Check
failed: print_job_manager_.get(). 
        base::debug::StackTrace::StackTrace() [0x7f48f1b5b764]
        logging::LogMessage::~LogMessage() [0x7f48f1b8d5f9]
        BrowserProcessImpl::print_job_manager() [0x1017080]
        PrintingMessageFilter::PrintingMessageFilter() [0xdfac91]
        chrome::ChromeContentBrowserClient::RenderProcessHostCreated()
[0xcc4b22]
        content::RenderProcessHostImpl::Init() [0x7f48ea7caa89]
        content::RenderViewHostImpl::CreateRenderView() [0x7f48ea7de57c]
        content::WebContentsImpl::CreateRenderViewForRenderManager()
[0x7f48ea89d913]
        content::RenderViewHostManager::InitRenderView() [0x7f48ea88b110]
        content::RenderViewHostManager::CreateRenderView() [0x7f48ea88afeb]
        content::RenderViewHostManager::UpdateRendererStateForNavigate()
[0x7f48ea88bbe3]
        content::RenderViewHostManager::Navigate() [0x7f48ea88947e]
        content::WebContentsImpl::NavigateToEntry() [0x7f48ea896e9b]
        content::WebContentsImpl::NavigateToPendingEntry() [0x7f48ea896d80]
        content::NavigationControllerImpl::NavigateToPendingEntry()
[0x7f48ea88414e]
        content::NavigationControllerImpl::LoadEntry() [0x7f48ea87f6b2]
        content::NavigationControllerImpl::LoadURLWithParams() [0x7f48ea880997]
        content::NavigationControllerImpl::LoadURL() [0x7f48ea880434]
        extensions::ExtensionHost::LoadInitialURL() [0x1134012]
        extensions::ExtensionHost::CreateRenderViewNow() [0x1133d52]
        extensions::ExtensionHost::ProcessCreationQueue::ProcessOneHost()
[0x11362b7]
        base::internal::RunnableAdapter<>::Run() [0x1137610]
        base::internal::InvokeHelper<>::MakeItSo() [0x11374fc]
        base::internal::Invoker<>::Run() [0x11373c9]
        base::Callback<>::Run() [0x7f48f1b537a1]
        MessageLoop::RunTask() [0x7f48f1b924a9]
        MessageLoop::DeferOrRunPendingTask() [0x7f48f1b925c4]
        MessageLoop::DoWork() [0x7f48f1b92e31]
        base::MessagePumpGlib::HandleDispatch() [0x7f48f1b396fd]
        (anonymous namespace)::WorkSourceDispatch() [0x7f48f1b39023]
        <unknown> [0x7f48e69b2d53]
        <unknown> [0x7f48e69b30a0]
        <unknown> [0x7f48e69b3164]
        base::MessagePumpGlib::RunWithDispatcher() [0x7f48f1b393ac]
        base::MessagePumpGlib::Run() [0x7f48f1b397da]
        MessageLoop::RunInternal() [0x7f48f1b920ad]
        MessageLoop::RunHandler() [0x7f48f1b91f64]
        base::RunLoop::Run() [0x7f48f1bc6b54]
        content::RunThisRunLoop() [0x1096283]
        content::RunMessageLoop() [0x1096218]
        content::RunAllPendingInMessageLoop() [0x1096316]
        SyncTest::CleanUpOnMainThread() [0x757cec]
        InProcessBrowserTest::RunTestOnMainThreadLoop() [0x13a3e8a]
        content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() [0x20f34c1]
        base::internal::RunnableAdapter<>::Run() [0x20f39f8]
        base::internal::InvokeHelper<>::MakeItSo() [0x20f398f]
        base::internal::Invoker<>::Run() [0x20f3947]
        base::Callback<>::Run() [0x70043b]
        ChromeBrowserMainParts::PreMainMessageLoopRunImpl() [0xfdc78e]
        ChromeBrowserMainParts::PreMainMessageLoopRun() [0xfdaf68]
        content::BrowserMainLoop::CreateThreads() [0x7f48ea59128c]
        content::BrowserMainRunnerImpl::Initialize() [0x7f48ea593d0a]
        content::BrowserMain() [0x7f48ea58f84f]
        content::BrowserTestBase::SetUp() [0x20f344c]
        InProcessBrowserTest::SetUp() [0x13a3193]
        SyncTest::SetUp() [0x7563ef]
        testing::internal::HandleSehExceptionsInMethodIfSupported<>()
[0x1603fb4]
        testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1600e85]
        testing::Test::Run() [0x15f681f]
        testing::TestInfo::Run() [0x15f6e4e]
        testing::TestCase::Run() [0x15f7340]
        testing::internal::UnitTestImpl::RunAllTests() [0x15fb94e]

[commit-bot: I haz the power, 2012-11-19 20:19:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11308072/11007

[commit-bot: I haz the power, 2012-11-19 23:33:19]

Change committed as 168631