Enable Pointer Lock for packaged apps using ShellWindow.

Enable Pointer Lock for packaged apps using ShellWindow.

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

[scheib, 2012-11-28 04:03:43]

jeremya, could you take a look and see if you can suggest a way to have
ExtensionPointerLockAccessPass in
chrome/browser/extensions/extension_pointer_lock_apitest.cc to have the
RenderWidgetHostView be focused, and wait until that before attempting pointer
lock?

[scheib, 2012-11-28 16:29:09]

miket, review or suggest a reviewer, please.

[asargent_no_longer_on_chrome, 2012-11-28 20:32:12]

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests.gypi#o...
chrome/chrome_tests.gypi:176:
'browser/extensions/window_open_interactive_apitest.cc',
Did you actually mean to remove this file from the list? Or should you have
added your entry just above it?

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:111: 'browser/extensions/extension_browsertest.h',
These classes have a bunch of pretty browser-specific functionality that doesn't
make sense in, say, unit_tests, so it seems unfortunate to have to move them
here.

I assume you're doing this so that you can run your test in the
interactive_ui_tests binary instead of the browser_tests one? I'm not as
familiar with interactive_ui_tests, but is there a reason we can't just have its
target just depend on browser_tests instead of doing this?

https://codereview.chromium.org/11413131/diff/2025/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://codereview.chromium.org/11413131/diff/2025/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:330:
IDS_EXTENSION_PROMPT_WARNING_POINTER_LOCK,
We should ask Erik's opinion about whether this permission rises to the level of
something we should explicitly mention in the install dialog.

For context, in the past we've taken the position that we want fine-granularity
for the permissions developers need to specify in their manifest, but much
rougher granularity in the install confirmation messages we actually show to
human users. In many cases we've decided to elide warnings about
"trivial/nuisance" permissions that don't have particularly dire security
implications (keeping in mind that installing an app is a more explicit user
action than loading an iframe with a potentially compromised ad network on some
random website), and I wonder whether pointer lock should be considered that
way.

[scheib, 2012-11-28 20:58:12]

Erik, please take a look at if we should include permission text for pointer
lock:
https://codereview.chromium.org/11413131/diff/2025/chrome/common/extensions/p...

Antony, thanks, see responses:

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests.gypi#o...
chrome/chrome_tests.gypi:176:
'browser/extensions/window_open_interactive_apitest.cc',
On 2012/11/28 20:32:13, Antony Sargent wrote:
> Did you actually mean to remove this file from the list? Or should you have
> added your entry just above it?

Done.

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/11413131/diff/2025/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:111: 'browser/extensions/extension_browsertest.h',
On 2012/11/28 20:32:13, Antony Sargent wrote:
> These classes have a bunch of pretty browser-specific functionality that
doesn't
> make sense in, say, unit_tests, so it seems unfortunate to have to move them
> here.
> 
> I assume you're doing this so that you can run your test in the
> interactive_ui_tests binary instead of the browser_tests one? I'm not as
> familiar with interactive_ui_tests, but is there a reason we can't just have
its
> target just depend on browser_tests instead of doing this?

Yes, moved so the test can run in interactive_ui_tests, which should really just
be named 'browser_tests_that_run_one_at_a_time' vs browser_tests which run in
parallel and fight over focus.

I didn't duplicate the files between the tests because I believe the current
practice to be to include a source file in only one target. Also, duplicating
increases total build time if you are building all the tests, e.g. on continuous
builders.

Thoughts? Do you propose duplicating?

https://codereview.chromium.org/11413131/diff/2025/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://codereview.chromium.org/11413131/diff/2025/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:330:
IDS_EXTENSION_PROMPT_WARNING_POINTER_LOCK,
On 2012/11/28 20:32:13, Antony Sargent wrote:
> We should ask Erik's opinion about whether this permission rises to the level
of
> something we should explicitly mention in the install dialog.
> 
> For context, in the past we've taken the position that we want
fine-granularity
> for the permissions developers need to specify in their manifest, but much
> rougher granularity in the install confirmation messages we actually show to
> human users. In many cases we've decided to elide warnings about
> "trivial/nuisance" permissions that don't have particularly dire security
> implications (keeping in mind that installing an app is a more explicit user
> action than loading an iframe with a potentially compromised ad network on
some
> random website), and I wonder whether pointer lock should be considered that
> way.

Looping Erik in.

[Erik does not do reviews, 2012-11-30 01:11:05]

https://codereview.chromium.org/11413131/diff/2025/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://codereview.chromium.org/11413131/diff/2025/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.cc:330:
IDS_EXTENSION_PROMPT_WARNING_POINTER_LOCK,
On 2012/11/28 20:58:12, scheib wrote:
> On 2012/11/28 20:32:13, Antony Sargent wrote:
> > We should ask Erik's opinion about whether this permission rises to the
level
> of
> > something we should explicitly mention in the install dialog.
> > 
> > For context, in the past we've taken the position that we want
> fine-granularity
> > for the permissions developers need to specify in their manifest, but much
> > rougher granularity in the install confirmation messages we actually show to
> > human users. In many cases we've decided to elide warnings about
> > "trivial/nuisance" permissions that don't have particularly dire security
> > implications (keeping in mind that installing an app is a more explicit user
> > action than loading an iframe with a potentially compromised ad network on
> some
> > random website), and I wonder whether pointer lock should be considered that
> > way.
> 
> Looping Erik in.

Nope, we don't need a permission string for pointer lock, full screen or
gamepad.

[scheib, 2012-11-30 02:35:47]

Antony, take another look: no common test files moved and no user permission
string.

[asargent_no_longer_on_chrome, 2012-11-30 18:48:05]

LGTM

https://codereview.chromium.org/11413131/diff/12017/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/11413131/diff/12017/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/shell_window.cc:285: bool user_gesture,
style nit: indentation looks wrong here

[scheib, 2012-11-30 21:00:14]

mkearney, FYI doc in 
chrome/common/extensions/docs/templates/articles/declare_permissions.html

miket, OWNERS for chrome/browser/ui/extensions/shell_window.*
(asargent, consider if you should be an owner there too?)

[miket_OOO, 2012-11-30 21:20:19]

chrome/browser/ui/extensions/shell_window.* LGTM

[commit-bot: I haz the power, 2012-11-30 21:38:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/11413131/6021

[commit-bot: I haz the power, 2012-11-30 21:51:16]

Retried try job too often on mac_rel for step(s) sync_integration_tests,
unit_tests

[commit-bot: I haz the power, 2012-11-30 21:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/11413131/6021

[commit-bot: I haz the power, 2012-11-30 23:11:09]

Retried try job too often on linux_chromeos for step(s) unit_tests

[commit-bot: I haz the power, 2012-11-30 23:38:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scheib@chromium.org/11413131/1070

[commit-bot: I haz the power, 2012-12-01 01:59:07]

Change committed as 170625