Allow component IME extension use app.window and add 'ime' window type for app window

Allow component IME extensions use app.window and add 'ime' boolean in CreateWindowOptions for app window

To create an IME window, an IME must be whitelisted, must have app.window.ime permission and must use set 'ime' and 'frame' this way:
chrome.app.window.create(url, {
  'ime': true,
  'frame': 'none
}, function() {})
Note only whitelisted component IME extensions can use this boolean parameter. See doc here: https://docs.google.com/a/google.com/document/d/1JQHpLu_RjN8C1Yi0i_eApS5SZtPoMDqvlq06YOwXCFU/edit

BUG=401984
Committed: https://crrev.com/c3875427bb19532a238608d7c04d34bc0fa22421
Cr-Commit-Position: refs/heads/master@{#297163}

[bshe, 2014-08-26 18:03:39]

bshe@chromium.org changed reviewers:
+ benwells@chromium.org

[bshe, 2014-08-26 18:03:39]

Hi Ben.

Do you mind to take a first look at this CL?

This CL allows component IME extensions create app window. The main change I did
is to reparent the new app window to kShellWindowId_ImeWindowParentContainer.
Once I did that, text focus wont change after user click on the new app window.
And the new app window is stacked on top of virtual keyboard window(VK window
belongs to the same container). The two new behaviors are desired IME window
behaviors. 

I also added an 'ime' window type to app.window api's window type. It should
only be available for component extension.

On the client side, IME author will create a window this way:
chrome.app.window.create(
  {
    'type': 'ime'
  }, function() {}
);

Any suggestion are welcome. Thanks!

Biao

[benwells, 2014-08-26 22:35:40]

On 2014/08/26 18:03:39, bshe wrote:
> Hi Ben.
> 
> Do you mind to take a first look at this CL?
> 
> This CL allows component IME extensions create app window. The main change I
did
> is to reparent the new app window to kShellWindowId_ImeWindowParentContainer.
> Once I did that, text focus wont change after user click on the new app
window.
> And the new app window is stacked on top of virtual keyboard window(VK window
> belongs to the same container). The two new behaviors are desired IME window
> behaviors. 
> 
> I also added an 'ime' window type to app.window api's window type. It should
> only be available for component extension.
> 
> On the client side, IME author will create a window this way:
> chrome.app.window.create(
>   {
>     'type': 'ime'
>   }, function() {}
> );
> 
> Any suggestion are welcome. Thanks!
> 
> Biao

From a quick glance it looks great. I think I'd prefer this to be boolean create
window option as opposed to a window type thing. E.g. ime: true. But other
people might disagree.

Can you put this approach through the API process?

[not at google - send to devlin, 2014-08-27 15:14:33]

kalman@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2014-08-27 15:14:33]

Remember to also blacklist these extensions for the chrome.app API.

[bshe, 2014-08-28 13:41:43]

bshe@chromium.org changed reviewers:
+ oshima@chromium.org

[bshe, 2014-08-28 13:41:43]

I have updated this CL according to the suggestions. Now we have 'ime' boolean
insteand of ime window type. And I also added a hardcode check and restricted
the new parameter to whitelisted component extensions only.

Do you mind to take another look? Thanks!

+oshima for ash related files (I renamed a window id)

[not at google - send to devlin, 2014-08-28 15:38:33]

kalman@chromium.org changed reviewers:
- kalman@chromium.org

[not at google - send to devlin, 2014-08-28 15:38:33]

Thanks for adding those to the blacklist.

Now I depart :)

[benwells, 2014-08-28 22:22:23]

I'll wait to get the proposal lg'd before reviewing. I'm happy but want to see
what security thinks.

[oshima, 2014-08-29 00:01:12]

