Initial WebUI extension for enhanced bookmarks

Initial WebUI extension for enhanced bookmarks

This extension will be used for an experimental prototype of enhanced
bookmark styling in Chrome behind the enable_enhanced_bookmarks build
flag. This change duplicates the existing bookmarks WebUI extension as
a basis for future changes.

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

[Mike Wittman, 2013-09-26 23:44:53]

Hi James, please take a look.

[Mike Wittman, 2013-09-27 22:41:02]

Swapping reviewers since James will be OOO.

Evan: please review this as a whole.
Benjamin: please review chrome/{browser,common}/extensions
Scott: please do OWNERS review for chrome/browser/browser_resources.grd,
chrome/browser/ui/browser_navigator.cc

[sky, 2013-09-28 00:17:05]

My portion LGTM

[Evan Stade, 2013-09-30 18:52:20]

I think it would make sense to have this be a build-time flag (at least for the
time being) so you don't bloat the binary size while both old and new are still
around.

[Mike Wittman, 2013-09-30 19:22:22]

On 2013/09/30 18:52:20, Evan Stade wrote:
> I think it would make sense to have this be a build-time flag (at least for
the
> time being) so you don't bloat the binary size while both old and new are
still
> around.

Probably; looks like the extension is an extra 50k compressed. I'll update.

[Mike Wittman, 2013-10-01 00:16:20]

On 2013/09/30 19:22:22, Mike Wittman wrote:
> On 2013/09/30 18:52:20, Evan Stade wrote:
> > I think it would make sense to have this be a build-time flag (at least for
> the
> > time being) so you don't bloat the binary size while both old and new are
> still
> > around.
> 
> Probably; looks like the extension is an extra 50k compressed. I'll update.

Updated to use a build-time flag rather than run-time flag.

[not at google - send to devlin, 2013-10-01 00:21:01]

https://codereview.chromium.org/24891002/diff/1/chrome/browser/extensions/com...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/24891002/diff/1/chrome/browser/extensions/com...
chrome/browser/extensions/component_loader.cc:397: }
you could move both the normal and the enhanced extension registration to a
method.

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

https://codereview.chromium.org/24891002/diff/14001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.cc:281: #endif
so you're going to prevent extensions from overriding this?

[Evan Stade, 2013-10-01 17:35:26]

if I were you, I'd reuse the chrome://boomkarks URL for the enhanced bookmarks
so you don't need ifdefs everywhere (and presumably you'll eventually one day
want to make that change anyway). But lgtm either way.

https://codereview.chromium.org/24891002/diff/14001/chrome/browser/extensions...
File chrome/browser/extensions/extension_web_ui.cc (right):

https://codereview.chromium.org/24891002/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/extension_web_ui.cc:182: url->host() ==
chrome::kChromeUIEnhancedBookmarksHost)
nit: curlies

[Mike Wittman, 2013-10-01 17:56:05]

On 2013/10/01 17:35:26, Evan Stade wrote:
> if I were you, I'd reuse the chrome://boomkarks URL for the enhanced bookmarks
> so you don't need ifdefs everywhere (and presumably you'll eventually one day
> want to make that change anyway). But lgtm either way.

I strongly prefer to have separate URLs to facilitate testing potential changes
to the bookmark model against both the current and enhanced bookmarks.

https://codereview.chromium.org/24891002/diff/1/chrome/browser/extensions/com...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/24891002/diff/1/chrome/browser/extensions/com...
chrome/browser/extensions/component_loader.cc:397: }
On 2013/10/01 00:21:01, kalman wrote:
> you could move both the normal and the enhanced extension registration to a
> method.

Done.

https://codereview.chromium.org/24891002/diff/14001/chrome/browser/extensions...
File chrome/browser/extensions/extension_web_ui.cc (right):

https://codereview.chromium.org/24891002/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/extension_web_ui.cc:182: url->host() ==
chrome::kChromeUIEnhancedBookmarksHost)
On 2013/10/01 17:35:26, Evan Stade wrote:
> nit: curlies

I'll remove this change entirely since we don't need it yet.

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

https://codereview.chromium.org/24891002/diff/14001/chrome/common/extensions/...
chrome/common/extensions/manifest_url_handler.cc:281: #endif
On 2013/10/01 00:21:01, kalman wrote:
> so you're going to prevent extensions from overriding this?

Yes, added a check so that only component extensions can override this URL and
moved the logic down with the other #if defined.. blocks.

[not at google - send to devlin, 2013-10-01 17:58:16]

extensions lgtm

[commit-bot: I haz the power, 2013-10-01 18:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/24891002/30001

[commit-bot: I haz the power, 2013-10-01 22:05:25]

Change committed as 226314

[oshima, 2013-11-04 21:59:27]

Mike, CQ can't handle binary files, and as a result,
png files in this CL are all corrupted in repository.

Are these file really used?

[Mike Wittman, 2013-11-04 22:11:21]

On 2013/11/04 21:59:27, oshima wrote:
> Mike, CQ can't handle binary files, and as a result,
> png files in this CL are all corrupted in repository.
> 
> Are these file really used?

Yes, they're used by this experimental extension. We haven't begun actively
prototyping with the extension yet, which is why no one has raised the issue.

I can create a CL with the appropriate image file contents to fix this.