No longer start BG mode until a BackgroundContents is loaded

No longer start BG mode until a BackgroundContents is loaded

BUG=120448
TEST=Install hosted app, ensure it does not kick off background mode until it opens a background contents via JS.


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

[Andrew T Wilson (Slow), 2012-05-02 01:30:13]

Mihai, PTAL. I've tested this manually and most of the tests are working, except
for one new test that isn't doing what I expect. It's close enough that I'd like
to have you take a look at it while I debug the last test.

[Andrew T Wilson (Slow), 2012-05-03 00:09:25]

Erik, it looks like Mihai is OOO. Can you take a look at this, or forward to
someone else on the extensions team who has some cycles?

[Mihai Parparita -not on Chrome, 2012-05-03 00:14:49]

Actually, I'm back from leave. I'll take a look at this now.

Mihai

On Wed, May 2, 2012 at 5:09 PM, <atwilson@chromium.org> wrote:

> Erik, it looks like Mihai is OOO. Can you take a look at this, or forward
> to
> someone else on the extensions team who has some cycles?
>
>
http://codereview.chromium.**org/10298002/<http://codereview.chromium.org/102...
>

[Andrew T Wilson (Slow), 2012-05-03 00:17:07]

Awesome, thanks - you're the ideal reviewer since you have more experience with
this part of the code base.

[Mihai Parparita -not on Chrome, 2012-05-03 00:54:11]

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_application_list_model.cc (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_application_list_model.cc:273: //
Extensions with background permission are always treated as background
Nit: "Extensions and packaged apps"

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_application_list_model_unittest.cc
(right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_application_list_model_unittest.cc:87: if
(BackgroundApplicationListModel::IsBackgroundApp(*extension,
Nit: You may want to make this into a IsBackgroundApp(extension) protected
helper method in BackgroundApplicationListModelTest.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_contents_service.cc (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_contents_service.cc:256:
SendChangeNotification(profile);
Why would a navigation cause the list of background contents to change?

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_contents_service.cc:537: // Walk our
dictionary looking for any entries with this key.
Why not use contents->HasKey(app)?

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_contents_service.h (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_contents_service.h:60: bool
HasRegisteredBackgroundContents(const string16& appid);
Beyond the scope of this CL, but why are app IDs stored as string16 here? In all
other cases, extension/app IDs are stored as std::strings.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager.h (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.h:68: friend class
AppBackgroundPageApiTest;
Given this friend declaration, does IsBackgroundModeActiveForTest need to be
public?

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/app_background_page/basic_close/test.js
(right):

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/app_background_page/basic_close/test.js:14:
var pageCloser;
This seems unused (it's set, but never read).

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/app_background_page/basic_close/test.js:38:
var close_url =
Nit: this should be called closeUrl.

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/app_background_page/common/close.html
(right):

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/app_background_page/common/close.html:20:
alert("foo");
This should be removed.

[Mihai Parparita -not on Chrome, 2012-05-03 00:56:38]

BTW, thanks for taking this on. It ended up being quite clean to implement the
desired behavior.

[Andrew T Wilson (Slow), 2012-05-03 07:42:56]

PTAL. I addressed your feedback, and also made some changes to the tests to make
them less flaky/race-y (since closing background contents does not happen
instantaneously, it can take some time after calling window.close() for chrome
to shift out of background mode, so now the tests wait for chrome to transition
to the expected mode rather than failing out prematurely.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_application_list_model.cc (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_application_list_model.cc:273: //
Extensions with background permission are always treated as background
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> Nit: "Extensions and packaged apps"

Done.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_application_list_model_unittest.cc
(right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_application_list_model_unittest.cc:87: if
(BackgroundApplicationListModel::IsBackgroundApp(*extension,
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> Nit: You may want to make this into a IsBackgroundApp(extension) protected
> helper method in BackgroundApplicationListModelTest.

Done.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_contents_service.cc (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_contents_service.cc:256:
SendChangeNotification(profile);
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> Why would a navigation cause the list of background contents to change?

It doesn't change the list of background content *objects*, but it *does* change
the list of registered background content URLs (we don't register them until
they have navigated, because that's the first time we actually get the URL).

But, we probably don't need to send a change notification in this case (I added
this originally when IsBackgroundApp() was going to only look at registered
URLs), so I'll remove this.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_contents_service.cc:537: // Walk our
dictionary looking for any entries with this key.
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> Why not use contents->HasKey(app)?

Ah, good idea. I should've checked DictionaryValue for a useful API first.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_contents_service.h (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_contents_service.h:60: bool
HasRegisteredBackgroundContents(const string16& appid);
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> Beyond the scope of this CL, but why are app IDs stored as string16 here? In
all
> other cases, extension/app IDs are stored as std::strings.

Yeah, at one point a few years back when writing this, we (chrome team) were
going to convert to using string16 everywhere, and new APIs were supposed to use
string16. So that's why we added this as string16 - not sure if this is still a
goal or not.

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager.h (right):

http://codereview.chromium.org/10298002/diff/7001/chrome/browser/background/b...
chrome/browser/background/background_mode_manager.h:68: friend class
AppBackgroundPageApiTest;
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> Given this friend declaration, does IsBackgroundModeActiveForTest need to be
> public?

Agreed, it can be private.

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/app_background_page/basic_close/test.js
(right):

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/app_background_page/basic_close/test.js:14:
var pageCloser;
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> This seems unused (it's set, but never read).

Done.

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/app_background_page/basic_close/test.js:38:
var close_url =
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> Nit: this should be called closeUrl.

Done.

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/app_background_page/common/close.html
(right):

http://codereview.chromium.org/10298002/diff/7001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/app_background_page/common/close.html:20:
alert("foo");
On 2012/05/03 00:54:11, Mihai Parparita wrote:
> This should be removed.

Done.

[Mihai Parparita -not on Chrome, 2012-05-03 19:32:35]

LGTM

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/background/...
File chrome/browser/background/background_mode_manager.cc (right):

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/background/...
chrome/browser/background/background_mode_manager.cc:546: bool new_value = true;
Why not use &in_background_mode_?

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/background/...
chrome/browser/background/background_mode_manager.cc:570: bool new_value =
false;
Same here.

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/app_background_page_apitest.cc:57: this,
chrome::NOTIFICATION_BACKGROUND_MODE_CHANGED,
Does this end up firing more than once, before it gets to the expected value, or
can you just use WindowedNotificationObserverWithDetails?

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/app_background_page_apitest.cc:433: 
Nit: extra newline.

[Andrew T Wilson (Slow), 2012-05-03 21:36:58]

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/background/...
File chrome/browser/background/background_mode_manager.cc (right):

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/background/...
chrome/browser/background/background_mode_manager.cc:546: bool new_value = true;
On 2012/05/03 19:32:35, Mihai Parparita wrote:
> Why not use &in_background_mode_?

Good idea - heh, the dangers of writing code too late at night :)

http://codereview.chromium.org/10298002/diff/18001/chrome/browser/background/...
chrome/browser/background/background_mode_manager.cc:570: bool new_value =
false;
On 2012/05/03 19:32:35, Mihai Parparita wrote:
> Same here.

Done.

[commit-bot: I haz the power, 2012-05-03 23:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/10298002/22001

[commit-bot: I haz the power, 2012-05-04 01:17:30]

Change committed as 135264