ash/ lgtm (assuming that's what you want me to review)

[bshe, 2014-09-04 18:20:36]

Patchset #4 (id:60001) has been deleted

[benwells, 2014-09-09 00:02:30]

This needs tests. In particular:
- a functional test that the API does what you want
- a test that a non-whitelisted extension can't use app.window
- a test that a whitelisted extension can't create windows without ime: true.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:54: "Only whitelisted
component IME extensions can create ime window.";
Nit: s/window/windows/

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:209:
"06BE211D5F014BAB34BC22D9DDA09C63A81D828E",
I think the best way to do this checking is to create a new permission, similar
to the way alpha is done below.

That way you can use the _permission_features.json file to restrict the
permission to the one component extension, and also restrict it to dev channel.
(all development APIs should be resticted to dev channel until they are complete
and have gone through a final API review).

You don't need to do any whitelisting checking here (alphaEnabled does it but it
isn't really necessary).

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:219:
create_params.is_ime_window = true;
Should be setting to *options->ime.get() for cases where ime: false is
specified.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:221: }
There needs to be a check to make sure that if the client of this is not a
packaged app, create_params.is_ime_window is true. That needs to be enforced
somehow. Currently your component extension can create non-ime windows.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:230: #if
defined(USE_ASH)
This will compile on most platforms. Is it a problem that this code will execute
on Windows even in desktop mode? Should you be restricting based on the current
desktop type as well?

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:232: // Puts ime
windows to ime window container.
Nit: s/to/into/ .... or something.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:266: #if
defined(USE_ASH)
I think this part should defitely restrict on the current desktop type, not just
whether ash is available.

[bshe, 2014-09-10 22:32:23]

Patchset #5 (id:100001) has been deleted

[bshe, 2014-09-10 22:34:55]

Hi Ben.

We talked offline and agreed that use whitelist for ime options is fine for now.
I have also added a simple test in the latest patch. Do you mind to take a look?
I will add more tests in a follow up CL to test various cases.

Thanks

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:54: "Only whitelisted
component IME extensions can create ime window.";
On 2014/09/09 00:02:30, benwells wrote:
> Nit: s/window/windows/

Done.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:209:
"06BE211D5F014BAB34BC22D9DDA09C63A81D828E",
On 2014/09/09 00:02:30, benwells wrote:
> I think the best way to do this checking is to create a new permission,
similar
> to the way alpha is done below.
> 
> That way you can use the _permission_features.json file to restrict the
> permission to the one component extension, and also restrict it to dev
channel.
> (all development APIs should be resticted to dev channel until they are
complete
> and have gone through a final API review).
> 
> You don't need to do any whitelisting checking here (alphaEnabled does it but
it
> isn't really necessary).

Done.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:219:
create_params.is_ime_window = true;
On 2014/09/09 00:02:30, benwells wrote:
> Should be setting to *options->ime.get() for cases where ime: false is
> specified.

Oops. I thought options->ime.get() returns the boolean value.
Done.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:221: }
It is now enforced by whitelist. Only two whitelisted extension can request the
permission and hence create ime window.

It should be fine to allow whitelisted IMEs to create non-ime window IMO. Do you
have any specific concern?

On 2014/09/09 00:02:30, benwells wrote:
> There needs to be a check to make sure that if the client of this is not a
> packaged app, create_params.is_ime_window is true. That needs to be enforced
> somehow. Currently your component extension can create non-ime windows.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:230: #if
defined(USE_ASH)
Restricted to chromeos only as IME is ChromeOS only. And this window should only
be used with IME.

On 2014/09/09 00:02:30, benwells wrote:
> This will compile on most platforms. Is it a problem that this code will
execute
> on Windows even in desktop mode? Should you be restricting based on the
current
> desktop type as well?

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:232: // Puts ime
windows to ime window container.
On 2014/09/09 00:02:30, benwells wrote:
> Nit: s/to/into/ .... or something.

Done.

https://codereview.chromium.org/503383002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:266: #if
defined(USE_ASH)
On 2014/09/09 00:02:30, benwells wrote:
> I think this part should defitely restrict on the current desktop type, not
just
> whether ash is available.

Done.

[benwells, 2014-09-11 19:35:09]

benwells@chromium.org changed reviewers:
+ jackhou@google.com

[benwells, 2014-09-11 19:35:10]

removing myself as I am about to go on three weeks leave.

Jack should do an overall review, after which you will need to find extension
owners.

