Manifest handler for all keys background-related.

Manifest handler for all keys background-related.

Changes the manifest handler registration interface to (new Handler)->Register() and requires handlers to declare what keys they parse in Keys().
Also fixes const correctness in Manifest::Get* and ManifestHandler.

Based on https://codereview.chromium.org/12256031/.

BUG=159265
TBR=ben@chromium.org

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

[Yoyo Zhou, 2013-02-15 02:28:25]

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/w...
File chrome/common/extensions/web_intents_handler.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/w...
chrome/common/extensions/web_intents_handler.cc:212: const
std::vector<std::string>& WebIntentsHandler::PrerequisiteKeys() const {
I went to all this effort and then found out WebIntents is going to be deleted,
so feel free to ignore this file. I'm deleting it in another CL.

[Matt Perry, 2013-02-15 20:15:02]

Noting for posterity: I had some concerns about whether it was worth pulling
BackgroundInfo out of Extension, since it is such a core concept to extensions.
But you pointed out not all extension types have background pages - like themes.
I can imagine a world where someone would want a build of Chrome that supports
themes but not extensions in general, so modularizing background pages makes
sense.

https://codereview.chromium.org/12253022/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.cc:194:
std::vector<std::string> background_keys(BackgroundManifestHandler::keys());
This is cumbersome. How about making it a method on the handler:
  new BackgroundManifestHandler().Register();

https://codereview.chromium.org/12253022/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.cc:194:
std::vector<std::string> background_keys(BackgroundManifestHandler::keys());
Also, it feels a little out of place here, but I can't think of a better place.
Maybe we should have an ExtensionCore class that registers all the handlers that
are central to extensions. Just spitballing...

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
File chrome/common/extensions/background_info.h (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
chrome/common/extensions/background_info.h:18: struct BackgroundInfo : public
Extension::ManifestData {
Make this a class.

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
chrome/common/extensions/background_info.h:36: GURL background_url;
These should all be private. You only access them from BackgroundInfo.

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1499:
tmp_isolation->GetAsList(&isolation_list);
As long as you using this method, combine it with the previous if.
  if (!tmp_iso->GetAsList()) error

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util.cc:377:
extensions::BackgroundInfo::GetBackgroundScripts(extension).empty()) {
already have background_scripts

[Yoyo Zhou, 2013-02-16 00:54:04]

Haven't finished refactoring the 85 callsites of ManifestHandler::Register, but
I don't understand one of your comments.

https://codereview.chromium.org/12253022/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.cc:194:
std::vector<std::string> background_keys(BackgroundManifestHandler::keys());
On 2013/02/15 20:15:02, Matt Perry wrote:
> Also, it feels a little out of place here, but I can't think of a better
place.
> Maybe we should have an ExtensionCore class that registers all the handlers
that
> are central to extensions. Just spitballing...

Maybe we could call it ExtensionService? =)

Seriously, I think there are other such things that are specific to extensions
that are extensions/apps/platform apps.

https://codereview.chromium.org/12253022/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.cc:194:
std::vector<std::string> background_keys(BackgroundManifestHandler::keys());
On 2013/02/15 20:15:02, Matt Perry wrote:
> This is cumbersome. How about making it a method on the handler:
>   new BackgroundManifestHandler().Register();

I definitely considered this but I thought the ownership would be kind of
opaque. No more opaque than it is now, though. I'll change this throughout.

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
File chrome/common/extensions/background_info.h (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
chrome/common/extensions/background_info.h:18: struct BackgroundInfo : public
Extension::ManifestData {
On 2013/02/15 20:15:02, Matt Perry wrote:
> Make this a class.

Done.

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
chrome/common/extensions/background_info.h:36: GURL background_url;
On 2013/02/15 20:15:02, Matt Perry wrote:
> These should all be private. You only access them from BackgroundInfo.

Done; also needed to make the load functions member functions.

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1499:
tmp_isolation->GetAsList(&isolation_list);
On 2013/02/15 20:15:02, Matt Perry wrote:
> As long as you using this method, combine it with the previous if.
>   if (!tmp_iso->GetAsList()) error

Done.

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util.cc:377:
extensions::BackgroundInfo::GetBackgroundScripts(extension).empty()) {
On 2013/02/15 20:15:02, Matt Perry wrote:
> already have background_scripts

Not sure I understand this comment.

[Matt Perry, 2013-02-16 01:16:52]

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
File chrome/common/extensions/background_info.h (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/b...
chrome/common/extensions/background_info.h:36: GURL background_url;
On 2013/02/16 00:54:04, Yoyo Zhou wrote:
> On 2013/02/15 20:15:02, Matt Perry wrote:
> > These should all be private. You only access them from BackgroundInfo.
> 
> Done; also needed to make the load functions member functions.

Aw, didn't see that originally. Another thing you could do is make
BackgroundInfo only contain the static methods. Then in the .cc file, define
BackgroundInfoImpl that contains the actual ManifestData, which BackgroundInfo
uses to implement its methods.

It's not a big deal though. This is fine as-is.

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util.cc (right):

https://codereview.chromium.org/12253022/diff/5001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util.cc:377:
extensions::BackgroundInfo::GetBackgroundScripts(extension).empty()) {
On 2013/02/16 00:54:04, Yoyo Zhou wrote:
> On 2013/02/15 20:15:02, Matt Perry wrote:
> > already have background_scripts
> 
> Not sure I understand this comment.

you cache the scripts in |background_scripts| above - no need to call the
function again.

[Yoyo Zhou, 2013-02-26 23:39:41]

Please look again.

[Matt Perry, 2013-02-26 23:53:01]

Good cleanup! LGTM

[Yoyo Zhou, 2013-02-27 03:11:44]

TBR=ben for mechanical refactoring

[commit-bot: I haz the power, 2013-02-27 04:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12253022/38006

[commit-bot: I haz the power, 2013-02-27 04:59:08]

Failed to apply patch for
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/media_gallery/media_file_system_registry_unittest.cc
  Hunk #1 FAILED at 31.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/media_gallery/media_file_system_registry_unittest.cc.rej

Patch:       chrome/browser/media_gallery/media_file_system_registry_unittest.cc
Index: chrome/browser/media_gallery/media_file_system_registry_unittest.cc
diff --git a/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
b/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
index
6867e7d160c39e3206f6f14797d4330ac0facfd9..00920ca87300a2aba4e06489200ed21240adc49e
100644
--- a/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
+++ b/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
@@ -31,7 +31,9 @@
 #include "chrome/browser/storage_monitor/removable_device_constants.h"
 #include "chrome/browser/storage_monitor/removable_storage_notifications.h"
 #include
"chrome/browser/storage_monitor/test_removable_storage_notifications.h"
+#include "chrome/common/extensions/background_info.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/manifest_handler.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -789,6 +791,7 @@ void MediaFileSystemRegistryTest::SetUp() {
   TestMediaStorageUtil::SetTestingMode();
   test_file_system_context_ = new TestMediaFileSystemContext(
       g_browser_process->media_file_system_registry());
+  (new extensions::BackgroundManifestHandler)->Register();
 
   ASSERT_TRUE(galleries_dir_.CreateUniqueTempDir());
   empty_dir_ = galleries_dir_.path().AppendASCII("empty");
@@ -806,6 +809,7 @@ void MediaFileSystemRegistryTest::TearDown() {
   EXPECT_EQ(0U, registry->GetExtensionGalleriesHostCountForTests());
   BrowserThread::GetBlockingPool()->FlushForTesting();
   MessageLoop::current()->RunUntilIdle();
+  extensions::ManifestHandler::ClearRegistryForTesting();
 }
 
 ///////////

[commit-bot: I haz the power, 2013-02-27 22:49:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12253022/52001

[Yoyo Zhou, 2013-02-27 22:50:06]

Another minor cleanup: making PrerequisiteKeys not return by reference.

[commit-bot: I haz the power, 2013-02-28 00:40:03]

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

[commit-bot: I haz the power, 2013-02-28 17:50:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12253022/52001

[commit-bot: I haz the power, 2013-02-28 17:50:55]

Failed to apply patch for
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/media_gallery/media_file_system_registry_unittest.cc
  Hunk #1 FAILED at 31.
  Hunk #2 FAILED at 789.
  Hunk #3 FAILED at 806.
  3 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/media_gallery/media_file_system_registry_unittest.cc.rej

Patch:       chrome/browser/media_gallery/media_file_system_registry_unittest.cc
Index: chrome/browser/media_gallery/media_file_system_registry_unittest.cc
diff --git a/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
b/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
index
9df3f48165273a88fd09a623076367c31401d7ca..3ec41976f5440c40b51030bbd412bb051fb286e9
100644
--- a/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
+++ b/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
@@ -31,7 +31,9 @@
 #include "chrome/browser/storage_monitor/removable_device_constants.h"
 #include "chrome/browser/storage_monitor/storage_monitor.h"
 #include "chrome/browser/storage_monitor/test_storage_monitor.h"
+#include "chrome/common/extensions/background_info.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/extensions/manifest_handler.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -789,6 +791,7 @@ void MediaFileSystemRegistryTest::SetUp() {
   TestMediaStorageUtil::SetTestingMode();
   test_file_system_context_ = new TestMediaFileSystemContext(
       g_browser_process->media_file_system_registry());
+  (new extensions::BackgroundManifestHandler)->Register();
 
   ASSERT_TRUE(galleries_dir_.CreateUniqueTempDir());
   empty_dir_ = galleries_dir_.path().AppendASCII("empty");
@@ -806,6 +809,7 @@ void MediaFileSystemRegistryTest::TearDown() {
   EXPECT_EQ(0U, registry->GetExtensionGalleriesHostCountForTests());
   BrowserThread::GetBlockingPool()->FlushForTesting();
   MessageLoop::current()->RunUntilIdle();
+  extensions::ManifestHandler::ClearRegistryForTesting();
 }
 
 ///////////

[commit-bot: I haz the power, 2013-02-28 18:37:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12253022/75001

[commit-bot: I haz the power, 2013-02-28 21:12:18]

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

[commit-bot: I haz the power, 2013-02-28 21:12:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12253022/78002

[commit-bot: I haz the power, 2013-02-28 21:30:37]

Failed to trigger a try job on mac
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-02-28 21:30:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12253022/78005

[commit-bot: I haz the power, 2013-03-01 01:29:32]

Change committed as 185391