Add command line option to use a tabbed bookmark manager

Add command line option to use a tabbed bookmark manager.

Start with --enable-tabbed-bookmark-manager. This will show chrome://bookmarks in a tab instead of the old bookmark manager. However, to get the bookmark manager extension to show you need to use --load-extension=path_to_extension. The extension uses the chrome_url_override to show the extension instead of the bookmark manager.

BUG=4890
TEST=See description


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40468

[arv (Not doing code reviews), 2010-02-24 22:35:19]

I'll have to submit the images separately but I'll split this after the reviews.

[Aaron Boodman, 2010-02-26 06:35:43]

http://codereview.chromium.org/661019/diff/40/47
File chrome/browser/dom_ui/bookmarks_ui.cc (right):

http://codereview.chromium.org/661019/diff/40/47#newcode31
chrome/browser/dom_ui/bookmarks_ui.cc:31: static const base::StringPiece
bookmarks_html(
I think there is no need for this HTML resource. Since this is just going to get
overridden by the extension, you can just have it return an empty string (and
add a comment explaining what is supposed to happen in this method).

http://codereview.chromium.org/661019/diff/40/48
File chrome/browser/dom_ui/bookmarks_ui.h (right):

http://codereview.chromium.org/661019/diff/40/48#newcode16
chrome/browser/dom_ui/bookmarks_ui.h:16: class BookmarksUIHTMLSource : public
ChromeURLDataManager::DataSource {
class comments pls

http://codereview.chromium.org/661019/diff/40/48#newcode25
chrome/browser/dom_ui/bookmarks_ui.h:25: virtual std::string GetMimeType(const
std::string&) const {
Seems worthwhile to check the path is what you're expecting.

http://codereview.chromium.org/661019/diff/40/48#newcode30
chrome/browser/dom_ui/bookmarks_ui.h:30: ~BookmarksUIHTMLSource() {}
No need for empty dctors, they are generated by the compiler.

http://codereview.chromium.org/661019/diff/40/52
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/661019/diff/40/52#newcode69
chrome/common/extensions/extension.cc:69: // experimental APIs.
I think the place to do this is in ExtensionFunctionDispatcher where the
functions are dispatched. You can just skip the experimental ones if
kEnableExperimentalExtensinApis is not present or the extension being dispatched
from is not the bookmark manager.

[arv (Not doing code reviews), 2010-02-26 20:26:51]

http://codereview.chromium.org/661019/diff/40/47
File chrome/browser/dom_ui/bookmarks_ui.cc (right):

http://codereview.chromium.org/661019/diff/40/47#newcode31
chrome/browser/dom_ui/bookmarks_ui.cc:31: static const base::StringPiece
bookmarks_html(
On 2010/02/26 06:35:43, Aaron Boodman wrote:
> I think there is no need for this HTML resource. Since this is just going to
get
> overridden by the extension, you can just have it return an empty string (and
> add a comment explaining what is supposed to happen in this method).

I added a NOTREACHED() instead.

http://codereview.chromium.org/661019/diff/40/48
File chrome/browser/dom_ui/bookmarks_ui.h (right):

http://codereview.chromium.org/661019/diff/40/48#newcode16
chrome/browser/dom_ui/bookmarks_ui.h:16: class BookmarksUIHTMLSource : public
ChromeURLDataManager::DataSource {
On 2010/02/26 06:35:43, Aaron Boodman wrote:
> class comments pls

Done.

http://codereview.chromium.org/661019/diff/40/48#newcode25
chrome/browser/dom_ui/bookmarks_ui.h:25: virtual std::string GetMimeType(const
std::string&) const {
On 2010/02/26 06:35:43, Aaron Boodman wrote:
> Seems worthwhile to check the path is what you're expecting.

I'm not sure it is worth it. I would have to strip #... and ?... (hash params
are being used).

http://codereview.chromium.org/661019/diff/40/48#newcode30
chrome/browser/dom_ui/bookmarks_ui.h:30: ~BookmarksUIHTMLSource() {}
On 2010/02/26 06:35:43, Aaron Boodman wrote:
> No need for empty dctors, they are generated by the compiler.

Done.

http://codereview.chromium.org/661019/diff/40/52
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/661019/diff/40/52#newcode69
chrome/common/extensions/extension.cc:69: // experimental APIs.
On 2010/02/26 06:35:43, Aaron Boodman wrote:
> I think the place to do this is in ExtensionFunctionDispatcher where the
> functions are dispatched. You can just skip the experimental ones if
> kEnableExperimentalExtensinApis is not present or the extension being
dispatched
> from is not the bookmark manager.

The main issue at the moment is that there is no way to test if an extension is
the bookmark manager.

I think the best way would be to flag an extension as "internal" and then move
chrome.experimental.bookmarkManager to chrome.internal.bookmarkManager and then
let all "internal" extension have access to the "internal" API permission.

[Aaron Boodman, 2010-03-02 18:35:05]

lgtm

http://codereview.chromium.org/661019/diff/40/48
File chrome/browser/dom_ui/bookmarks_ui.h (right):

http://codereview.chromium.org/661019/diff/40/48#newcode25
chrome/browser/dom_ui/bookmarks_ui.h:25: virtual std::string GetMimeType(const
std::string&) const {
On 2010/02/26 20:26:51, arv wrote:
> On 2010/02/26 06:35:43, Aaron Boodman wrote:
> > Seems worthwhile to check the path is what you're expecting.
> 
> I'm not sure it is worth it. I would have to strip #... and ?... (hash params
> are being used).

GURL can do that for you quite easily, but whatevs. Will leave it to you.

http://codereview.chromium.org/661019/diff/40/52
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/661019/diff/40/52#newcode69
chrome/common/extensions/extension.cc:69: // experimental APIs.
On 2010/02/26 20:26:51, arv wrote:
> On 2010/02/26 06:35:43, Aaron Boodman wrote:
> > I think the place to do this is in ExtensionFunctionDispatcher where the
> > functions are dispatched. You can just skip the experimental ones if
> > kEnableExperimentalExtensinApis is not present or the extension being
> dispatched
> > from is not the bookmark manager.
> 
> The main issue at the moment is that there is no way to test if an extension
is
> the bookmark manager.
> 
> I think the best way would be to flag an extension as "internal" and then move
> chrome.experimental.bookmarkManager to chrome.internal.bookmarkManager and
then
> let all "internal" extension have access to the "internal" API permission.

I'm afraid of having multiple special classes of extension APIs. Let's leave it
experimental, but otherwise do your suggestion. I can add it to my change I
think.

[arv (Not doing code reviews), 2010-03-02 20:13:54]

http://codereview.chromium.org/661019/diff/40/48
File chrome/browser/dom_ui/bookmarks_ui.h (right):

http://codereview.chromium.org/661019/diff/40/48#newcode25
chrome/browser/dom_ui/bookmarks_ui.h:25: virtual std::string GetMimeType(const
std::string&) const {
On 2010/03/02 18:35:05, Aaron Boodman wrote:
> On 2010/02/26 20:26:51, arv wrote:
> > On 2010/02/26 06:35:43, Aaron Boodman wrote:
> > > Seems worthwhile to check the path is what you're expecting.
> > 
> > I'm not sure it is worth it. I would have to strip #... and ?... (hash
params
> > are being used).
> 
> GURL can do that for you quite easily, but whatevs. Will leave it to you.

I changed this to a NOTREACHED() as well since we should not be hitting this
either.

http://codereview.chromium.org/661019/diff/40/52
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/661019/diff/40/52#newcode69
chrome/common/extensions/extension.cc:69: // experimental APIs.
On 2010/03/02 18:35:05, Aaron Boodman wrote:
> On 2010/02/26 20:26:51, arv wrote:
> > On 2010/02/26 06:35:43, Aaron Boodman wrote:
> > > I think the place to do this is in ExtensionFunctionDispatcher where the
> > > functions are dispatched. You can just skip the experimental ones if
> > > kEnableExperimentalExtensinApis is not present or the extension being
> > dispatched
> > > from is not the bookmark manager.
> > 
> > The main issue at the moment is that there is no way to test if an extension
> is
> > the bookmark manager.
> > 
> > I think the best way would be to flag an extension as "internal" and then
move
> > chrome.experimental.bookmarkManager to chrome.internal.bookmarkManager and
> then
> > let all "internal" extension have access to the "internal" API permission.
> 
> I'm afraid of having multiple special classes of extension APIs. Let's leave
it
> experimental, but otherwise do your suggestion. I can add it to my change I
> think.

After your CL is in we can remove "experimental" from the BMM manifest and
update EFD to handle the BMM. I'll file a bug.

[feldstein, 2010-03-02 20:54:10]

LGTM

On 2010/03/02 20:13:54, arv wrote:
> http://codereview.chromium.org/661019/diff/40/48
> File chrome/browser/dom_ui/bookmarks_ui.h (right):
> 
> http://codereview.chromium.org/661019/diff/40/48#newcode25
> chrome/browser/dom_ui/bookmarks_ui.h:25: virtual std::string GetMimeType(const
> std::string&) const {
> On 2010/03/02 18:35:05, Aaron Boodman wrote:
> > On 2010/02/26 20:26:51, arv wrote:
> > > On 2010/02/26 06:35:43, Aaron Boodman wrote:
> > > > Seems worthwhile to check the path is what you're expecting.
> > > 
> > > I'm not sure it is worth it. I would have to strip #... and ?... (hash
> params
> > > are being used).
> > 
> > GURL can do that for you quite easily, but whatevs. Will leave it to you.
> 
> I changed this to a NOTREACHED() as well since we should not be hitting this
> either.
> 
> http://codereview.chromium.org/661019/diff/40/52
> File chrome/common/extensions/extension.cc (right):
> 
> http://codereview.chromium.org/661019/diff/40/52#newcode69
> chrome/common/extensions/extension.cc:69: // experimental APIs.
> On 2010/03/02 18:35:05, Aaron Boodman wrote:
> > On 2010/02/26 20:26:51, arv wrote:
> > > On 2010/02/26 06:35:43, Aaron Boodman wrote:
> > > > I think the place to do this is in ExtensionFunctionDispatcher where the
> > > > functions are dispatched. You can just skip the experimental ones if
> > > > kEnableExperimentalExtensinApis is not present or the extension being
> > > dispatched
> > > > from is not the bookmark manager.
> > > 
> > > The main issue at the moment is that there is no way to test if an
extension
> > is
> > > the bookmark manager.
> > > 
> > > I think the best way would be to flag an extension as "internal" and then
> move
> > > chrome.experimental.bookmarkManager to chrome.internal.bookmarkManager and
> > then
> > > let all "internal" extension have access to the "internal" API permission.
> > 
> > I'm afraid of having multiple special classes of extension APIs. Let's leave
> it
> > experimental, but otherwise do your suggestion. I can add it to my change I
> > think.
> 
> After your CL is in we can remove "experimental" from the BMM manifest and
> update EFD to handle the BMM. I'll file a bug.