Added proper extension manifest parser for launcher_page.

Added proper extension manifest parser for launcher_page.

Previously, AppListViewDelegate manually parsed the manifest. We now
have a proper parser class that generates appropriate errors for invalid
launcher_page fields in a manifest, with full valid and invalid unit
tests.

BUG=399131
Committed: https://crrev.com/2be0d05247cc0f2f236540bd19b62a41896dd2ae
Cr-Commit-Position: refs/heads/master@{#291856}

[Matt Giuca, 2014-08-20 06:55:42]

calamity@: Sorry bro, I choose YOU to review this rather large CL. (It feels a
lot simpler than its length suggests, though.)

I've been reviewing these with benwells but I think he's got a lot of reviews on
his plate.

I will pick OWNERS later, just need an initial look from you.

[calamity, 2014-08-21 02:53:09]

lgtm w/ nits.

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
File
chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc
(right):

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc:33:

nit: remove whitespace.

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc:93:
RunTestcases(testcases, arraysize(testcases),
nit: add whitespace back.

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
File
chrome/common/extensions/manifest_tests/extension_manifests_launcher_page_unittest.cc
(right):

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_launcher_page_unittest.cc:6:
#include "base/memory/linked_ptr.h"
I think you can remove this.

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_launcher_page_unittest.cc:24:
class LauncherPageManifestTest : public ExtensionManifestTest {};
I've seen typedef around more often. Any reason not to use that here?

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
File extensions/common/manifest_handlers/launcher_page_info.cc (right):

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:35: }
nit: blank line after early return

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:41: }
nit: blank line after early return

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:54:
extension->GetManifestData(manifest_keys::kLauncherPage));
Other manufest handlers seem to have a static Get() method that does this cast
instead of casting it everywhere. See icons_handler.h.

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:60: }
nit: blank line after early return

[Matt Giuca, 2014-08-22 02:48:41]

benwells@: for OWNERS of:
chrome/common/extensions/manifest_tests/*
extensions/common/*

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
File
chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc
(right):

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc:33:

On 2014/08/21 02:53:08, calamity wrote:
> nit: remove whitespace.

Done.

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc:93:
RunTestcases(testcases, arraysize(testcases),
On 2014/08/21 02:53:08, calamity wrote:
> nit: add whitespace back.

Done.

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
File
chrome/common/extensions/manifest_tests/extension_manifests_launcher_page_unittest.cc
(right):

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_launcher_page_unittest.cc:6:
#include "base/memory/linked_ptr.h"
On 2014/08/21 02:53:08, calamity wrote:
> I think you can remove this.

Done.

https://codereview.chromium.org/489753002/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_tests/extension_manifests_launcher_page_unittest.cc:24:
class LauncherPageManifestTest : public ExtensionManifestTest {};
On 2014/08/21 02:53:08, calamity wrote:
> I've seen typedef around more often. Any reason not to use that here?

Done.

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
File extensions/common/manifest_handlers/launcher_page_info.cc (right):

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:35: }
On 2014/08/21 02:53:08, calamity wrote:
> nit: blank line after early return

Done.

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:41: }
On 2014/08/21 02:53:08, calamity wrote:
> nit: blank line after early return

Done.

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:54:
extension->GetManifestData(manifest_keys::kLauncherPage));
Actually icons_handler is what I copied this off and it does the static_cast
manually (albeit only in one place).

But I'll do this anyway because it's much neater and safer.

https://codereview.chromium.org/489753002/diff/1/extensions/common/manifest_h...
extensions/common/manifest_handlers/launcher_page_info.cc:60: }
On 2014/08/21 02:53:08, calamity wrote:
> nit: blank line after early return

Done.

[Matt Giuca, 2014-08-26 02:54:04]

benwells: ping

[benwells, 2014-08-26 03:29:13]

lgtm

https://codereview.chromium.org/489753002/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/489753002/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:142:
urls->push_back(extension->GetResourceURL(info->page));
Nit: please add a blank line after continue.

[Matt Giuca, 2014-08-26 03:36:30]

https://codereview.chromium.org/489753002/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/489753002/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:142:
urls->push_back(extension->GetResourceURL(info->page));
On 2014/08/26 03:29:12, benwells wrote:
> Nit: please add a blank line after continue.

Done.

[Matt Giuca, 2014-08-26 03:36:41]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2014-08-26 03:36:46]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-08-26 03:37:04]

Patchset #5 (id:80001) has been deleted

[Matt Giuca, 2014-08-26 03:39:02]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-08-26 03:40:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/489753002/120001

[commit-bot: I haz the power, 2014-08-26 06:42:31]

Committed patchset #6 (120001) as 5f06f2017bc7514afbe8b58cc3c5907a6ca78a81

[commit-bot: I haz the power, 2014-09-10 02:41:03]

Patchset 6 (id:??) landed as
https://crrev.com/2be0d05247cc0f2f236540bd19b62a41896dd2ae
Cr-Commit-Position: refs/heads/master@{#291856}