Whitelist IME extenions for app.window with type:panel

Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc: https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoOfo/edit#heading=h.nikhlivlb11n

BUG=570761
TEST=Locally verified app.window API accessable for IME extensions.

Committed: https://crrev.com/49f72db8dbff60fb22dfddf09dddeffe54f674db
Cr-Commit-Position: refs/heads/master@{#366279}

[Azure Wei, 2015-12-16 03:32:32]

Description was changed from

==========
Whitelist IME extenions for app.window with type:panel

BUG=
TEST=Locally verified app.window API accessable for IME extensions.
==========

to

==========
Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc:
https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoO...

BUG=
TEST=Locally verified app.window API accessable for IME extensions.
==========

[Azure Wei, 2015-12-18 06:00:26]

Description was changed from

==========
Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc:
https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoO...

BUG=
TEST=Locally verified app.window API accessable for IME extensions.
==========

to

==========
Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc:
https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoO...

BUG=570761
TEST=Locally verified app.window API accessable for IME extensions.
==========

[Azure Wei, 2015-12-18 06:00:26]

azurewei@chromium.org changed reviewers:
+ msw@chromium.org, rdevlin.cronin@chromium.org, shuchen@chromium.org

[Azure Wei, 2015-12-18 06:02:15]

Hi msw@, Devlin and Shu, can you please review this cl? Thanks!

[Devlin, 2015-12-18 17:58:41]

extensions lgtm

https://codereview.chromium.org/1516253002/diff/20001/extensions/common/const...
File extensions/common/constants.h (right):

https://codereview.chromium.org/1516253002/diff/20001/extensions/common/const...
extensions/common/constants.h:214: // Extension ids used by IME
nit: end in a period.

[msw, 2015-12-18 19:42:01]

c/b/ui lgtm

[Azure Wei, 2015-12-19 07:21:42]

https://codereview.chromium.org/1516253002/diff/20001/extensions/common/const...
File extensions/common/constants.h (right):

https://codereview.chromium.org/1516253002/diff/20001/extensions/common/const...
extensions/common/constants.h:214: // Extension ids used by IME
On 2015/12/18 17:58:41, Devlin wrote:
> nit: end in a period.

Done. Thanks!

[Azure Wei, 2015-12-19 07:21:56]

The CQ bit was checked by azurewei@chromium.org

[Azure Wei, 2015-12-19 07:21:57]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1516253002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-12-19 07:22:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516253002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516253002/40001

[commit-bot: I haz the power, 2015-12-19 08:29:59]

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

[commit-bot: I haz the power, 2015-12-19 08:30:00]

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

[Azure Wei, 2015-12-19 09:22:52]

The CQ bit was checked by azurewei@chromium.org

[Azure Wei, 2015-12-19 09:22:52]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1516253002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-12-19 09:23:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516253002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516253002/80001

[commit-bot: I haz the power, 2015-12-19 10:45:54]

Description was changed from

==========
Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc:
https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoO...

BUG=570761
TEST=Locally verified app.window API accessable for IME extensions.
==========

to

==========
Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc:
https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoO...

BUG=570761
TEST=Locally verified app.window API accessable for IME extensions.
==========

[commit-bot: I haz the power, 2015-12-19 10:45:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-19 10:46:51]

Description was changed from

==========
Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc:
https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoO...

BUG=570761
TEST=Locally verified app.window API accessable for IME extensions.
==========

to

==========
Whitelist IME extensions for app.window with type:panel.
The app.window will be used to create IME menu. Please see the design doc:
https://docs.google.com/document/d/1Ml1QUsaNbLXYFh3dW8Q3md7cY1oUhpYvyZWRqdjoO...

BUG=570761
TEST=Locally verified app.window API accessable for IME extensions.

Committed: https://crrev.com/49f72db8dbff60fb22dfddf09dddeffe54f674db
Cr-Commit-Position: refs/heads/master@{#366279}
==========

[commit-bot: I haz the power, 2015-12-19 10:46:52]

Patchset 5 (id:??) landed as
https://crrev.com/49f72db8dbff60fb22dfddf09dddeffe54f674db
Cr-Commit-Position: refs/heads/master@{#366279}

[Evan Stade, 2015-12-21 23:54:20]

estade@chromium.org changed reviewers:
+ estade@chromium.org, stevenjb@chromium.org

[Evan Stade, 2015-12-21 23:54:20]

from what I can tell, stevenjb has some unresolved comments on the design doc.
Seems like you should address those before proceeding further.

[stevenjb, 2015-12-22 00:50:06]

On 2015/12/21 23:54:20, Evan Stade wrote:
> from what I can tell, stevenjb has some unresolved comments on the design doc.
> Seems like you should address those before proceeding further.

I think that most of my unresolved comments stemmed from confusion on my part
between the proposed "IME Menu" and the existing "is_ime_window" type.

This change will allow testing of using panels for the IME Menu, and if it
proves to be a good solution that seems fine. We will need to continue to
support the code in tabs_api.cc that allows whitelisted extensions to use panels
on Chrome OS, we'll just need to move that logic out of panel_manager.cc when it
gets deprecated.

[Azure Wei, 2015-12-22 01:53:24]

On 2015/12/22 00:50:06, stevenjb wrote:
> On 2015/12/21 23:54:20, Evan Stade wrote:
> > from what I can tell, stevenjb has some unresolved comments on the design
doc.
> > Seems like you should address those before proceeding further.
> 
> I think that most of my unresolved comments stemmed from confusion on my part
> between the proposed "IME Menu" and the existing "is_ime_window" type.
> 
> This change will allow testing of using panels for the IME Menu, and if it
> proves to be a good solution that seems fine. We will need to continue to
> support the code in tabs_api.cc that allows whitelisted extensions to use
panels
> on Chrome OS, we'll just need to move that logic out of panel_manager.cc when
it
> gets deprecated.

Do you mean the app.window will get deprecated someday? And if the AppWindow
gets deprecated, I should add codes in tabs_api.cc to support IME using panels
on ChromeOS. 
As I am not using windows.create API, I don't need to add other logic for now,
right?

[stevenjb, 2015-12-22 18:08:15]

On 2015/12/22 01:53:24, Azure Wei wrote:
> On 2015/12/22 00:50:06, stevenjb wrote:
> > On 2015/12/21 23:54:20, Evan Stade wrote:
> > > from what I can tell, stevenjb has some unresolved comments on the design
> doc.
> > > Seems like you should address those before proceeding further.
> > 
> > I think that most of my unresolved comments stemmed from confusion on my
part
> > between the proposed "IME Menu" and the existing "is_ime_window" type.
> > 
> > This change will allow testing of using panels for the IME Menu, and if it
> > proves to be a good solution that seems fine. We will need to continue to
> > support the code in tabs_api.cc that allows whitelisted extensions to use
> panels
> > on Chrome OS, we'll just need to move that logic out of panel_manager.cc
when
> it
> > gets deprecated.
> 
> Do you mean the app.window will get deprecated someday? And if the AppWindow
> gets deprecated, I should add codes in tabs_api.cc to support IME using panels
> on ChromeOS. 
> As I am not using windows.create API, I don't need to add other logic for now,
> right?

No, I did not suggest that app.window will get deprecated. I was under the
impression that the IME extension was planning to use chrome.windows to create
panel windows, in which case we need to ensure that the logic in
PanelManager::ShouldUsePanels() gits moved to tabs_api.cc where it is used when
we deprecate PanelManager.

However, looking more closely, that is irrelevant to this CL, which should have
been reviewed more carefully. The change to PanelManager::ShouldUsePanels() only
affects WindowsCreateFunction() in tabs_api.cc which is for chrome.windows. It
is unnecessary for chrome.app.window. i.e. the change to app_window_api.cc here
is the relevant change based on the CL description.

[Azure Wei, 2015-12-23 01:13:48]

On 2015/12/22 18:08:15, stevenjb wrote:
> On 2015/12/22 01:53:24, Azure Wei wrote:
> > On 2015/12/22 00:50:06, stevenjb wrote:
> > > On 2015/12/21 23:54:20, Evan Stade wrote:
> > > > from what I can tell, stevenjb has some unresolved comments on the
design
> > doc.
> > > > Seems like you should address those before proceeding further.
> > > 
> > > I think that most of my unresolved comments stemmed from confusion on my
> part
> > > between the proposed "IME Menu" and the existing "is_ime_window" type.
> > > 
> > > This change will allow testing of using panels for the IME Menu, and if it
> > > proves to be a good solution that seems fine. We will need to continue to
> > > support the code in tabs_api.cc that allows whitelisted extensions to use
> > panels
> > > on Chrome OS, we'll just need to move that logic out of panel_manager.cc
> when
> > it
> > > gets deprecated.
> > 
> > Do you mean the app.window will get deprecated someday? And if the AppWindow
> > gets deprecated, I should add codes in tabs_api.cc to support IME using
panels
> > on ChromeOS. 
> > As I am not using windows.create API, I don't need to add other logic for
now,
> > right?
> 
> No, I did not suggest that app.window will get deprecated. I was under the
> impression that the IME extension was planning to use chrome.windows to create
> panel windows, in which case we need to ensure that the logic in
> PanelManager::ShouldUsePanels() gits moved to tabs_api.cc where it is used
when
> we deprecate PanelManager.
> 
> However, looking more closely, that is irrelevant to this CL, which should
have
> been reviewed more carefully. The change to PanelManager::ShouldUsePanels()
only
> affects WindowsCreateFunction() in tabs_api.cc which is for chrome.windows. It
> is unnecessary for chrome.app.window. i.e. the change to app_window_api.cc
here
> is the relevant change based on the CL description.

Yes, the changes in panel_manager.cc is unnecessary, as I just need to create
panel with app.window. Sorry about my carelessness. I will open another cl to
revert the change.