override chrome:// URLs via extensions.

override chrome:// URLs via extensions.
Overrides are declared in an extension's manifest.  The last one installed
wins.  However, we keep a list of those installed per page so that priority
is preserved and so that uninstall will revert to a previous state.

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

[Erik does not do reviews, 2009-08-22 00:00:48]

brettw: DOMUI stuff
aa: extensions

This isn't quite ready for submission.  I have two important TODOs to finish,
and I need to write a test for this code.  However, I wanted to get it out to
make sure I'm on the right track here.

[brettw, 2009-08-22 01:20:35]

I think this looks like a good start.

http://codereview.chromium.org/174277/diff/1001/1006
File chrome/browser/dom_ui/extension_override_ui.cc (right):

http://codereview.chromium.org/174277/diff/1001/1006#newcode49
Line 49: : DataSource(source_name, MessageLoop::current()),
Style nit: this should only be indented 4 spaces.

http://codereview.chromium.org/174277/diff/1001/1006#newcode61
Line 61: file_util::ReadFileToString(file_path, &file_contents);
So this is basically loading a file provided by the extension as the new tab
page contents, right?

This should be done asnychronously on the UI thread in the final version.

[Erik does not do reviews, 2009-08-24 16:00:46]

http://codereview.chromium.org/174277/diff/1001/1006
File chrome/browser/dom_ui/extension_override_ui.cc (right):

http://codereview.chromium.org/174277/diff/1001/1006#newcode61
Line 61: file_util::ReadFileToString(file_path, &file_contents);
On 2009/08/22 01:20:35, brettw wrote:
> So this is basically loading a file provided by the extension as the new tab
> page contents, right?
> 
> This should be done asnychronously on the UI thread in the final version.

On the UI thread?  Did you mean the IO or File thread?

FYI, I copied this pattern directly from new_tab_ui.cc.  I had assumed that the
reason it was done this way was that it was already happening on the correct
thread and hadn't really looked into it in more detail.

[brettw, 2009-08-24 16:14:10]

On 2009/08/24 16:00:46, Erik Kay wrote:
> http://codereview.chromium.org/174277/diff/1001/1006
> File chrome/browser/dom_ui/extension_override_ui.cc (right):
> 
> http://codereview.chromium.org/174277/diff/1001/1006#newcode61
> Line 61: file_util::ReadFileToString(file_path, &file_contents);
> On 2009/08/22 01:20:35, brettw wrote:
> > So this is basically loading a file provided by the extension as the new tab
> > page contents, right?
> > 
> > This should be done asnychronously on the UI thread in the final version.
> 
> On the UI thread?  Did you mean the IO or File thread?

I meant the file thread, sorry.

> FYI, I copied this pattern directly from new_tab_ui.cc.  I had assumed that
the
> reason it was done this way was that it was already happening on the correct
> thread and hadn't really looked into it in more detail.

The code in question you copied looks like it was testing code that isn't used
anywhere (executes only from a command line switch). So it's not good for a
general use case.

[Erik does not do reviews, 2009-08-24 17:39:35]

On 2009/08/24 16:14:10, brettw wrote:
> On 2009/08/24 16:00:46, Erik Kay wrote:
> > http://codereview.chromium.org/174277/diff/1001/1006
> > File chrome/browser/dom_ui/extension_override_ui.cc (right):
> > 
> > http://codereview.chromium.org/174277/diff/1001/1006#newcode61
> > Line 61: file_util::ReadFileToString(file_path, &file_contents);
> > On 2009/08/22 01:20:35, brettw wrote:
> > > So this is basically loading a file provided by the extension as the new
tab
> > > page contents, right?
> > > 
> > > This should be done asnychronously on the UI thread in the final version.
> > 
> > On the UI thread?  Did you mean the IO or File thread?
> 
> I meant the file thread, sorry.
> 
> > FYI, I copied this pattern directly from new_tab_ui.cc.  I had assumed that
> the
> > reason it was done this way was that it was already happening on the correct
> > thread and hadn't really looked into it in more detail.
> 
> The code in question you copied looks like it was testing code that isn't used
> anywhere (executes only from a command line switch). So it's not good for a
> general use case.

Yep.  I fixed my code here.

I also now handle the unregister case.

[Aaron Boodman, 2009-08-24 17:49:04]

http://codereview.chromium.org/174277/diff/1017/31
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/174277/diff/1017/31#newcode76
Line 76: if (ExtensionOverrideUI::CreateDOMUI(url, tab_contents, new_ui))
It doesn't look like these pages will have access to any of the extension APIs
like chrome.*, because that is implemented in ExtensionDOMUI.

I think that instead of having your own subclass of DOMUI, you should use the
existing ExtensionDOMUI here.

http://codereview.chromium.org/174277/diff/1017/32
File chrome/browser/dom_ui/extension_override_ui.cc (right):

http://codereview.chromium.org/174277/diff/1017/32#newcode103
Line 103: PrefService* prefs = profile->GetPrefs();
Why do we need to keep a separate copy of this state from what is already in the
manifest? They seem likely to get out of sync.

It seems better to have the intercept code refer to the list of Extension
instances in ExtensionsService to figure out if anybody wants to intercept.

There is the issue of priority. I would suggest that for a first version the
behavior is undefined when multiple extensions override the same chrome:// URL.
But we could add a general purpose order or priority to extension installation
later.

http://codereview.chromium.org/174277/diff/1017/36
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/174277/diff/1017/36#newcode976
Line 976: static_cast<DictionaryValue*>(overrides->DeepCopy()));
You also need to validate that these resources exist in extension_file_util.

[brettw, 2009-08-24 20:20:32]

