Make calculator app handle onRestarted events.

Make calculator app handle onRestarted events.


BUG=162057
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173079

[dharcourt, 2012-12-12 06:55:18]

By the way, I'm not a committer yet so you should pick someone like jeremya or
benwells as a reviewer to get a useful L*TM. I've removed my name from the
reviewers line but remain in the CC's to follow any updates.

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/apps/calculator/app/controller.js
(right):

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/apps/calculator/app/controller.js:13:
function launchApp() {
This would be better with a function expression instead of a function
declaration for launchApp. See http://goo.gl/aYaGp and http://goo.gl/r4py.

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/apps/calculator/app/controller.js:34:
launchApp();
Why is this necessary? Are we sending onRestarted to applications that had no
windows open before the restart? We shouldn't, otherwise every app developer
will have to implement code like this--and since they won't users will have to
live with applications that mysteriously get relaunched on restart :-).

If we fix onRestarted then it should be possible to simplify this greatly.

[jeremya, 2012-12-12 07:00:28]

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/apps/calculator/app/controller.js
(right):

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/apps/calculator/app/controller.js:34:
launchApp();
On 2012/12/12 06:55:18, dharcourt wrote:
> Why is this necessary? Are we sending onRestarted to applications that had no
> windows open before the restart? We shouldn't, otherwise every app developer
> will have to implement code like this--and since they won't users will have to
> live with applications that mysteriously get relaunched on restart :-).
> 
> If we fix onRestarted then it should be possible to simplify this greatly.

FWIW I agree with Charles here :) This code seems kinda odd.

[koz (OOO until 15th September), 2012-12-12 23:22:19]

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/apps/calculator/app/controller.js
(right):

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/apps/calculator/app/controller.js:13:
function launchApp() {
On 2012/12/12 06:55:18, dharcourt wrote:
> This would be better with a function expression instead of a function
> declaration for launchApp. See http://goo.gl/aYaGp and http://goo.gl/r4py.

Cool, I've learned something new. Fixed.

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/apps/calculator/app/controller.js:34:
launchApp();
On 2012/12/12 07:00:28, jeremya wrote:
> On 2012/12/12 06:55:18, dharcourt wrote:
> > Why is this necessary? Are we sending onRestarted to applications that had
no
> > windows open before the restart? We shouldn't, otherwise every app developer
> > will have to implement code like this--and since they won't users will have
to
> > live with applications that mysteriously get relaunched on restart :-).
> > 
> > If we fix onRestarted then it should be possible to simplify this greatly.
> 
> FWIW I agree with Charles here :) This code seems kinda odd.

I know what you mean - I'm distressed by the number of things one needs to do in
order to, eg, make a multi-window app handle onRestarted() properly. Stateless,
singleton window apps like the calculator app are *almost* trivially made
restart-resistant, but in the general case it's not a trivial transformation.
I'm open to ideas for making the general case easier, but I think perhaps the
best solution to that problem hasn't been discovered yet.

For example, making onRestarted only get fired to apps that had windows open
when the restart occurred would mean that UI-less apps (eg: servers) don't get
onRestarted events. At the end of the day the lifecycle for apps is more complex
than "alive when windows are there, not alive when windows aren't" and I feel we
shouldn't try and hide that, otherwise surprises will happen.

(Also, some of the counter-intuitiveness for this code is due to poorly chosen
variable names, which I have changed.)

[dharcourt, 2012-12-13 16:44:12]

LGTM (but you'll need a committer to agree before you can check that checkbox
:-).

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/examples/apps/calculator/app/controller.js
(right):

https://codereview.chromium.org/11316346/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/apps/calculator/app/controller.js:34:
launchApp();
On 2012/12/12 23:22:19, koz wrote:
> On 2012/12/12 07:00:28, jeremya wrote:
> > On 2012/12/12 06:55:18, dharcourt wrote:
> > > Why is this necessary? Are we sending onRestarted to applications that had
> no
> > > windows open before the restart? We shouldn't, otherwise every app
developer
> > > will have to implement code like this--and since they won't users will
have
> to
> > > live with applications that mysteriously get relaunched on restart :-).
> > > 
> > > If we fix onRestarted then it should be possible to simplify this greatly.
> > 
> > FWIW I agree with Charles here :) This code seems kinda odd.
> 
> I know what you mean - I'm distressed by the number of things one needs to do
in
> order to, eg, make a multi-window app handle onRestarted() properly.
Stateless,
> singleton window apps like the calculator app are *almost* trivially made
> restart-resistant, but in the general case it's not a trivial transformation.
> I'm open to ideas for making the general case easier, but I think perhaps the
> best solution to that problem hasn't been discovered yet.
> 
> For example, making onRestarted only get fired to apps that had windows open
> when the restart occurred would mean that UI-less apps (eg: servers) don't get
> onRestarted events. At the end of the day the lifecycle for apps is more
complex
> than "alive when windows are there, not alive when windows aren't" and I feel
we
> shouldn't try and hide that, otherwise surprises will happen.
> 
> (Also, some of the counter-intuitiveness for this code is due to poorly chosen
> variable names, which I have changed.)

The rename helped a lot, thanks! We should still make this process easier for
developers, but this is no the place for that. I'll follow up in the issue or
more likely create a new issue.

[jeremya, 2012-12-13 23:00:25]

lgtm

[commit-bot: I haz the power, 2012-12-13 23:22:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/11316346/6001

[commit-bot: I haz the power, 2012-12-14 04:13:16]

Change committed as 173079