I haven't looked closely but I've added some comments for things I think should
be fixed.

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:214: if
(options->ime.get()) {
It is important that non-app extensions MUST specify this, but that doesn't seem
to be enforced in any way.

There needs to be a check after this that if the extension isn't a packaged app
that create_params.is_ime_window is true. This also needs to be tested.

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:54: #if
defined(OS_CHROMEOS)
what about ash on windows etc.? It doesn't seem like it would be hard to
support.

[bshe, 2014-09-11 22:35:59]

Thanks for the feedback Ben.

I have addressed your review. jackhou@ do you mind to take a look?

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:214: if
(options->ime.get()) {
I see what you mean. I thought whitelist is enough.

In the latest patch:
1. if platform app set ime option, we will give an error;
2. if extension didn't set ime option, we will give an error;

And the two new cases are tested.


On 2014/09/11 19:35:10, benwells (OOO until October 7) wrote:
> It is important that non-app extensions MUST specify this, but that doesn't
seem
> to be enforced in any way.
> 
> There needs to be a check after this that if the extension isn't a packaged
app
> that create_params.is_ime_window is true. This also needs to be tested.

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views.cc (right):

https://codereview.chromium.org/503383002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views.cc:54: #if
defined(OS_CHROMEOS)
IME api and the component extensions are not available on Windows. And the ime
option only intend to be used by an IME extension. So I restricted it to
ChromeOS as IME api did.

On 2014/09/11 19:35:10, benwells (OOO until October 7) wrote:
> what about ash on windows etc.? It doesn't seem like it would be hard to
> support.

[jackhou1, 2014-09-12 03:24:50]

jackhou@chromium.org changed reviewers:
+ jackhou@chromium.org

[jackhou1, 2014-09-12 03:24:52]

Looks pretty good, thanks for the tests. I have a few comments but let me know
if I'm misunderstanding your requirements for this API.

Could you link the API proposal in the CL description?

Nit: In comments and error strings, we should try to use the correct
capitalization of IME. I.e. "IME" when referring to the concept, "Ime" in
function names, "ime" when referring to the field in CreateWindowOptions.

https://codereview.chromium.org/503383002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:219: if
(extension()->GetType() == extensions::Manifest::TYPE_PLATFORM_APP) {
Prior to this, this entire function assumed TYPE_PLATFORM_APP. It might be
clearer to check for TYPE_EXTENSION, and/or add a comment here about why the
type might be an extension.

https://codereview.chromium.org/503383002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:226: if
(!options->ime.get()) {
IIUC, extensions can only ever use chrome.app.window.create() if they are IME
extensions? If so, it might be better to order these checks like so:

1) Permission, since this identifies it as an IME extension. Since
kImeWindowMissingPermission is only shown to extensions, it could say something
like "chrome.app.window requires the app.window.ime permission".
2) Chrome OS.
3) ime: true, checking this after permission means we can give a better error
message in kImeOptionIsRequired, maybe something like "IME extensions must
create windows with \"ime: true\"."
4) frame: none.

https://codereview.chromium.org/503383002/diff/180001/chrome/common/extension...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/503383002/diff/180001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:379: [nodoc] static boolean
isImeWindow();
Is this necessary? For alphaEnabled, requesting alphaEnabled:true does not
guarantee an alpha enabled window, so the window is expected to check what it
got. For IME, I think we either guarantee an IME window, or fail to create the
window, so the extension wouldn't need to check this.

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_platform_app/background.js
(right):

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_platform_app/background.js:34:
function testImeEnabledPermTransInitFalse() {
Please use un-abbreviated words in function names.

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_whitelisted/background.js
(right):

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_whitelisted/background.js:35:
testImeEnabled('testImeEnabledPermTransInitFalse',
Do we actually allow this case (ime:false)? It would mean the extension can
create a normal window.

Since extensions get a settings page, it doesn't seem necessary to open more
windows. I think we should disallow this until a compelling use case comes up.

[benwells, 2014-09-12 04:49:50]

benwells@chromium.org changed reviewers:
+ miket@chromium.org

[benwells, 2014-09-12 04:49:51]

+miket for extensions ownership

[bshe, 2014-09-12 17:41:44]

Patchset #10 (id:220001) has been deleted

[bshe, 2014-09-12 17:41:55]

Patchset #9 (id:200001) has been deleted

[bshe, 2014-09-12 17:44:12]

I will add a link in the bug description to the design doc after I clean up the
doc. At the same time, do you mind to take another look?

https://codereview.chromium.org/503383002/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:219: if
(extension()->GetType() == extensions::Manifest::TYPE_PLATFORM_APP) {
On 2014/09/12 03:24:51, jackhou1 wrote:
> Prior to this, this entire function assumed TYPE_PLATFORM_APP. It might be
> clearer to check for TYPE_EXTENSION, and/or add a comment here about why the
> type might be an extension.

Done.

https://codereview.chromium.org/503383002/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:226: if
(!options->ime.get()) {
On 2014/09/12 03:24:51, jackhou1 wrote:
> IIUC, extensions can only ever use chrome.app.window.create() if they are IME
> extensions? If so, it might be better to order these checks like so:
> 
> 1) Permission, since this identifies it as an IME extension. Since
> kImeWindowMissingPermission is only shown to extensions, it could say
something
> like "chrome.app.window requires the app.window.ime permission".
> 2) Chrome OS.
> 3) ime: true, checking this after permission means we can give a better error
> message in kImeOptionIsRequired, maybe something like "IME extensions must
> create windows with \"ime: true\"."
> 4) frame: none.