http://codereview.chromium.org/174277/diff/1017/31
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/174277/diff/1017/31#newcode76
Line 76: if (ExtensionOverrideUI::CreateDOMUI(url, tab_contents, new_ui))
On 2009/08/24 17:49:04, Aaron Boodman wrote:
> It doesn't look like these pages will have access to any of the extension APIs
> like chrome.*, because that is implemented in ExtensionDOMUI.
> 
> I think that instead of having your own subclass of DOMUI, you should use the
> existing ExtensionDOMUI here.

I think this should work.

[Erik does not do reviews, 2009-08-25 21:11:09]

I've changed the CL a fair amount.  Rather than hooking into DOMUIFactory and
using its own DOMUI subclass, it's using BrowserURLHandler, which allows you to
supply a different underlying URL than what's displayed to the user (we use this
for about:, view-source:, and a few others).  This happens very early on, so we
don't need to do any wacky redirects or anything similar.  This seems to work
well, so I'm starting to work on a test for it.

[brettw, 2009-08-25 22:24:11]

LGTM

http://codereview.chromium.org/174277/diff/1021/51
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/174277/diff/1021/51#newcode40
Line 40: GURL url = controller.pending_entry()->url();
Can you use a GURL& here instead? Seems kind of unnecessary to copy here.

http://codereview.chromium.org/174277/diff/1021/52
File chrome/browser/extensions/extension_dom_ui.h (right):

http://codereview.chromium.org/174277/diff/1021/52#newcode64
Line 64: // Reset the EFD.
This commend is actually less clear than if it wasn't here. How about mentining
what it does to the parameter?

[Aaron Boodman, 2009-08-26 19:56:54]

I wonder if we should only allow the NTP to be released at first. If we did
that, we could add extensions to the UI where you select what your homepage
would be. We wouldn't need all the logic and state to fall back to other
extensions because it would be under the user's control which page is the home
page.

http://codereview.chromium.org/174277/diff/1021/51
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/174277/diff/1021/51#newcode32
Line 32: should_hide_url_ = false;
Shouldn't we always hide the URL?

http://codereview.chromium.org/174277/diff/1021/51#newcode39
Line 39: NavigationController& controller = tab_contents()->controller();
A comment here explaining the reason you're not using tab_contents()->GetURL()
would be useful.

[tim (not reviewing), 2009-08-26 20:47:24]

http://codereview.chromium.org/174277/diff/1021/51
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/174277/diff/1021/51#newcode89
Line 89: return false;
I don't know the whole context here but it looks like it'd be pretty easy for
someone to write an extension that would phish our own GAIA login HTML for sync
setup with their own HTML and steal credentials. Is that troublesome?

[Erik does not do reviews, 2009-08-26 21:10:53]

new snapshot uploaded

> I wonder if we should only allow the NTP to be released 
> at first. If we did that, we could add extensions to the
> UI where you select what your homepage would be. 

I'm fine restricting overrides to newtab with the initial release (since that
was the initial goal here anyway), but I don't think it helps that much with the
UI or the fallback logic.  You still need fallback for uninstall, disabling,
etc.  For the UI, NTP and home page are totally different, so it wouldn't be an
extension so much as a new section to the options UI, which would be the same
for any of the pages we allow to be overridden.

http://codereview.chromium.org/174277/diff/1021/51
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/174277/diff/1021/51#newcode32
Line 32: should_hide_url_ = false;
On 2009/08/26 19:56:54, Aaron Boodman wrote:
> Shouldn't we always hide the URL?

No.  For chrome:// pages, only newtab hides its URL. (I believe it only does
this to make navigation easier / more clear)

http://codereview.chromium.org/174277/diff/1021/51#newcode39
Line 39: NavigationController& controller = tab_contents()->controller();
On 2009/08/26 19:56:54, Aaron Boodman wrote:
> A comment here explaining the reason you're not using tab_contents()->GetURL()
> would be useful.

done

http://codereview.chromium.org/174277/diff/1021/51#newcode40
Line 40: GURL url = controller.pending_entry()->url();
On 2009/08/25 22:24:11, brettw wrote:
> Can you use a GURL& here instead? Seems kind of unnecessary to copy here.

fixed

[Erik does not do reviews, 2009-08-26 21:13:09]

http://codereview.chromium.org/174277/diff/1021/51
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/174277/diff/1021/51#newcode89
Line 89: return false;
On 2009/08/26 20:47:24, timsteele wrote:
> I don't know the whole context here but it looks like it'd be pretty easy for
> someone to write an extension that would phish our own GAIA login HTML for
sync
> setup with their own HTML and steal credentials. Is that troublesome?

This is among the least dangerous things that extensions can do.  We're
protected by the gallery, reputation, blacklisting, etc.

[Aaron Boodman, 2009-08-26 21:22:28]

LGTM

On 2009/08/26 21:10:53, Erik Kay wrote:
> new snapshot uploaded
> 
> > I wonder if we should only allow the NTP to be released 
> > at first. If we did that, we could add extensions to the
> > UI where you select what your homepage would be. 
> 
> I'm fine restricting overrides to newtab with the initial release (since that
> was the initial goal here anyway)

Ok, my vote is to do that then until we get more people wanting to override
other pages.

> but I don't think it helps that much with the
> UI or the fallback logic.  You still need fallback for uninstall, disabling,
> etc.  For the UI, NTP and home page are totally different, so it wouldn't be
an
> extension so much as a new section to the options UI, which would be the same
> for any of the pages we allow to be overridden.

Per offline discussion, you are right. I did not realize that the existing UI
does not affect what happens when you use ctrl+t or press the new tab button.

- a