[Apps Developer Tool] Show installation warnings as chrome://extensions.

[Apps Developer Tool] Show extension install warnings. Uses the same appearance as the chrome://extensions page.

BUG=267187
TEST=1. Load an unpacked app/extension with some installation warnings;
            2. Open the Apps Developer Tool;
            3. Installation warnings are shown as chrome://extensions.

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

[zhchbin, 2013-08-05 14:29:55]

PTAL.

@nkostylev for chrome/browser/resources/*
@kalman for chrome/browser/extensions/* and chrome/common/extensions/*.

Also include @dvh for the issue owner.

Image:
https://docs.google.com/file/d/0B_7w63Sy3AuyQ0UtSzlYZDg3MEU/edit?usp=sharing

[not at google - send to devlin, 2013-08-05 14:35:33]

TEST line is complicated, just say "see bug". Also 

The CL description is confusing, maybe say "show extension install warnings.
Uses the same appearance as the chrome://extensions page".

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/developer_private.idl:62: InstallWarning[]?
install_warnings;
code would be simpler if this was non-optional and the JS test for warning
presence was install_warnings.length > 0.

[zhchbin, 2013-08-05 14:54:07]

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/developer_private.idl:62: InstallWarning[]?
install_warnings;
On 2013/08/05 14:35:33, kalman wrote:
> code would be simpler if this was non-optional and the JS test for warning
> presence was install_warnings.length > 0.

But it seems that chrome://extensions page only shows installation warnings for
unpacked apps/extensions. So I choose optional.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[not at google - send to devlin, 2013-08-05 18:21:23]

https://codereview.chromium.org/22191003/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/22191003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:316: }
instead:

info->path.reset(...)
for (std::vector<extensions::InstallWarning>::const_iterator it =
         item.install_warnings().begin();
     it != item.install_warnings().end(); ++it) {
  developer::InstallWarning* warning = new developer::InstallWarning();
  warning->is_html = ...;
  warning->message = it->message;
  info->install_warnings->push_back(make_linked_ptr(warning));
}

less code and identical effect.

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/developer_private.idl:62: InstallWarning[]?
install_warnings;
On 2013/08/05 14:54:07, zhchbin wrote:
> On 2013/08/05 14:35:33, kalman wrote:
> > code would be simpler if this was non-optional and the JS test for warning
> > presence was install_warnings.length > 0.
> 
> But it seems that chrome://extensions page only shows installation warnings
for
> unpacked apps/extensions. So I choose optional.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

The effect would be identical, unpacked doesn't come into it. I will comment in
the other place.

[Nikita (slow), 2013-08-05 22:26:39]

https://codereview.chromium.org/22191003/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/22191003/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items_list.js:290: li[warning.isHTML ?
'innerHTML' : 'innerText'] = warning.message;
nit: Please use textContent instead of innerText.

Do you really want to support HTML warnings? Where do they come from? Should we
trust that source?

[zhchbin, 2013-08-06 02:05:43]

https://codereview.chromium.org/22191003/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/22191003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:316: }
On 2013/08/05 18:21:23, kalman wrote:
> instead:
> 
> info->path.reset(...)
> for (std::vector<extensions::InstallWarning>::const_iterator it =
>          item.install_warnings().begin();
>      it != item.install_warnings().end(); ++it) {
>   developer::InstallWarning* warning = new developer::InstallWarning();
>   warning->is_html = ...;
>   warning->message = it->message;
>   info->install_warnings->push_back(make_linked_ptr(warning));
> }
> 
> less code and identical effect.

Done.

https://codereview.chromium.org/22191003/diff/1/chrome/browser/resources/apps...
File chrome/browser/resources/apps_debugger/js/items_list.js (right):

https://codereview.chromium.org/22191003/diff/1/chrome/browser/resources/apps...
chrome/browser/resources/apps_debugger/js/items_list.js:290: li[warning.isHTML ?
'innerHTML' : 'innerText'] = warning.message;
On 2013/08/05 22:26:40, Nikita Kostylev wrote:
> nit: Please use textContent instead of innerText.
> 
> Do you really want to support HTML warnings? Where do they come from? Should
we
> trust that source?

Done.

The C++ side struct: InstallWarning[0] support FORMAT_HTML. Even though I didn't
find any warning that uses FORMAT_HTML, it should be supported.  

[0]:https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/install_warning.h

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/developer_private.idl (right):

https://codereview.chromium.org/22191003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/developer_private.idl:62: InstallWarning[]?
install_warnings;
On 2013/08/05 18:21:23, kalman wrote:
> On 2013/08/05 14:54:07, zhchbin wrote:
> > On 2013/08/05 14:35:33, kalman wrote:
> > > code would be simpler if this was non-optional and the JS test for warning
> > > presence was install_warnings.length > 0.
> > 
> > But it seems that chrome://extensions page only shows installation warnings
> for
> > unpacked apps/extensions. So I choose optional.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> The effect would be identical, unpacked doesn't come into it. I will comment
in
> the other place.

Done.

[Nikita (slow), 2013-08-06 13:00:38]

lgtm

[not at google - send to devlin, 2013-08-06 14:17:02]

lgtm

who would be a good owner for the developer private API in the future?

[commit-bot: I haz the power, 2013-08-06 14:49:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/22191003/11001

[zhchbin, 2013-08-06 15:02:56]

On 2013/08/06 14:17:02, kalman wrote:
> lgtm
> 
> who would be a good owner for the developer private API in the future?

Aha, I Hope one day I can be this "Mr. Right".

Thanks for the review!

[commit-bot: I haz the power, 2013-08-06 17:35:16]

Change committed as 215923

[dvh, 2013-08-16 20:25:38]

On 2013/08/06 14:17:02, kalman wrote:
> lgtm
> 
> who would be a good owner for the developer private API in the future?

grv@ (or I) is probably the best owner for it now.

[not at google - send to devlin, 2013-08-16 21:35:19]

Cool, send me a cl?
On 16 Aug 2013 13:25, <dvh@chromium.org> wrote:

> On 2013/08/06 14:17:02, kalman wrote:
>
>> lgtm
>>
>
>  who would be a good owner for the developer private API in the future?
>>
>
> grv@ (or I) is probably the best owner for it now.
>
>
>
https://chromiumcodereview.**appspot.com/22191003/<https://chromiumcodereview...
>