Done.

https://codereview.chromium.org/503383002/diff/180001/chrome/common/extension...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/503383002/diff/180001/chrome/common/extension...
chrome/common/extensions/api/app_window.idl:379: [nodoc] static boolean
isImeWindow();
I thought that IME extensions can create non-ime window. But since you and ben
both dont like the idea, I have changed the code so that IME extensions can only
create IME window. In that case, this is not necessary. Removed.

On 2014/09/12 03:24:51, jackhou1 wrote:
> Is this necessary? For alphaEnabled, requesting alphaEnabled:true does not
> guarantee an alpha enabled window, so the window is expected to check what it
> got. For IME, I think we either guarantee an IME window, or fail to create the
> window, so the extension wouldn't need to check this.

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_platform_app/background.js
(right):

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_platform_app/background.js:34:
function testImeEnabledPermTransInitFalse() {
On 2014/09/12 03:24:52, jackhou1 wrote:
> Please use un-abbreviated words in function names.

Done.

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_whitelisted/background.js
(right):

https://codereview.chromium.org/503383002/diff/180001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/windows_api_ime/has_permissions_whitelisted/background.js:35:
testImeEnabled('testImeEnabledPermTransInitFalse',
The new patch disallow IME extensions to create normal window.

On 2014/09/12 03:24:52, jackhou1 wrote:
> Do we actually allow this case (ime:false)? It would mean the extension can
> create a normal window.
> 
> Since extensions get a settings page, it doesn't seem necessary to open more
> windows. I think we should disallow this until a compelling use case comes up.

[jackhou1, 2014-09-15 03:49:33]

LGTM with a few comments.

https://codereview.chromium.org/503383002/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:60: const char
kImeOptionMustTrueAndNeedsFrameNone[] =
Nit: s/MustTrue/MustBeTrue/

https://codereview.chromium.org/503383002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:64: "The \"ime\"
option requires \"app.window.ime\" permission.";
"Extensions require the \"app.window.ime\" permission to create windows."

(It's more useful to the developer, and we can change this if we ever allow
extensions to create normal windows).

https://codereview.chromium.org/503383002/diff/240001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/503383002/diff/240001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:153: bool is_ime_window;
I think you can remove this too if it's not used any more.

[miket_OOO, 2014-09-15 18:23:12]

c/b/e OWNERS LGTM

[bshe, 2014-09-16 12:44:54]

bshe@chromium.org changed reviewers:
+ shuchen@chromium.org, tapted@chromium.org

[bshe, 2014-09-16 12:44:56]

jackhou@ I have addressed your comments. And currently investigating the
no_parent issue. Thanks for review!

+missing owners

shuchen@ for
chrome/browser/resources/chromeos/input_method/*
tapted@ for chrome/browser/ui/views/apps/chrome_native_app_window_views.cc

https://codereview.chromium.org/503383002/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/503383002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:60: const char
kImeOptionMustTrueAndNeedsFrameNone[] =
On 2014/09/15 03:49:33, jackhou1 wrote:
> Nit: s/MustTrue/MustBeTrue/

Done.

https://codereview.chromium.org/503383002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/app_window/app_window_api.cc:64: "The \"ime\"
option requires \"app.window.ime\" permission.";
On 2014/09/15 03:49:33, jackhou1 wrote:
> "Extensions require the \"app.window.ime\" permission to create windows."
> 
> (It's more useful to the developer, and we can change this if we ever allow
> extensions to create normal windows).

Done.

https://codereview.chromium.org/503383002/diff/240001/extensions/browser/app_...
File extensions/browser/app_window/app_window.h (right):

https://codereview.chromium.org/503383002/diff/240001/extensions/browser/app_...
extensions/browser/app_window/app_window.h:153: bool is_ime_window;
On 2014/09/15 03:49:33, jackhou1 wrote:
> I think you can remove this too if it's not used any more.

This is still used in app_window_api.cc to remember the ime option from options.

[Shu Chen, 2014-09-16 13:00:45]

lgtm for changes under chrome/browser/resources/chromeos/input_method/...

[tapted, 2014-09-16 21:46:33]

c/b/ui/views/apps lgtm

[bshe, 2014-09-18 23:11:12]

Patchset #11 (id:280001) has been deleted

[bshe, 2014-09-19 02:21:29]

Patchset #12 (id:320001) has been deleted

[bshe, 2014-09-19 02:21:42]

Patchset #11 (id:300001) has been deleted

[bshe, 2014-09-19 02:30:14]

Thanks for reviewing.

jackhou@ do you mind to take another look at patchset 11? I made the
app.window.canSetVisibleOnAllWorkspaces feature a complex feature too (see
extensions/common/api/_api_features.json). It seems that we could avoid the
problem this way. I have filed a bug and added a comment link to the bug. I am
hoping to land this CL early as I have a few CLs that are blocked on this one
now.

[jackhou1, 2014-09-19 03:32:08]

On 2014/09/19 02:30:14, bshe wrote:
> Thanks for reviewing.
> 
> jackhou@ do you mind to take another look at patchset 11? I made the
> app.window.canSetVisibleOnAllWorkspaces feature a complex feature too (see
> extensions/common/api/_api_features.json). It seems that we could avoid the
> problem this way. I have filed a bug and added a comment link to the bug. I am
> hoping to land this CL early as I have a few CLs that are blocked on this one
> now.

lgtm

[Ken Rockot(use gerrit already), 2014-09-28 20:56:35]

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

[Ken Rockot(use gerrit already), 2014-09-28 20:56:36]

https://codereview.chromium.org/503383002/diff/340001/extensions/common/api/_...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/503383002/diff/340001/extensions/common/api/_...
extensions/common/api/_api_features.json:48: "noparent": true,
Due to the particularly gnarly bug which prevents both chrome.app and
chrome.app.window from being usable in the same context, you want to set
"component_extensions_auto_granted" to false here.

This will fix the failing tts API test.

[bshe, 2014-09-29 01:34:04]

https://codereview.chromium.org/503383002/diff/340001/extensions/common/api/_...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/503383002/diff/340001/extensions/common/api/_...
extensions/common/api/_api_features.json:48: "noparent": true,
Thanks for your suggestion, Really appreciate! I have set the property to false
and will try to land my CL now.


On 2014/09/28 20:56:35, Ken Rockot (slow this week) wrote:
> Due to the particularly gnarly bug which prevents both chrome.app and
> chrome.app.window from being usable in the same context, you want to set
> "component_extensions_auto_granted" to false here.
> 
> This will fix the failing tts API test.

[bshe, 2014-09-29 01:34:19]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-09-29 01:34:58]

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

[commit-bot: I haz the power, 2014-09-29 01:37:54]

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

[commit-bot: I haz the power, 2014-09-29 01:37:55]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/61694)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[bshe, 2014-09-29 01:55:06]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-09-29 01:55:31]

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

[commit-bot: I haz the power, 2014-09-29 03:22:38]

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

[commit-bot: I haz the power, 2014-09-29 03:22:40]

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

[Ken Rockot(use gerrit already), 2014-09-29 04:54:43]

https://codereview.chromium.org/503383002/diff/380001/extensions/common/api/_...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/503383002/diff/380001/extensions/common/api/_...
extensions/common/api/_api_features.json:49: "component_extension_auto_granted":
false,
Missing an 's' in "extensions"

[bshe, 2014-09-29 11:05:27]

https://codereview.chromium.org/503383002/diff/380001/extensions/common/api/_...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/503383002/diff/380001/extensions/common/api/_...
extensions/common/api/_api_features.json:49: "component_extension_auto_granted":
false,
Dooh. Thanks!

On 2014/09/29 04:54:43, Ken Rockot (slow this week) wrote:
> Missing an 's' in "extensions"

[bshe, 2014-09-29 11:05:37]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-09-29 11:06:13]

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

[commit-bot: I haz the power, 2014-09-29 13:21:38]

Committed patchset #14 (id:400001) as d53641b0551a260e6b92aa752f3d3634376a1acc

[commit-bot: I haz the power, 2014-09-29 13:22:29]

Patchset 14 (id:??) landed as
https://crrev.com/c3875427bb19532a238608d7c04d34bc0fa22421
Cr-Commit-Position: refs/heads/master@{#297163}