Move chrome.alarms API from chrome/ to extensions/.

Move chrome.alarms API from chrome/ to extensions/.

This CL allows app_shell to use chrome.alarms.

Also it includes the following changes related to unit tests:
- Moved chrome.alarms tests to extensions/
- Moved a helper function CreateEmptyExtensionWithLocation()
  from extension_function_test_utils.h to api_test_utils.h.
- Moved CreateBackgroundPage() from extension_api_unittest.h to
  api_unittest.h.
- Creates WebContents for the extension that can be used to associate
  a RenderViewHost in extension unit tests.

BUG=443622
Committed: https://crrev.com/745ff1db8cca6a4fdc0c19cf3edab909bd532457
Cr-Commit-Position: refs/heads/master@{#314040}

[babu, 2015-01-05 08:10:33]

sudarsana.nagineni@intel.com changed reviewers:
+ jamescook@chromium.org, mpcomplete@chromium.org, yoz@chromium.org

[James Cook, 2015-01-05 19:22:43]

Looking good, but please move the unit test to //extensions as well.

https://codereview.chromium.org/789643004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api_unittest.cc (right):

https://codereview.chromium.org/789643004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Can this file also be moved into //extensions?  You should be able to use
extensions/browser/api_unittest.h and the browser()->profile() can probably be
replaced with browser_context().

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
File extensions/browser/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
extensions/browser/api/alarms/alarm_manager.h:5: #ifndef
EXTENSIONS_BROWSER_API_ALARMS_ALARM_MANAGER_H__
nit, since you are in the file: end with single underscore,
"...ALARM_MANAGER_H_"

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
File extensions/browser/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
extensions/browser/api/alarms/alarms_api.h:5: #ifndef
EXTENSIONS_BROWSER_API_ALARMS_ALARMS_API_H__
same comment

[babu, 2015-01-16 19:32:37]

Sorry for the late response.

https://codereview.chromium.org/789643004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api_unittest.cc (right):

https://codereview.chromium.org/789643004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2015/01/05 19:22:43, James Cook wrote:
> Can this file also be moved into //extensions?  You should be able to use
> extensions/browser/api_unittest.h and the browser()->profile() can probably be
> replaced with browser_context().

Some refactoring is needed in tests since one of the test case is checking for  
warning message posted on sink and it is dependent on RenderViewHost and
RenderViewHostTestEnabler.

I'm guessing that adding dependency on those objects in api unittests might not
be acceptable, so is it okay to refactor the tests in a separate CL?

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
File extensions/browser/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
extensions/browser/api/alarms/alarm_manager.h:5: #ifndef
EXTENSIONS_BROWSER_API_ALARMS_ALARM_MANAGER_H__
On 2015/01/05 19:22:43, James Cook wrote:
> nit, since you are in the file: end with single underscore,
> "...ALARM_MANAGER_H_"

Done.

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
File extensions/browser/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/789643004/diff/1/extensions/browser/api/alarm...
extensions/browser/api/alarms/alarms_api.h:5: #ifndef
EXTENSIONS_BROWSER_API_ALARMS_ALARMS_API_H__
On 2015/01/05 19:22:43, James Cook wrote:
> same comment

Done.

[James Cook, 2015-01-16 20:56:31]

https://codereview.chromium.org/789643004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/alarms/alarms_api_unittest.cc (right):

https://codereview.chromium.org/789643004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/alarms/alarms_api_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2015/01/16 19:32:37, babu wrote:
> On 2015/01/05 19:22:43, James Cook wrote:
> > Can this file also be moved into //extensions?  You should be able to use
> > extensions/browser/api_unittest.h and the browser()->profile() can probably
be
> > replaced with browser_context().
> 
> Some refactoring is needed in tests since one of the test case is checking for
 
> warning message posted on sink and it is dependent on RenderViewHost and
> RenderViewHostTestEnabler.
> 
> I'm guessing that adding dependency on those objects in api unittests might
not
> be acceptable, so is it okay to refactor the tests in a separate CL?

RenderViewHost and RenderViewHostTestEnabler are in //content, which can be used
freely from //extensions.  I would prefer the test to move in the same CL that
moves the implementation.

[babu, 2015-01-16 23:08:51]

On 2015/01/16 20:56:31, James Cook wrote:
> RenderViewHost and RenderViewHostTestEnabler are in //content, which can be
used
> freely from //extensions.  I would prefer the test to move in the same CL that
> moves the implementation.

Thanks for reviewing, James. I updated the CL with the tests.

[babu, 2015-01-16 23:32:28]

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
File extensions/browser/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
extensions/browser/api/alarms/alarm_manager.h:213: static const bool
kServiceIsCreatedWithBrowserContext = false;
I'm not very sure about this change, but without this tests were crashing with
the following trace.

#2 0x7f6c08a9bbb0 <unknown>
#3 0x00000085c115 extensions::ExtensionSystem::Get()
#4 0x00000081eb82 extensions::AlarmManager::AlarmManager()
#5 0x00000081ec41
extensions::BrowserContextKeyedAPIFactory<>::BuildServiceInstanceFor()
#6 0x7f6c0eae2ace KeyedServiceFactory::GetServiceForContext()
#7 0x7f6c0eae2155 DependencyManager::CreateContextServices()
#8 0x7f6c0ebd9480
BrowserContextDependencyManager::DoCreateBrowserContextServices()
#9 0x0000007478e9 extensions::ApiUnitTest::SetUp()
#10 0x00000047affc extensions::ExtensionAlarmsTest::SetUp()
#11 0x0000006d990d testing::Test::Run()

[James Cook, 2015-01-17 00:51:29]

Looking better!

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
File extensions/browser/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
extensions/browser/api/alarms/alarm_manager.h:213: static const bool
kServiceIsCreatedWithBrowserContext = false;
On 2015/01/16 23:32:28, babu wrote:
> I'm not very sure about this change, but without this tests were crashing with
> the following trace.
> 
> #2 0x7f6c08a9bbb0 <unknown>
> #3 0x00000085c115 extensions::ExtensionSystem::Get()
> #4 0x00000081eb82 extensions::AlarmManager::AlarmManager()
> #5 0x00000081ec41
> extensions::BrowserContextKeyedAPIFactory<>::BuildServiceInstanceFor()
> #6 0x7f6c0eae2ace KeyedServiceFactory::GetServiceForContext()
> #7 0x7f6c0eae2155 DependencyManager::CreateContextServices()
> #8 0x7f6c0ebd9480
> BrowserContextDependencyManager::DoCreateBrowserContextServices()
> #9 0x0000007478e9 extensions::ApiUnitTest::SetUp()
> #10 0x00000047affc extensions::ExtensionAlarmsTest::SetUp()
> #11 0x0000006d990d testing::Test::Run()

It's unclear to me that this change is safe. It changes the creation time of
AlarmManager and I'm not sure what effect that has in Chrome. If the suggestion
below doesn't help we can try to figure out if it's OK to lazily create this
service.  It does something to "StateStore" in the constructor I'm not familiar
with.

That said, I think you just found a bug in ApiUnitTest! ApiUnitTest::SetUp()
calls ExtensionsTest::SetUp() which tries to build BrowserContextKeyedServices,
but there's no ExtensionSystemFactory yet.  Can you try moving the call to
set_extension_system_factory() into the ApiUnitTest constructor and see what
happens?

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

You may discover you need a fancier ExtensionSystem than the MockExtensionSystem
provided by ExtensionsTest, but maybe the one provided is sufficient.

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_t...
File extensions/browser/api_test_utils.cc (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_t...
extensions/browser/api_test_utils.cc:140: scoped_ptr<base::DictionaryValue>
test_extension_value(
This looks like it is copied from //chrome. Can you either:
* Move the callers in //chrome to call this copy, or
* Make the implementation in //chrome call this one?

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
File extensions/browser/api_unittest.cc (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
extensions/browser/api_unittest.cc:111: function->set_extension(extension());
Should this do the same if (content_) also?  I think the version in //chrome
does that.

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
File extensions/browser/api_unittest.h (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
extensions/browser/api_unittest.h:56: void CreateWebContents();
Is there any particular reason not to call this CreateBackgroundPage()?  Keeping
the name might make it easier to move other tests in the future, and helps new
authors realize this is the same method.

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
extensions/browser/api_unittest.h:97: // or NULL.
nit: "null" in comments, since we don't use the NULL macro any more.

[babu, 2015-01-17 08:50:50]

Patchset #4 (id:60001) has been deleted

[babu, 2015-01-17 09:16:24]

Patchset #4 (id:80001) has been deleted

[babu, 2015-01-17 09:54:38]

Patchset #4 (id:100001) has been deleted

[babu, 2015-01-17 11:53:56]

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
File extensions/browser/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
extensions/browser/api/alarms/alarm_manager.h:213: static const bool
kServiceIsCreatedWithBrowserContext = false;
On 2015/01/17 00:51:28, James Cook wrote:
> On 2015/01/16 23:32:28, babu wrote:
> > I'm not very sure about this change, but without this tests were crashing
with
> > the following trace.
> > 
> > #2 0x7f6c08a9bbb0 <unknown>
> > #3 0x00000085c115 extensions::ExtensionSystem::Get()
> > #4 0x00000081eb82 extensions::AlarmManager::AlarmManager()
> > #5 0x00000081ec41
> > extensions::BrowserContextKeyedAPIFactory<>::BuildServiceInstanceFor()
> > #6 0x7f6c0eae2ace KeyedServiceFactory::GetServiceForContext()
> > #7 0x7f6c0eae2155 DependencyManager::CreateContextServices()
> > #8 0x7f6c0ebd9480
> > BrowserContextDependencyManager::DoCreateBrowserContextServices()
> > #9 0x0000007478e9 extensions::ApiUnitTest::SetUp()
> > #10 0x00000047affc extensions::ExtensionAlarmsTest::SetUp()
> > #11 0x0000006d990d testing::Test::Run()
> 
> It's unclear to me that this change is safe. It changes the creation time of
> AlarmManager and I'm not sure what effect that has in Chrome. If the
suggestion
> below doesn't help we can try to figure out if it's OK to lazily create this
> service.  It does something to "StateStore" in the constructor I'm not
familiar
> with.
> 
> That said, I think you just found a bug in ApiUnitTest! ApiUnitTest::SetUp()
> calls ExtensionsTest::SetUp() which tries to build
BrowserContextKeyedServices,
> but there's no ExtensionSystemFactory yet.  Can you try moving the call to
> set_extension_system_factory() into the ApiUnitTest constructor and see what
> happens?

Thanks for the suggestion, it fixes the problem.

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_t...
File extensions/browser/api_test_utils.cc (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_t...
extensions/browser/api_test_utils.cc:140: scoped_ptr<base::DictionaryValue>
test_extension_value(
On 2015/01/17 00:51:28, James Cook wrote:
> This looks like it is copied from //chrome. Can you either:
> * Move the callers in //chrome to call this copy, or
> * Make the implementation in //chrome call this one?

Done.

Moved this helper function from extension_function_test_utils.h to
api_test_utils.h.

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
File extensions/browser/api_unittest.cc (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
extensions/browser/api_unittest.cc:111: function->set_extension(extension());
On 2015/01/17 00:51:29, James Cook wrote:
> Should this do the same if (content_) also?  I think the version in //chrome
> does that.

I missed this. It's fixed now.

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
File extensions/browser/api_unittest.h (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
extensions/browser/api_unittest.h:56: void CreateWebContents();
On 2015/01/17 00:51:29, James Cook wrote:
> Is there any particular reason not to call this CreateBackgroundPage()? 
Keeping
> the name might make it easier to move other tests in the future, and helps new
> authors realize this is the same method.

Done. 

Changed to the original name and removed the function from
extension_api_unittest.h since no other tests are using it.

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api_u...
extensions/browser/api_unittest.h:97: // or NULL.
On 2015/01/17 00:51:29, James Cook wrote:
> nit: "null" in comments, since we don't use the NULL macro any more.

Done.

[James Cook, 2015-01-20 17:54:14]

LGTM. Nice patch!

https://codereview.chromium.org/789643004/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_function_test_utils.h (left):

https://codereview.chromium.org/789643004/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_function_test_utils.h:46:
extensions::Manifest::Location location);
Hooray, it's great that this file is getting smaller!

[babu, 2015-01-20 21:26:22]

sudarsana.nagineni@intel.com changed reviewers:
+ thestig@chromium.org

[babu, 2015-01-21 08:08:37]

+thestig@ for chrome/

[Lei Zhang, 2015-01-21 09:09:41]

chrome/ lgtm, but you may need to update
chrome/common/extensions/docs/server2/platform_bundle_test.py since it
references alarms.idl. +kalman, +rockot

[Ken Rockot(use gerrit already), 2015-01-21 16:51:36]

On 2015/01/21 09:09:41, Lei Zhang wrote:
> chrome/ lgtm, but you may need to update
> chrome/common/extensions/docs/server2/platform_bundle_test.py since it
> references alarms.idl. +kalman, +rockot

Good catch. There are actually two affected docserver tests. Here's a diff you
can apply to this CL:

diff --git a/chrome/common/extensions/docs/server2/api_models_test.py
b/chrome/common/extensions/docs/server2/api_models_test.py
index f9c863e4..4d96632 100755
--- a/chrome/common/extensions/docs/server2/api_models_test.py
+++ b/chrome/common/extensions/docs/server2/api_models_test.py
@@ -8,7 +8,8 @@ import unittest
 
 from api_models import APIModels
 from compiled_file_system import CompiledFileSystem
-from extensions_paths import API_PATHS, CHROME_API, CHROME_EXTENSIONS
+from extensions_paths import (API_PATHS, CHROME_API, CHROME_EXTENSIONS,
+    EXTENSIONS_API)
 from features_bundle import FeaturesBundle
 from file_system import FileNotFoundError
 from mock_file_system import MockFileSystem
@@ -38,7 +39,7 @@ _TEST_DATA = {
     }),
     '_manifest_features.json': '{}',
     '_permission_features.json': '{}',
-    'alarms.idl': ReadFile(CHROME_API, 'alarms.idl'),
+    'alarms.idl': ReadFile(EXTENSIONS_API, 'alarms.idl'),
     'input_ime.json': ReadFile(CHROME_API, 'input_ime.json'),
     'page_action.json': ReadFile(CHROME_API, 'page_action.json'),
   },
diff --git a/chrome/common/extensions/docs/server2/platform_bundle_test.py
b/chrome/common/extensions/docs/server2/platform_bundle_test.py
index fefecf6..6a8f4a6 100755
--- a/chrome/common/extensions/docs/server2/platform_bundle_test.py
+++ b/chrome/common/extensions/docs/server2/platform_bundle_test.py
@@ -6,7 +6,7 @@
 import json
 import unittest
 
-from extensions_paths import CHROME_API, CHROME_EXTENSIONS
+from extensions_paths import CHROME_API, CHROME_EXTENSIONS, EXTENSIONS_API
 from mock_file_system import MockFileSystem
 from server_instance import ServerInstance
 from test_file_system import TestFileSystem
@@ -32,7 +32,7 @@ _TEST_DATA = {
     }),
     '_manifest_features.json': '{}',
     '_permission_features.json': '{}',
-    'alarms.idl': ReadFile(CHROME_API, 'alarms.idl'),
+    'alarms.idl': ReadFile(EXTENSIONS_API, 'alarms.idl'),
     'input_ime.json': ReadFile(CHROME_API, 'input_ime.json'),
     'page_action.json': ReadFile(CHROME_API, 'page_action.json'),
   },

[babu, 2015-01-21 22:20:15]

sudarsana.nagineni@intel.com changed reviewers:
+ rockot@chromium.org

[babu, 2015-01-21 22:22:22]

On 2015/01/21 16:51:36, Ken Rockot wrote:
> On 2015/01/21 09:09:41, Lei Zhang wrote:
> > chrome/ lgtm, but you may need to update
> > chrome/common/extensions/docs/server2/platform_bundle_test.py since it
> > references alarms.idl. +kalman, +rockot
> 
> Good catch. There are actually two affected docserver tests. Here's a diff you
> can apply to this CL:

Thanks, Ken, for the fix. Updated CL with the diff you provided.

[babu, 2015-01-22 06:15:56]

yoz@, could you please take a look at //extensions?

[Yoyo Zhou, 2015-01-23 06:22:40]

extensions LGTM

[babu, 2015-01-27 08:43:41]

The CQ bit was checked by sudarsana.nagineni@intel.com

[commit-bot: I haz the power, 2015-01-27 08:44:17]

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

[commit-bot: I haz the power, 2015-01-27 09:24:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-27 09:24:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[James Cook, 2015-01-27 15:59:58]

On 2015/01/27 09:24:07, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Hmm, looks like some kind of docserver problem.

yoz, rockot, ideas?

[Ken Rockot(use gerrit already), 2015-01-27 16:10:44]

rockot@chromium.org changed reviewers:
+ mkearney@chromium.org

[Ken Rockot(use gerrit already), 2015-01-27 16:12:36]

+Meggin: Didn't you change some redirect(s) recently, or am I remembering
incorrectly? The failing docserver test in this case is getting an unexpected
redirect from /extensions.

[chromium-reviews, 2015-01-27 17:28:50]

Yes, I changed a redirect recently. I'll take a look.

On Tue, Jan 27, 2015 at 8:12 AM, <rockot@chromium.org> wrote:

> +Meggin: Didn't you change some redirect(s) recently, or am I remembering
> incorrectly? The failing docserver test in this case is getting an
> unexpected
> redirect from /extensions.
>
> https://codereview.chromium.org/789643004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkearney1, 2015-01-28 00:17:51]

On 2015/01/27 17:28:50, chromium-reviews wrote:
> Yes, I changed a redirect recently. I'll take a look.
> 
> On Tue, Jan 27, 2015 at 8:12 AM, <mailto:rockot@chromium.org> wrote:
> 
> > +Meggin: Didn't you change some redirect(s) recently, or am I remembering
> > incorrectly? The failing docserver test in this case is getting an
> > unexpected
> > redirect from /extensions.
> >
> > https://codereview.chromium.org/789643004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hey, guys

This cl should fix the problem and also fix the left-side navigation:
https://codereview.chromium.org/789643004/

Two-birds and all that.

[babu, 2015-01-29 16:09:35]

On 2015/01/28 00:17:51, mkearney1 wrote:
> On 2015/01/27 17:28:50, chromium-reviews wrote:
> > Yes, I changed a redirect recently. I'll take a look.
> > 
> > On Tue, Jan 27, 2015 at 8:12 AM, <mailto:rockot@chromium.org> wrote:
> > 
> > > +Meggin: Didn't you change some redirect(s) recently, or am I remembering
> > > incorrectly? The failing docserver test in this case is getting an
> > > unexpected
> > > redirect from /extensions.
> > >
> > > https://codereview.chromium.org/789643004/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Hey, guys
> 
> This cl should fix the problem and also fix the left-side navigation:
> https://codereview.chromium.org/789643004/
> 
> Two-birds and all that.

@mkearney1, I guess the fix you made was landed in
https://codereview.chromium.org/883963002/, but two docserver tests are still
failing.

http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...
- FAIL: testIndexRender (__main__.RenderServletTest)
- FAIL: testCronAndPublicFiles (__main__.IntegrationTest)

[Ken Rockot(use gerrit already), 2015-01-29 17:59:48]

On 2015/01/29 16:09:35, babu wrote:
> On 2015/01/28 00:17:51, mkearney1 wrote:
> > On 2015/01/27 17:28:50, chromium-reviews wrote:
> > > Yes, I changed a redirect recently. I'll take a look.
> > > 
> > > On Tue, Jan 27, 2015 at 8:12 AM, <mailto:rockot@chromium.org> wrote:
> > > 
> > > > +Meggin: Didn't you change some redirect(s) recently, or am I
remembering
> > > > incorrectly? The failing docserver test in this case is getting an
> > > > unexpected
> > > > redirect from /extensions.
> > > >
> > > > https://codereview.chromium.org/789643004/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > Hey, guys
> > 
> > This cl should fix the problem and also fix the left-side navigation:
> > https://codereview.chromium.org/789643004/
> > 
> > Two-birds and all that.
> 
> @mkearney1, I guess the fix you made was landed in
> https://codereview.chromium.org/883963002/, but two docserver tests are still
> failing.
> 
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...
> - FAIL: testIndexRender (__main__.RenderServletTest)
> - FAIL: testCronAndPublicFiles (__main__.IntegrationTest)

Sorry for all this trouble, we're looking into the failures and hope to have a
fix soon. Will keep you posted here.

[chromium-reviews, 2015-01-29 19:15:47]

Yeah, so sorry about this. The weird thing is that we were able to put a
fix in that didn't fail and fixed a bug. We're investigating the
possibility of reworking the test a bit. But we want to make sure it's the
right thing to do. If it turns out it isn't, I'll roll back my bug fix
today so you guys at least aren't held up!

On Thu, Jan 29, 2015 at 9:59 AM, <rockot@chromium.org> wrote:

> On 2015/01/29 16:09:35, babu wrote:
>
>> On 2015/01/28 00:17:51, mkearney1 wrote:
>> > On 2015/01/27 17:28:50, chromium-reviews wrote:
>> > > Yes, I changed a redirect recently. I'll take a look.
>> > >
>> > > On Tue, Jan 27, 2015 at 8:12 AM, <mailto:rockot@chromium.org> wrote:
>> > >
>> > > > +Meggin: Didn't you change some redirect(s) recently, or am I
>>
> remembering
>
>> > > > incorrectly? The failing docserver test in this case is getting an
>> > > > unexpected
>> > > > redirect from /extensions.
>> > > >
>> > > > https://codereview.chromium.org/789643004/
>> > > >
>> > >
>> > > To unsubscribe from this group and stop receiving emails from it,
>> send an
>> > email
>> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >
>> > Hey, guys
>> >
>> > This cl should fix the problem and also fix the left-side navigation:
>> > https://codereview.chromium.org/789643004/
>> >
>> > Two-birds and all that.
>>
>
>  @mkearney1, I guess the fix you made was landed in
>> https://codereview.chromium.org/883963002/, but two docserver tests are
>> still
>> failing.
>>
>
>
> http://build.chromium.org/p/tryserver.chromium.linux/
> builders/chromium_presubmit/builds/39152/steps/presubmit/logs/stdio
>
>> - FAIL: testIndexRender (__main__.RenderServletTest)
>> - FAIL: testCronAndPublicFiles (__main__.IntegrationTest)
>>
>
> Sorry for all this trouble, we're looking into the failures and hope to
> have a
> fix soon. Will keep you posted here.
>
> https://codereview.chromium.org/789643004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-01-30 00:52:08]

Hi, everyone

I put through a fix, which worked, but only part of the way. One test is
passing, the other is still falling over. I need to update it based on the
changes I made. The integration test takes awhile to run. And I'd like to
head home to have dinner with the family and do the night time routine
(sorry for so much information). I'll try pushing this through tonight once
the kids are asleep and then start again in the morning. Trying. Making
progress. But still not there yet.

On Thu, Jan 29, 2015 at 11:15 AM, Meggin Kearney <mkearney@google.com>
wrote:

> Yeah, so sorry about this. The weird thing is that we were able to put a
> fix in that didn't fail and fixed a bug. We're investigating the
> possibility of reworking the test a bit. But we want to make sure it's the
> right thing to do. If it turns out it isn't, I'll roll back my bug fix
> today so you guys at least aren't held up!
>
> On Thu, Jan 29, 2015 at 9:59 AM, <rockot@chromium.org> wrote:
>
>> On 2015/01/29 16:09:35, babu wrote:
>>
>>> On 2015/01/28 00:17:51, mkearney1 wrote:
>>> > On 2015/01/27 17:28:50, chromium-reviews wrote:
>>> > > Yes, I changed a redirect recently. I'll take a look.
>>> > >
>>> > > On Tue, Jan 27, 2015 at 8:12 AM, <mailto:rockot@chromium.org> wrote:
>>> > >
>>> > > > +Meggin: Didn't you change some redirect(s) recently, or am I
>>>
>> remembering
>>
>>> > > > incorrectly? The failing docserver test in this case is getting an
>>> > > > unexpected
>>> > > > redirect from /extensions.
>>> > > >
>>> > > > https://codereview.chromium.org/789643004/
>>> > > >
>>> > >
>>> > > To unsubscribe from this group and stop receiving emails from it,
>>> send an
>>> > email
>>> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>> >
>>> > Hey, guys
>>> >
>>> > This cl should fix the problem and also fix the left-side navigation:
>>> > https://codereview.chromium.org/789643004/
>>> >
>>> > Two-birds and all that.
>>>
>>
>>  @mkearney1, I guess the fix you made was landed in
>>> https://codereview.chromium.org/883963002/, but two docserver tests are
>>> still
>>> failing.
>>>
>>
>>
>> http://build.chromium.org/p/tryserver.chromium.linux/
>> builders/chromium_presubmit/builds/39152/steps/presubmit/logs/stdio
>>
>>> - FAIL: testIndexRender (__main__.RenderServletTest)
>>> - FAIL: testCronAndPublicFiles (__main__.IntegrationTest)
>>>
>>
>> Sorry for all this trouble, we're looking into the failures and hope to
>> have a
>> fix soon. Will keep you posted here.
>>
>> https://codereview.chromium.org/789643004/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[babu, 2015-01-30 10:29:05]

Meggin and Ken, thanks a lot for looking into those test failures - very much
appreciated!

On 2015/01/30 00:52:08, chromium-reviews wrote:
> Hi, everyone
> 
> I put through a fix, which worked, but only part of the way. One test is
> passing, the other is still falling over. I need to update it based on the
> changes I made. The integration test takes awhile to run. And I'd like to
> head home to have dinner with the family and do the night time routine
> (sorry for so much information). I'll try pushing this through tonight once
> the kids are asleep and then start again in the morning. Trying. Making
> progress. But still not there yet.
> 
> On Thu, Jan 29, 2015 at 11:15 AM, Meggin Kearney <mailto:mkearney@google.com>
> wrote:
> 
> > Yeah, so sorry about this. The weird thing is that we were able to put a
> > fix in that didn't fail and fixed a bug. We're investigating the
> > possibility of reworking the test a bit. But we want to make sure it's the
> > right thing to do. If it turns out it isn't, I'll roll back my bug fix
> > today so you guys at least aren't held up!
> >
> > On Thu, Jan 29, 2015 at 9:59 AM, <mailto:rockot@chromium.org> wrote:
> >
> >> On 2015/01/29 16:09:35, babu wrote:
> >>
> >>> On 2015/01/28 00:17:51, mkearney1 wrote:
> >>> > On 2015/01/27 17:28:50, chromium-reviews wrote:
> >>> > > Yes, I changed a redirect recently. I'll take a look.
> >>> > >
> >>> > > On Tue, Jan 27, 2015 at 8:12 AM, <mailto:rockot@chromium.org> wrote:
> >>> > >
> >>> > > > +Meggin: Didn't you change some redirect(s) recently, or am I
> >>>
> >> remembering
> >>
> >>> > > > incorrectly? The failing docserver test in this case is getting an
> >>> > > > unexpected
> >>> > > > redirect from /extensions.
> >>> > > >
> >>> > > > https://codereview.chromium.org/789643004/
> >>> > > >
> >>> > >
> >>> > > To unsubscribe from this group and stop receiving emails from it,
> >>> send an
> >>> > email
> >>> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>> >
> >>> > Hey, guys
> >>> >
> >>> > This cl should fix the problem and also fix the left-side navigation:
> >>> > https://codereview.chromium.org/789643004/
> >>> >
> >>> > Two-birds and all that.
> >>>
> >>
> >>  @mkearney1, I guess the fix you made was landed in
> >>> https://codereview.chromium.org/883963002/, but two docserver tests are
> >>> still
> >>> failing.
> >>>
> >>
> >>
> >> http://build.chromium.org/p/tryserver.chromium.linux/
> >> builders/chromium_presubmit/builds/39152/steps/presubmit/logs/stdio
> >>
> >>> - FAIL: testIndexRender (__main__.RenderServletTest)
> >>> - FAIL: testCronAndPublicFiles (__main__.IntegrationTest)
> >>>
> >>
> >> Sorry for all this trouble, we're looking into the failures and hope to
> >> have a
> >> fix soon. Will keep you posted here.
> >>
> >> https://codereview.chromium.org/789643004/

[chromium-reviews, 2015-01-30 20:55:47]

Hey, guys

Sorry this took so long-- I finally got a success on the integration tests
and I was able to commit the cl with the fixes.

So I'm hoping this means this cl can land without issues... fingers crossed.

Meggin

On Fri, Jan 30, 2015 at 2:29 AM, <sudarsana.nagineni@intel.com> wrote:

> Meggin and Ken, thanks a lot for looking into those test failures - very
> much
> appreciated!
>
> On 2015/01/30 00:52:08, chromium-reviews wrote:
>
>> Hi, everyone
>>
>
>  I put through a fix, which worked, but only part of the way. One test is
>> passing, the other is still falling over. I need to update it based on the
>> changes I made. The integration test takes awhile to run. And I'd like to
>> head home to have dinner with the family and do the night time routine
>> (sorry for so much information). I'll try pushing this through tonight
>> once
>> the kids are asleep and then start again in the morning. Trying. Making
>> progress. But still not there yet.
>>
>
>  On Thu, Jan 29, 2015 at 11:15 AM, Meggin Kearney <mailto:
>> mkearney@google.com>
>> wrote:
>>
>
>  > Yeah, so sorry about this. The weird thing is that we were able to put a
>> > fix in that didn't fail and fixed a bug. We're investigating the
>> > possibility of reworking the test a bit. But we want to make sure it's
>> the
>> > right thing to do. If it turns out it isn't, I'll roll back my bug fix
>> > today so you guys at least aren't held up!
>> >
>> > On Thu, Jan 29, 2015 at 9:59 AM, <mailto:rockot@chromium.org> wrote:
>> >
>> >> On 2015/01/29 16:09:35, babu wrote:
>> >>
>> >>> On 2015/01/28 00:17:51, mkearney1 wrote:
>> >>> > On 2015/01/27 17:28:50, chromium-reviews wrote:
>> >>> > > Yes, I changed a redirect recently. I'll take a look.
>> >>> > >
>> >>> > > On Tue, Jan 27, 2015 at 8:12 AM, <mailto:rockot@chromium.org>
>> wrote:
>> >>> > >
>> >>> > > > +Meggin: Didn't you change some redirect(s) recently, or am I
>> >>>
>> >> remembering
>> >>
>> >>> > > > incorrectly? The failing docserver test in this case is getting
>> an
>> >>> > > > unexpected
>> >>> > > > redirect from /extensions.
>> >>> > > >
>> >>> > > > https://codereview.chromium.org/789643004/
>> >>> > > >
>> >>> > >
>> >>> > > To unsubscribe from this group and stop receiving emails from it,
>> >>> send an
>> >>> > email
>> >>> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >>> >
>> >>> > Hey, guys
>> >>> >
>> >>> > This cl should fix the problem and also fix the left-side
>> navigation:
>> >>> > https://codereview.chromium.org/789643004/
>> >>> >
>> >>> > Two-birds and all that.
>> >>>
>> >>
>> >>  @mkearney1, I guess the fix you made was landed in
>> >>> https://codereview.chromium.org/883963002/, but two docserver tests
>> are
>> >>> still
>> >>> failing.
>> >>>
>> >>
>> >>
>> >> http://build.chromium.org/p/tryserver.chromium.linux/
>> >> builders/chromium_presubmit/builds/39152/steps/presubmit/logs/stdio
>> >>
>> >>> - FAIL: testIndexRender (__main__.RenderServletTest)
>> >>> - FAIL: testCronAndPublicFiles (__main__.IntegrationTest)
>> >>>
>> >>
>> >> Sorry for all this trouble, we're looking into the failures and hope to
>> >> have a
>> >> fix soon. Will keep you posted here.
>> >>
>> >> https://codereview.chromium.org/789643004/
>>
>
> https://codereview.chromium.org/789643004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2015-01-30 23:34:08]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2015-01-30 23:35:34]

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

[commit-bot: I haz the power, 2015-01-31 00:26:50]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-01-31 00:28:02]

Patchset 7 (id:??) landed as
https://crrev.com/745ff1db8cca6a4fdc0c19cf3edab909bd532457
Cr-Commit-Position: refs/heads/master@{#314040}

[Lei Zhang, 2015-02-02 22:58:30]

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
File extensions/browser/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/789643004/diff/40001/extensions/browser/api/a...
extensions/browser/api/alarms/alarm_manager.h:213: static const bool
kServiceIsCreatedWithBrowserContext = false;
On 2015/01/17 11:53:56, babu wrote:
> On 2015/01/17 00:51:28, James Cook wrote:
> > On 2015/01/16 23:32:28, babu wrote:
> > > I'm not very sure about this change, but without this tests were crashing
> with
> > > the following trace.
> > > 
> > > #2 0x7f6c08a9bbb0 <unknown>
> > > #3 0x00000085c115 extensions::ExtensionSystem::Get()
> > > #4 0x00000081eb82 extensions::AlarmManager::AlarmManager()
> > > #5 0x00000081ec41
> > > extensions::BrowserContextKeyedAPIFactory<>::BuildServiceInstanceFor()
> > > #6 0x7f6c0eae2ace KeyedServiceFactory::GetServiceForContext()
> > > #7 0x7f6c0eae2155 DependencyManager::CreateContextServices()
> > > #8 0x7f6c0ebd9480
> > > BrowserContextDependencyManager::DoCreateBrowserContextServices()
> > > #9 0x0000007478e9 extensions::ApiUnitTest::SetUp()
> > > #10 0x00000047affc extensions::ExtensionAlarmsTest::SetUp()
> > > #11 0x0000006d990d testing::Test::Run()
> > 
> > It's unclear to me that this change is safe. It changes the creation time of
> > AlarmManager and I'm not sure what effect that has in Chrome. If the
> suggestion
> > below doesn't help we can try to figure out if it's OK to lazily create this
> > service.  It does something to "StateStore" in the constructor I'm not
> familiar
> > with.
> > 
> > That said, I think you just found a bug in ApiUnitTest! ApiUnitTest::SetUp()
> > calls ExtensionsTest::SetUp() which tries to build
> BrowserContextKeyedServices,
> > but there's no ExtensionSystemFactory yet.  Can you try moving the call to
> > set_extension_system_factory() into the ApiUnitTest constructor and see what
> > happens?
> 
> Thanks for the suggestion, it fixes the problem.

But it only fixes the problem for ExtensionAlarmsSchedulingTest it seems. The
tests that run after it still crashes. I've filed http://crbug.com/454554 and
I'm trying out avoiding to call ExtensionSystem::Get() in the AlarmManager ctor.