Add the about_page key to shared_module manifest

Add the about_page key to shared_module manifest

Add the about_page key to the manifest for shared modules. This will
later be used to display the license information for shared module apps.

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

[sashab, 2014-06-05 00:45:16]

Not really tested yet; will make a separate patch for adding this to the app
info dialog, and update this one if necessary. Or maybe the dialog changes
should be part of this patch?

[sashab, 2014-06-05 00:52:09]

Nevermind; the change to the dialog is really small, so I just added it to this
patch.

[benwells, 2014-06-05 03:59:17]

I think this patch has the other patch for the dialog in it as well.

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.cc (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.cc:272:
AboutPageHandler::AboutPageHandler() {
This class is so similar to the options page class. I wonder if we could pull
out the commonality into a superclass. What do you think that would look like?

It would mean if we ever found / fixed a bug (e.g. in how we detect if a URL is
absolute) we wouldn't need to do it twice.

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.cc:280: std::string options_str;
Nit: options_str -> about_str

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.h (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.h:133: virtual bool Validate(const
Extension* extension,
Do you know what calls this? It is weird that it uses a string resource to
return errors. Normally developer errors aren't translated.

[benwells, 2014-06-05 03:59:42]

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:306: "extension",
"legacy_packaged_app", "hosted_app", "shared_module"
You should be removing this in this patch.

[sashab, 2014-06-10 05:44:13]

Any ideas for factoring out parts of the OptionsPageHandler class? I think
they're fine as two separate classes; it's a lot of boilerplate but other than
'helper functions' (which would probably only abstract single method calls
anyway), I don't think there's much point.

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_manifest_features.json:306: "extension",
"legacy_packaged_app", "hosted_app", "shared_module"
On 2014/06/05 03:59:42, benwells wrote:
> You should be removing this in this patch.

Thanks. Done.

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.cc (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.cc:272:
AboutPageHandler::AboutPageHandler() {
On 2014/06/05 03:59:17, benwells wrote:
> This class is so similar to the options page class. I wonder if we could pull
> out the commonality into a superclass. What do you think that would look like?
> 
> It would mean if we ever found / fixed a bug (e.g. in how we detect if a URL
is
> absolute) we wouldn't need to do it twice.

Not sure if they're _that_ similar. The options_page key allows for an absolute
URL for extensions, but a relative URL for packaged apps. But yes, I agree that
they are very similar.

A lot of it is already method calls, so I'm not sure how much could really be
abstracted out. Also, the error messages are different constants.

If you have any ideas though, let me know. I can also add a TODO.

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.cc:280: std::string options_str;
On 2014/06/05 03:59:17, benwells wrote:
> Nit: options_str -> about_str

Done.

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.h (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.h:133: virtual bool Validate(const
Extension* extension,
On 2014/06/05 03:59:17, benwells wrote:
> Do you know what calls this? It is weird that it uses a string resource to
> return errors. Normally developer errors aren't translated.

Not sure what calls it (there are a lot of callsites - it's a complex path), but
the comment says that it's specifically for validating files associated with the
manifest key exist:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

There's also a TODO on the method that says to change the error to
base::string16, so I'm guessing it's just a bool at the moment because it hasn't
been fixed yet:
// TODO(yoz): Change error to base::string16. See crbug.com/71980.

[Yoyo Zhou, 2014-06-10 11:16:18]

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.h (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.h:133: virtual bool Validate(const
Extension* extension,
On 2014/06/10 05:44:13, sasha_b wrote:
> On 2014/06/05 03:59:17, benwells wrote:
> > Do you know what calls this? It is weird that it uses a string resource to
> > return errors. Normally developer errors aren't translated.
> 
> Not sure what calls it (there are a lot of callsites - it's a complex path),
but
> the comment says that it's specifically for validating files associated with
the
> manifest key exist:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

This is principally called from extensions::file_util::ValidateExtension. I
think it may produce user-visible strings if e.g. the extension gets corrupted
on disk.

> There's also a TODO on the method that says to change the error to
> base::string16, so I'm guessing it's just a bool at the moment because it
hasn't
> been fixed yet:
> // TODO(yoz): Change error to base::string16. See crbug.com/71980.

[benwells, 2014-06-11 00:47:29]

This needs a manifest test.

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.cc (right):

https://codereview.chromium.org/311193005/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.cc:272:
AboutPageHandler::AboutPageHandler() {
On 2014/06/10 05:44:13, sasha_b wrote:
> On 2014/06/05 03:59:17, benwells wrote:
> > This class is so similar to the options page class. I wonder if we could
pull
> > out the commonality into a superclass. What do you think that would look
like?
> > 
> > It would mean if we ever found / fixed a bug (e.g. in how we detect if a URL
> is
> > absolute) we wouldn't need to do it twice.
> 
> Not sure if they're _that_ similar. The options_page key allows for an
absolute
> URL for extensions, but a relative URL for packaged apps. But yes, I agree
that
> they are very similar.
> 
> A lot of it is already method calls, so I'm not sure how much could really be
> abstracted out. Also, the error messages are different constants.
> 
> If you have any ideas though, let me know. I can also add a TODO.

I could imagine a URLManifestHandler, which when overridden you just provided
some fields like whether it is absolute or relative and error strings. But,
whatevs.

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
File chrome/common/extensions/chrome_manifest_handlers.cc (right):

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
chrome/common/extensions/chrome_manifest_handlers.cc:71: (new
AboutPageHandler)->Register();
Nit: these seem to be alphabetical, so let's keep them that way.

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.cc (right):

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.cc:89: return
GetManifestURL(extension, keys::kOptionsPage);
Shouldn't this be keys::kAboutPage?

https://codereview.chromium.org/311193005/diff/40001/extensions/common/manife...
File extensions/common/manifest_constants.h (right):

https://codereview.chromium.org/311193005/diff/40001/extensions/common/manife...
extensions/common/manifest_constants.h:259: extern const char
kInvalidAboutPage[];
Nit: alphabetical.

[sashab, 2014-06-11 05:40:10]

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
File chrome/common/extensions/chrome_manifest_handlers.cc (right):

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
chrome/common/extensions/chrome_manifest_handlers.cc:71: (new
AboutPageHandler)->Register();
On 2014/06/11 00:47:29, benwells wrote:
> Nit: these seem to be alphabetical, so let's keep them that way.

Done.

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
File chrome/common/extensions/manifest_url_handler.cc (right):

https://codereview.chromium.org/311193005/diff/40001/chrome/common/extensions...
chrome/common/extensions/manifest_url_handler.cc:89: return
GetManifestURL(extension, keys::kOptionsPage);
On 2014/06/11 00:47:29, benwells wrote:
> Shouldn't this be keys::kAboutPage?

Good catch. Done.

https://codereview.chromium.org/311193005/diff/40001/extensions/common/manife...
File extensions/common/manifest_constants.h (right):

https://codereview.chromium.org/311193005/diff/40001/extensions/common/manife...
extensions/common/manifest_constants.h:259: extern const char
kInvalidAboutPage[];
On 2014/06/11 00:47:29, benwells wrote:
> Nit: alphabetical.

Weird, I've done this in the CC file but not here. Done.

[benwells, 2014-06-11 07:50:49]

lgtm withanit

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

https://codereview.chromium.org/311193005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:265: // to
each of their options pages.
Nit: options -> about.

[sashab, 2014-06-11 22:20:28]

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

https://codereview.chromium.org/311193005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:265: // to
each of their options pages.
On 2014/06/11 07:50:49, benwells wrote:
> Nit: options -> about.

Done.

[sashab, 2014-06-11 22:20:33]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:23:23]

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

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15715)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18848)

[commit-bot: I haz the power, 2014-06-12 00:11:51]

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

[commit-bot: I haz the power, 2014-06-12 00:11:53]

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

[sashab, 2014-06-12 00:19:05]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-12 00:20:12]

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

[commit-bot: I haz the power, 2014-06-12 01:09:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15782)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18914)

[commit-bot: I haz the power, 2014-06-12 01:16:36]

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

[commit-bot: I haz the power, 2014-06-12 01:16:37]

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

[sashab, 2014-06-12 03:44:54]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-12 03:46:44]

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

[commit-bot: I haz the power, 2014-06-12 08:59:12]

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

[commit-bot: I haz the power, 2014-06-12 08:59:13]

Failed to apply patch for
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
  |diff --git
a/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
b/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
  |index
36a9ec50d7c1c28716ef9b456770de0b6d0f368b..19008a100c4877542ee60d0a03bd724688688bb8
100644
  |--- a/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
  |+++ b/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:      
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
Index: chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
diff --git
a/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
b/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
index
36a9ec50d7c1c28716ef9b456770de0b6d0f368b..19008a100c4877542ee60d0a03bd724688688bb8
100644
--- a/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
+++ b/chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_tab.cc
@@ -262,7 +262,7 @@ bool AppInfoSummaryPanel::CanShowAppInWebStore() const {
 }
 
 // A small summary panel with a list of the app's imported modules, and a link
-// to each of their options pages.
+// to each of their about pages.
 class AppInfoImportedModulesPanel : public views::View,
                                     public views::LinkListener {
  public:
@@ -344,14 +344,13 @@ AppInfoImportedModulesPanel::AppInfoImportedModulesPanel(
     layout->StartRow(0, kModulesListColumnSetId);
     layout->AddView(name_label);
 
-    // If this app has an options page, display it as an 'about' link.
-    GURL options_page =
-        extensions::ManifestURL::GetOptionsPage(imported_module);
-    if (options_page != GURL::EmptyGURL()) {
+    // If this app has an about page, display it as an 'about' link.
+    GURL about_page = extensions::ManifestURL::GetAboutPage(imported_module);
+    if (about_page != GURL::EmptyGURL()) {
       views::Link* about_link = new views::Link(l10n_util::GetStringUTF16(
           IDS_APPLICATION_INFO_IMPORTED_MODULES_ABOUT_LINK_TEXT));
       about_link->set_listener(this);
-      about_links_[about_link] = options_page;
+      about_links_[about_link] = about_page;
       layout->AddView(about_link);
     }
   }

[sashab, 2014-06-12 22:51:34]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-12 22:54:24]

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

[commit-bot: I haz the power, 2014-06-12 22:57:18]

Change committed as 276840