Add list of Shared Modules to App Info Dialog

Add list of Shared Modules to App Info Dialog

Add a list of Shared Modules to the App Info Dialog, along with an
'About' link next to each one that opens the shared module's options
page. This allows shared modules to display licenses, which meets a
legal requirement explained more in the bug.

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

[sashab, 2014-06-03 07:08:01]

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:266: class
AppInfoImportedModulesPanel : public views::View,
I chose to make this a View because it has a variable number of child views
created as a part of it's construction, so they can't be stored as member
variables (i.e. you can't separate the construction from the layout). I could
make this class smaller, moving stuff like the title out into the main layout,
but I'm happy either way.

[benwells, 2014-06-04 00:04:17]

Mostly nits, except for the question: do you think we should list a shared
module if it has no about?

https://codereview.chromium.org/314613002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/314613002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2193: <message
name="IDS_APPLICATION_INFO_IMPORTED_MODULES_ABOUT_LINK_TEXT" desc="Text for the
link next to each shared module that goes to the About page for that module. The
About page may include things such as open-source and copyright licenses for the
app.">
Nit: s/shared module/imported module/ or otherwise be consistent with the notes
above.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:266: class
AppInfoImportedModulesPanel : public views::View,
On 2014/06/03 07:08:01, sasha_b wrote:
> I chose to make this a View because it has a variable number of child views
> created as a part of it's construction, so they can't be stored as member
> variables (i.e. you can't separate the construction from the layout). I could
> make this class smaller, moving stuff like the title out into the main layout,
> but I'm happy either way.

As it is a recognizable chunk of the dialog, which is either all there or all
not, I think it makes sense as a separate view. So, sgtm.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:277:
views::Label* title_;
I don't think this needs to be a field.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:302: static
const int kTitleColumnId = 1;
Nit: these are column sets, not columns, so a name like kTitleColumnSetId would
be more appopriate.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:311: static
const int kModulesListColumnId = 2;
Same comment here, kModulesColumnSetId would be more appropriate.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:331: //
TODO(sashab): Since we only have 1 shared module at the moment, the
Nit: its a good idea not to say things like 'at the moment' in comments, as it
will be checked in and time goes on :)

I'd rephrase this to be something like: TODO(sashab): Revisit UI layout once
shared module usage becomes more common.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:345:
extensions::SharedModuleInfo::IsSharedModule(imported_module) &&
I didn't think IsSharedModule or IsExportAllowedByWhitelist is needed here? This
is checked during extension install already, which is the right place to be
checking for it.

[sashab, 2014-06-05 00:27:35]

Re: Should we display shared mods that have no about page
Answer: Yes.
Explanation: From the UI "This app imports the following modules...", it doesn't
suggest that apps without an about page would be excluded. Also, shared modules
that get updated to have an about page shouldn't randomly now appear in the list
- but the about page *should* randomly appear.

https://codereview.chromium.org/314613002/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/314613002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:2193: <message
name="IDS_APPLICATION_INFO_IMPORTED_MODULES_ABOUT_LINK_TEXT" desc="Text for the
link next to each shared module that goes to the About page for that module. The
About page may include things such as open-source and copyright licenses for the
app.">
On 2014/06/04 00:04:17, benwells wrote:
> Nit: s/shared module/imported module/ or otherwise be consistent with the
notes
> above.

Done.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
(right):

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:277:
views::Label* title_;
On 2014/06/04 00:04:17, benwells wrote:
> I don't think this needs to be a field.

Done.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:302: static
const int kTitleColumnId = 1;
On 2014/06/04 00:04:17, benwells wrote:
> Nit: these are column sets, not columns, so a name like kTitleColumnSetId
would
> be more appopriate.

Done.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:311: static
const int kModulesListColumnId = 2;
On 2014/06/04 00:04:17, benwells wrote:
> Same comment here, kModulesColumnSetId would be more appropriate.

Done.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:331: //
TODO(sashab): Since we only have 1 shared module at the moment, the
On 2014/06/04 00:04:17, benwells wrote:
> Nit: its a good idea not to say things like 'at the moment' in comments, as it
> will be checked in and time goes on :)
> 
> I'd rephrase this to be something like: TODO(sashab): Revisit UI layout once
> shared module usage becomes more common.

