first cut at uber page

first cut at uber page

the subpages are iframes. On the C++ side, the uber WebUI keeps a collection of subpage WebUIs and proxies function calls to them. Calls into JS are directed to the correct iframe by setting a frame name.

The fact it's in an iframe is almost completely transparent to the options page (both from the C++ and JS side); you'll notice there are no changes to any options files.*

*exception: temporary command line changes

BUG=100885
TEST=chrome://uber should have an iframe with chrome://settings in it (fully functional)

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

[Evan Stade, 2011-12-09 04:11:32]

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/personal_options.js (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/personal_options.js:121: if (cr.commandLine &&
cr.commandLine.options['--bwsi']) {
I'm not sure what to do about commandLine yet, but for now this keeps it from
causing errors. Our use of commandLine depends on CppBoundClass, but I'm not
sure why. I think it would be much easier to pass it in via templateData.

[arv (Not doing code reviews), 2011-12-09 06:47:43]

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
File chrome/browser/ui/webui/uber/uber_ui.cc (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.cc:35:
options->set_frame_xpath("//iframe[@id='settings']");
Why xpath and not CSS selector?

http://codereview.chromium.org/8889041/diff/2001/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/common/url_constants....
chrome/common/url_constants.cc:183: const char kChromeUIUberHost[] = "uber";
Are we serious about calling this uber???

[Evan Stade, 2011-12-09 20:19:38]

On 2011/12/09 06:47:43, arv wrote:
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
> File chrome/browser/ui/webui/uber/uber_ui.cc (right):
> 
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
> chrome/browser/ui/webui/uber/uber_ui.cc:35:
> options->set_frame_xpath("//iframe[@id='settings']");
> Why xpath and not CSS selector?

AFAICT RenderViewHost only accepts xpath.

> 
>
http://codereview.chromium.org/8889041/diff/2001/chrome/common/url_constants.cc
> File chrome/common/url_constants.cc (right):
> 
>
http://codereview.chromium.org/8889041/diff/2001/chrome/common/url_constants....
> chrome/common/url_constants.cc:183: const char kChromeUIUberHost[] = "uber";
> Are we serious about calling this uber???

I don't think this URL will really be uber; eventually this string will
doubtless change. Do you have a problem with having the internal name be uber?

[James Hawkins, 2011-12-10 03:03:13]

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.html (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:9: <script
src="chrome://uber/uber.js"></script>
I'm not sure we should have a landing page for the uber page, but if we do it's
URL is chrome://chrome according to crbug.com/100885.  We should structure the
code to match, so s/uber/chrome/.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.js (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.js:6: console.log('hello world');
Remove console spam before committing.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
File chrome/browser/ui/webui/uber/uber_ui.cc (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.cc:40: STLDeleteElements(&sub_uis_);
Use a ScopedVector.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.cc:77: /*
Remove?

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
File chrome/browser/ui/webui/uber/uber_ui.h (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:12: #include "base/string16.h"
Unused include.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:13: #include
"chrome/browser/history/history.h"
Unused include.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:14: #include
"chrome/browser/ui/webui/chrome_url_data_manager.h"
Unused include.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:16: #include
"content/browser/cancelable_request.h"
Unused include.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:17: #include
"content/public/browser/notification_registrar.h"
Unused include.

http://codereview.chromium.org/8889041/diff/2001/chrome/common/url_constants.cc
File chrome/common/url_constants.cc (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/common/url_constants....
chrome/common/url_constants.cc:183: const char kChromeUIUberHost[] = "uber";
On 2011/12/09 06:47:44, arv wrote:
> Are we serious about calling this uber???

The URL is chrome://chrome, so I have a review comment higher up to
s/uber/chrome/.  The code name is UberPage though.

[Evan Stade, 2011-12-10 03:32:31]

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.html (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:9: <script
src="chrome://uber/uber.js"></script>
On 2011/12/10 03:03:13, James Hawkins wrote:
> I'm not sure we should have a landing page for the uber page, but if we do
it's
> URL is chrome://chrome according to crbug.com/100885.  We should structure the
> code to match, so s/uber/chrome/.

I'm not sure what you mean that we shouldn't have a landing page. We need a page
that embeds subpages.

I really don't like the idea of replacing every instance of "uber" in this cl
with "chrome". For example, that would create a class with the mystifying name
ChromeUI. The name in the code doesn't have to match the URL. I have no problem
changing from "uber" to something that is equally unconfusing.

[James Hawkins, 2011-12-10 04:12:32]

On 2011/12/10 03:32:31, Evan Stade wrote:
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
> File chrome/browser/resources/uber/uber.html (right):
> 
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
> chrome/browser/resources/uber/uber.html:9: <script
> src="chrome://uber/uber.js"></script>
> On 2011/12/10 03:03:13, James Hawkins wrote:
> > I'm not sure we should have a landing page for the uber page, but if we do
> it's
> > URL is chrome://chrome according to crbug.com/100885.  We should structure
the
> > code to match, so s/uber/chrome/.
> 
> I'm not sure what you mean that we shouldn't have a landing page. We need a
page
> that embeds subpages.
> 
> I really don't like the idea of replacing every instance of "uber" in this cl
> with "chrome". For example, that would create a class with the mystifying name
> ChromeUI. The name in the code doesn't have to match the URL. I have no
problem
> changing from "uber" to something that is equally unconfusing.

Re: the landing page.  I'm not questioning the existence of the landing page,
only having a URL that takes the user to the landing page (other than the URLs
of the contained pages), i.e., chrome://downloads takes you to the uber page
with the downloads 'tab' activated.  Exposing more UI (the chrome://chrome URL)
to get to this page seems superfluous.  What are the use cases?

[Dan Beam, 2011-12-10 06:24:12]

> I have no problem changing from "uber" to something that is equally
unconfusing.

drive-by +1

[Evan Stade, 2011-12-12 19:32:32]

On 2011/12/10 04:12:32, James Hawkins wrote:
> On 2011/12/10 03:32:31, Evan Stade wrote:
> >
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
> > File chrome/browser/resources/uber/uber.html (right):
> > 
> >
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
> > chrome/browser/resources/uber/uber.html:9: <script
> > src="chrome://uber/uber.js"></script>
> > On 2011/12/10 03:03:13, James Hawkins wrote:
> > > I'm not sure we should have a landing page for the uber page, but if we do
> > it's
> > > URL is chrome://chrome according to crbug.com/100885.  We should structure
> the
> > > code to match, so s/uber/chrome/.
> > 
> > I'm not sure what you mean that we shouldn't have a landing page. We need a
> page
> > that embeds subpages.
> > 
> > I really don't like the idea of replacing every instance of "uber" in this
cl
> > with "chrome". For example, that would create a class with the mystifying
name
> > ChromeUI. The name in the code doesn't have to match the URL. I have no
> problem
> > changing from "uber" to something that is equally unconfusing.
> 
> Re: the landing page.  I'm not questioning the existence of the landing page,
> only having a URL that takes the user to the landing page (other than the URLs
> of the contained pages), i.e., chrome://downloads takes you to the uber page
> with the downloads 'tab' activated.  Exposing more UI (the chrome://chrome
URL)
> to get to this page seems superfluous.  What are the use cases?

on the technical side, I'm not sure what you want is possible. If we change the
host name that forces a navigation/reload. At some point (v1.0) I think it would
be possible/desirable for chrome://downloads to redirect to
chrome://chrome/downloads, and chrome://chrome to redirect to
chrome://chrome/[default page]. This is akin to settings; currently
chrome://settings redirects to chrome://settings/browser.

However most users won't be typing this in directly or even reading the URL so
they wouldn't notice one way or the other.

[James Hawkins, 2011-12-12 20:32:40]

On 2011/12/12 19:32:32, Evan Stade wrote:
> On 2011/12/10 04:12:32, James Hawkins wrote:
> > On 2011/12/10 03:32:31, Evan Stade wrote:
> > >
> >
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
> > > File chrome/browser/resources/uber/uber.html (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
> > > chrome/browser/resources/uber/uber.html:9: <script
> > > src="chrome://uber/uber.js"></script>
> > > On 2011/12/10 03:03:13, James Hawkins wrote:
> > > > I'm not sure we should have a landing page for the uber page, but if we
do
> > > it's
> > > > URL is chrome://chrome according to crbug.com/100885.  We should
structure
> > the
> > > > code to match, so s/uber/chrome/.
> > > 
> > > I'm not sure what you mean that we shouldn't have a landing page. We need
a
> > page
> > > that embeds subpages.
> > > 
> > > I really don't like the idea of replacing every instance of "uber" in this
> cl
> > > with "chrome". For example, that would create a class with the mystifying
> name
> > > ChromeUI. The name in the code doesn't have to match the URL. I have no
> > problem
> > > changing from "uber" to something that is equally unconfusing.
> > 
> > Re: the landing page.  I'm not questioning the existence of the landing
page,
> > only having a URL that takes the user to the landing page (other than the
URLs
> > of the contained pages), i.e., chrome://downloads takes you to the uber page
> > with the downloads 'tab' activated.  Exposing more UI (the chrome://chrome
> URL)
> > to get to this page seems superfluous.  What are the use cases?
> 
> on the technical side, I'm not sure what you want is possible. If we change
the
> host name that forces a navigation/reload. At some point (v1.0) I think it
would
> be possible/desirable for chrome://downloads to redirect to
> chrome://chrome/downloads, and chrome://chrome to redirect to
> chrome://chrome/[default page]. This is akin to settings; currently
> chrome://settings redirects to chrome://settings/browser.
> 
> However most users won't be typing this in directly or even reading the URL so
> they wouldn't notice one way or the other.

I see what you mean.  OK, we'll see how far we can push this technically.

[Evan Stade, 2011-12-13 00:15:27]

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.js (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.js:6: console.log('hello world');
On 2011/12/10 03:03:13, James Hawkins wrote:
> Remove console spam before committing.

Done.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
File chrome/browser/ui/webui/uber/uber_ui.cc (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.cc:40: STLDeleteElements(&sub_uis_);
On 2011/12/10 03:03:13, James Hawkins wrote:
> Use a ScopedVector.

Done.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.cc:77: /*
On 2011/12/10 03:03:13, James Hawkins wrote:
> Remove?

Done.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
File chrome/browser/ui/webui/uber/uber_ui.h (right):

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:12: #include "base/string16.h"
On 2011/12/10 03:03:13, James Hawkins wrote:
> Unused include.

Done.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:13: #include
"chrome/browser/history/history.h"
On 2011/12/10 03:03:13, James Hawkins wrote:
> Unused include.

Done.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:14: #include
"chrome/browser/ui/webui/chrome_url_data_manager.h"
On 2011/12/10 03:03:13, James Hawkins wrote:
> Unused include.

Done.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:16: #include
"content/browser/cancelable_request.h"
On 2011/12/10 03:03:13, James Hawkins wrote:
> Unused include.

Done.

http://codereview.chromium.org/8889041/diff/2001/chrome/browser/ui/webui/uber...
chrome/browser/ui/webui/uber/uber_ui.h:17: #include
"content/public/browser/notification_registrar.h"
On 2011/12/10 03:03:13, James Hawkins wrote:
> Unused include.

Done.

[James Hawkins, 2011-12-13 00:22:14]

LGTM with two nits and a question.

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/resources/ub...
File chrome/browser/resources/uber/uber.css (right):

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/resources/ub...
chrome/browser/resources/uber/uber.css:6: border: 1px solid red;
Is the red for debugging purposes?

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/ui/webui/ube...
File chrome/browser/ui/webui/uber/uber_ui.h (right):

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/ui/webui/ube...
chrome/browser/ui/webui/uber/uber_ui.h:15: class UberUI : public ChromeWebUI {
Document the class.

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/ui/webui/ube...
chrome/browser/ui/webui/uber/uber_ui.h:20: virtual void OnWebUISend(const GURL&
source_url,
// ChromeWebUI implementation.

[Evan Stade, 2011-12-13 01:00:40]

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/resources/ub...
File chrome/browser/resources/uber/uber.css (right):

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/resources/ub...
chrome/browser/resources/uber/uber.css:6: border: 1px solid red;
On 2011/12/13 00:22:15, James Hawkins wrote:
> Is the red for debugging purposes?

yea

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/ui/webui/ube...
File chrome/browser/ui/webui/uber/uber_ui.h (right):

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/ui/webui/ube...
chrome/browser/ui/webui/uber/uber_ui.h:15: class UberUI : public ChromeWebUI {
On 2011/12/13 00:22:15, James Hawkins wrote:
> Document the class.

Done.

http://codereview.chromium.org/8889041/diff/11001/chrome/browser/ui/webui/ube...
chrome/browser/ui/webui/uber/uber_ui.h:20: virtual void OnWebUISend(const GURL&
source_url,
On 2011/12/13 00:22:15, James Hawkins wrote:
> // ChromeWebUI implementation.

Done.

[commit-bot: I haz the power, 2011-12-13 01:01:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/8889041/15001

[commit-bot: I haz the power, 2011-12-13 01:01:18]

Presubmit check for 8889041-15001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/renderer/web_ui_bindings.cc

Presubmit checks took 2.8s to calculate.

[Evan Stade, 2011-12-13 01:08:30]

+sky for content/ review

[sky, 2011-12-13 01:20:28]

LGTM

[commit-bot: I haz the power, 2011-12-13 01:24:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/8889041/9003

[commit-bot: I haz the power, 2011-12-13 01:24:24]

Presubmit check for 8889041-9003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/renderer/web_ui_bindings.cc

Presubmit checks took 2.6s to calculate.