Add an API for hosted apps to check their install and running states.

Add an API for hosted apps to check their install and running states.

BUG=107216
TEST=ChromeAppAPITest.*

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

[jstritar, 2012-04-22 17:57:31]

http://codereview.chromium.org/10174001/diff/2001/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_app_api_browsertest.cc (right):

http://codereview.chromium.org/10174001/diff/2001/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_app_api_browsertest.cc:281:
EXPECT_EQ("cannot_run", RunningState(L"//html/iframe[1]"));
This part returns "running" .... what's supposed to happen within iframes? See
comment #3 in http://crbug.com/107216.

http://codereview.chromium.org/10174001/diff/2001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/10174001/diff/2001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:383: MatchPattern(test, path_escaped_ +
'/');
I did this because some apps, like Google Finance, define their extents as
"http://www.google.com/finance/" but they are actually located at
"http://www.google.com/finance". How do you think we should address that?

[miket_OOO, 2012-04-23 22:11:16]

LGTM but see comments, please.

http://codereview.chromium.org/10174001/diff/2001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/10174001/diff/2001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:383: MatchPattern(test, path_escaped_ +
'/');
On 2012/04/22 17:57:31, jstritar wrote:
> I did this because some apps, like Google Finance, define their extents as
> "http://www.google.com/finance/" but they are actually located at
> "http://www.google.com/finance". How do you think we should address that?

I'd prefer to force developers to get it exactly right than to support loose
matching forever. If someone believes their webserver isn't predictably going to
302 them over to the desired URL (finance to finance/ or vice-versa), they can
include two patterns (finance and finance/) in their extents list.

Also, I think this method is used in other contexts, too (content scripts), and
from its name it could be used in future contexts, so I think it's better to go
with the least surprising behavior (MatchesPath = exactly matches, not loose
matching).

Finally, I would smack anyone who did this, but you could see /foo and /foo/
doing totally different things on a webserver, so I'm not sure it's a safe
assumption that everyone meant them to be equivalent.

http://codereview.chromium.org/10174001/diff/5003/chrome/common/extensions/ap...
File chrome/common/extensions/api/app.json (right):

http://codereview.chromium.org/10174001/diff/5003/chrome/common/extensions/ap...
chrome/common/extensions/api/app.json:40: "description": "todo",
todo->TODO to be consistent.

[jstritar, 2012-04-23 23:46:12]

Can you take another look? I updated AppBindings::GetRunningState to work
correctly with iframes, and got rid of some flake in the test.

http://codereview.chromium.org/10174001/diff/2001/chrome/common/extensions/ur...
File chrome/common/extensions/url_pattern.cc (right):

http://codereview.chromium.org/10174001/diff/2001/chrome/common/extensions/ur...
chrome/common/extensions/url_pattern.cc:383: MatchPattern(test, path_escaped_ +
'/');
On 2012/04/23 22:11:16, miket wrote:
> On 2012/04/22 17:57:31, jstritar wrote:
> > I did this because some apps, like Google Finance, define their extents as
> > "http://www.google.com/finance/" but they are actually located at
> > "http://www.google.com/finance". How do you think we should address that?
> 
> I'd prefer to force developers to get it exactly right than to support loose
> matching forever. If someone believes their webserver isn't predictably going
to
> 302 them over to the desired URL (finance to finance/ or vice-versa), they can
> include two patterns (finance and finance/) in their extents list.
> 
> Also, I think this method is used in other contexts, too (content scripts),
and
> from its name it could be used in future contexts, so I think it's better to
go
> with the least surprising behavior (MatchesPath = exactly matches, not loose
> matching).
> 
> Finally, I would smack anyone who did this, but you could see /foo and /foo/
> doing totally different things on a webserver, so I'm not sure it's a safe
> assumption that everyone meant them to be equivalent.

Agreed... if we do any fuzzy matching it probably shouldn't be in the
URLPattern, but when we create the app's extent (if at all). 
Removed the change from the CL.

http://codereview.chromium.org/10174001/diff/5003/chrome/common/extensions/ap...
File chrome/common/extensions/api/app.json (right):

http://codereview.chromium.org/10174001/diff/5003/chrome/common/extensions/ap...
chrome/common/extensions/api/app.json:40: "description": "todo",
On 2012/04/23 22:11:16, miket wrote:
> todo->TODO to be consistent.

Done.

[miket_OOO, 2012-04-24 00:04:48]

http://codereview.chromium.org/10174001/diff/12001/chrome/renderer/extensions...
File chrome/renderer/extensions/app_bindings.cc (right):

http://codereview.chromium.org/10174001/diff/12001/chrome/renderer/extensions...
chrome/renderer/extensions/app_bindings.cc:231: return v8::String::New(state);
Just for clarity, can you move state's declaration below this code block and put
the constant string in the String::New() function call inside this code block?

The reason is that I always scrutinize early exits in functions, and it was
confusing to see state manipulated in this block with an immediate exit. It'd be
less confusing if state didn't even come into existence after this block.

[jstritar, 2012-04-24 00:11:59]

http://codereview.chromium.org/10174001/diff/12001/chrome/renderer/extensions...
File chrome/renderer/extensions/app_bindings.cc (right):

http://codereview.chromium.org/10174001/diff/12001/chrome/renderer/extensions...
chrome/renderer/extensions/app_bindings.cc:231: return v8::String::New(state);
On 2012/04/24 00:04:48, miket wrote:
> Just for clarity, can you move state's declaration below this code block and
put
> the constant string in the String::New() function call inside this code block?
> 
> The reason is that I always scrutinize early exits in functions, and it was
> confusing to see state manipulated in this block with an immediate exit. It'd
be
> less confusing if state didn't even come into existence after this block.

Done.

[miket_OOO, 2012-04-24 00:19:53]

Still LGTM.

[commit-bot: I haz the power, 2012-04-24 00:55:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/10174001/21003

[commit-bot: I haz the power, 2012-04-24 01:44:05]

Try job failure for 10174001-21003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-24 16:16:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/10174001/12005

[commit-bot: I haz the power, 2012-04-24 17:53:51]

Change committed as 133722