[IME] Supports options page for each IME instead of single options page for each IME extension.

[IME] Supports options page for each IME instead of single options page for each IME extension.

BUG=334527
TEST=Verified on sandbox.

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

[Shu Chen, 2014-01-14 03:26:00]

Guys, can you please review this cl?

This is to support options page for each IME entry, so that we can build
multiple IMEs into a single extension.

Thanks,
Shu

[not at google - send to devlin, 2014-01-14 17:28:06]

Could you file a bug for this? It's a bit fishy to me that we have a separate
options page per input component, but happy discuss it over a bug.

https://codereview.chromium.org/136693012/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/input_ime/input_components_handler.cc (right):

https://codereview.chromium.org/136693012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/input_ime/input_components_handler.cc:199: // Note:
'options_page_url' is optional in manifest.
it's just "options_page"?

[kevers, 2014-01-14 19:33:08]

https://codereview.chromium.org/136693012/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/input_ime/input_components_handler.cc (right):

https://codereview.chromium.org/136693012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/input_ime/input_components_handler.cc:208: }
Missing else block?  options_page_url does not appear to be initialized if the
key is missing from the manifest.

[Shu Chen, 2014-01-15 05:24:54]

Thanks for your reviews. I've updated the code per comments.

kalman@, I've created the bug:
https://code.google.com/p/chromium/issues/detail?id=334527

https://codereview.chromium.org/136693012/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/input_ime/input_components_handler.cc (right):

https://codereview.chromium.org/136693012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/input_ime/input_components_handler.cc:199: // Note:
'options_page_url' is optional in manifest.
On 2014/01/14 17:28:06, kalman wrote:
> it's just "options_page"?

Done.

https://codereview.chromium.org/136693012/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/input_ime/input_components_handler.cc:208: }
On 2014/01/14 19:33:08, kevers wrote:
> Missing else block?  options_page_url does not appear to be initialized if the
> key is missing from the manifest.

Done.

[kevers, 2014-01-15 19:51:35]

lgtm

[not at google - send to devlin, 2014-01-16 01:04:25]

https://codereview.chromium.org/136693012/diff/90001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/136693012/diff/90001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:360:
base::string16 options_page_str16;
I'd feel more comfortable if we didn't trust what JS told us, and instead looked
up the input method from the extension via |input_method_id| and directly found
its options page URL that way.

https://codereview.chromium.org/136693012/diff/90001/chrome/common/extensions...
File chrome/common/extensions/api/input_ime/input_components_handler.cc (right):

https://codereview.chromium.org/136693012/diff/90001/chrome/common/extensions...
chrome/common/extensions/api/input_ime/input_components_handler.cc:202:
options_page_url = extension->GetResourceURL(options_page_str);
Maybe you should warn if this isn't valid, as a developer courtesy. There is
some API for adding install warnings (not errors).

[Shu Chen, 2014-01-16 05:52:14]

https://codereview.chromium.org/136693012/diff/90001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/136693012/diff/90001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:360:
base::string16 options_page_str16;
On 2014/01/16 01:04:26, kalman wrote:
> I'd feel more comfortable if we didn't trust what JS told us, and instead
looked
> up the input method from the extension via |input_method_id| and directly
found
> its options page URL that way.

Done.

https://codereview.chromium.org/136693012/diff/90001/chrome/common/extensions...
File chrome/common/extensions/api/input_ime/input_components_handler.cc (right):

https://codereview.chromium.org/136693012/diff/90001/chrome/common/extensions...
chrome/common/extensions/api/input_ime/input_components_handler.cc:202:
options_page_url = extension->GetResourceURL(options_page_str);
On 2014/01/16 01:04:26, kalman wrote:
> Maybe you should warn if this isn't valid, as a developer courtesy. There is
> some API for adding install warnings (not errors).

I don't think it's necessary to install warnings on extension because it is hard
to make an invalid URL to verify the warnings. I've tracked the code from
GetResourceURL() to ReadUTFChar () in url/url_canon_icu.cc. The only way to make
an invalid URL is to put invalid unicode characters in the url string. I tried
"options_page": "\ud801.html" and when loading the extension, the error occurred
about "Invalid manifest json format".

Please check my latest patch. Now I simply return an error if the url is invalid
(just like the code above about input_view_url).

[not at google - send to devlin, 2014-01-16 17:51:13]

extensions lgtm

https://codereview.chromium.org/136693012/diff/90001/chrome/common/extensions...
File chrome/common/extensions/api/input_ime/input_components_handler.cc (right):

https://codereview.chromium.org/136693012/diff/90001/chrome/common/extensions...
chrome/common/extensions/api/input_ime/input_components_handler.cc:202:
options_page_url = extension->GetResourceURL(options_page_str);
On 2014/01/16 05:52:15, Shu Chen wrote:
> On 2014/01/16 01:04:26, kalman wrote:
> > Maybe you should warn if this isn't valid, as a developer courtesy. There is
> > some API for adding install warnings (not errors).
> 
> I don't think it's necessary to install warnings on extension because it is
hard
> to make an invalid URL to verify the warnings. I've tracked the code from
> GetResourceURL() to ReadUTFChar () in url/url_canon_icu.cc. The only way to
make
> an invalid URL is to put invalid unicode characters in the url string. I tried
> "options_page": "\ud801.html" and when loading the extension, the error
occurred
> about "Invalid manifest json format".
> 
> Please check my latest patch. Now I simply return an error if the url is
invalid
> (just like the code above about input_view_url).

Ok thanks, sorry about that.

https://codereview.chromium.org/136693012/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/136693012/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:377:
browser->tab_strip_model()->GetActiveWebContents();
indent += 2

[Shu Chen, 2014-01-17 02:52:36]

https://codereview.chromium.org/136693012/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc
(right):

https://codereview.chromium.org/136693012/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc:377:
browser->tab_strip_model()->GetActiveWebContents();
On 2014/01/16 17:51:14, kalman wrote:
> indent += 2

Done.

[Shu Chen, 2014-01-17 02:58:39]

Jun/Hiro, can you please take a look at the cl? Thanks

[Hiro Komatsu, 2014-01-17 04:48:21]

lgtm

[Jun Mukai, 2014-01-17 06:51:23]

lgtm

[commit-bot: I haz the power, 2014-01-17 09:24:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuchen@chromium.org/136693012/240001

[commit-bot: I haz the power, 2014-01-17 10:33:33]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-17 14:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuchen@chromium.org/136693012/490001

[commit-bot: I haz the power, 2014-01-17 15:59:38]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-18 00:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuchen@chromium.org/136693012/490001

[commit-bot: I haz the power, 2014-01-18 08:57:52]

Change committed as 245761