Done.

https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:345:
extensions::SharedModuleInfo::IsSharedModule(imported_module) &&
On 2014/06/04 00:04:17, benwells wrote:
> I didn't think IsSharedModule or IsExportAllowedByWhitelist is needed here?
This
> is checked during extension install already, which is the right place to be
> checking for it.

Ahh yes, I remember this discussion. *Technically* an shared module could still
change state (i.e. become disabled) while this dialog is open, in which case
GetExtensionById() would return NULL.

Update: Shared modules cannot be disabled. Changed to a DCHECK.

[benwells, 2014-06-05 00:41:45]

lgtm

On 2014/06/05 00:27:35, sasha_b wrote:
> Re: Should we display shared mods that have no about page
> Answer: Yes.
> Explanation: From the UI "This app imports the following modules...", it
doesn't
> suggest that apps without an about page would be excluded. Also, shared
modules
> that get updated to have an about page shouldn't randomly now appear in the
list
> - but the about page *should* randomly appear.

Yep I think with this UI makes sense. I was asking more to hear what you said,
as it is quite different to what we had originally planned. Now, instead of
having UI to show licenses, or even abouts, it is UI to show the shared modules.

This might be better served by having a standalone tab for shared modules, but
let's leave that whole discussion for later.

> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/app/generated_res...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/app/generated_res...
> chrome/app/generated_resources.grd:2193: <message
> name="IDS_APPLICATION_INFO_IMPORTED_MODULES_ABOUT_LINK_TEXT" desc="Text for
the
> link next to each shared module that goes to the About page for that module.
The
> About page may include things such as open-source and copyright licenses for
the
> app.">
> On 2014/06/04 00:04:17, benwells wrote:
> > Nit: s/shared module/imported module/ or otherwise be consistent with the
> notes
> > above.
> 
> Done.
> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
> (right):
> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:277:
> views::Label* title_;
> On 2014/06/04 00:04:17, benwells wrote:
> > I don't think this needs to be a field.
> 
> Done.
> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:302:
static
> const int kTitleColumnId = 1;
> On 2014/06/04 00:04:17, benwells wrote:
> > Nit: these are column sets, not columns, so a name like kTitleColumnSetId
> would
> > be more appopriate.
> 
> Done.
> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:311:
static
> const int kModulesListColumnId = 2;
> On 2014/06/04 00:04:17, benwells wrote:
> > Same comment here, kModulesColumnSetId would be more appropriate.
> 
> Done.
> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:331: //
> TODO(sashab): Since we only have 1 shared module at the moment, the
> On 2014/06/04 00:04:17, benwells wrote:
> > Nit: its a good idea not to say things like 'at the moment' in comments, as
it
> > will be checked in and time goes on :)
> > 
> > I'd rephrase this to be something like: TODO(sashab): Revisit UI layout once
> > shared module usage becomes more common.
> 
> Done.
> 
>
https://codereview.chromium.org/314613002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:345:
> extensions::SharedModuleInfo::IsSharedModule(imported_module) &&
> On 2014/06/04 00:04:17, benwells wrote:
> > I didn't think IsSharedModule or IsExportAllowedByWhitelist is needed here?
> This
> > is checked during extension install already, which is the right place to be
> > checking for it.
> 
> Ahh yes, I remember this discussion. *Technically* an shared module could
still
> change state (i.e. become disabled) while this dialog is open, in which case
> GetExtensionById() would return NULL.
> 
> Update: Shared modules cannot be disabled. Changed to a DCHECK.

[sashab, 2014-06-05 00:43:07]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-05 00:44:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/314613002/40001

[commit-bot: I haz the power, 2014-06-05 05:23:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-05 05:28:20]

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

[commit-bot: I haz the power, 2014-06-05 05:28:20]

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

[sashab, 2014-06-09 03:22:33]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-09 03:22:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/314613002/40001

[commit-bot: I haz the power, 2014-06-09 05:00:32]

Change committed as 275767