Add initial version of chrome://popular-sites-internals page

Add initial version of chrome://popular-sites-internals page

BUG=530490
Committed: https://crrev.com/d73c9ad988575309c5e627df59dbe7a2af768049
Cr-Commit-Position: refs/heads/master@{#350168}

[Marc Treib, 2015-09-11 08:58:16]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-09-11 08:58:17]

PTAL - screenshot on the bug!

[Bernhard Bauer, 2015-09-11 16:00:06]

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
File chrome/browser/android/popular_sites.cc (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
chrome/browser/android/popular_sites.cc:164: static bool overwrite = true;
Given that this does now not directly control anymore whether the file will be
overwritten, would it make sense to rename it to something like |first_time|?

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
File chrome/browser/android/popular_sites.h (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
chrome/browser/android/popular_sites.h:59: bool force_download,
Add a comment?

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.css (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.css:6: font-size: larger;
Do we really use "larger" as a value?

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:27: <form id="download">
I don't think you even need the <form> if you never submit it.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:45: <div id='sites'
class="section">
Use double quotes.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:56: <script
src="chrome://resources/js/jstemplate_compiled.js"></script>
I think you could move these scripts to the head, as they will only run once the
DOM has loaded anyway.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/ui/webui/pop...
File chrome/browser/ui/webui/popular_sites_internals_ui.cc (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/ui/webui/pop...
chrome/browser/ui/webui/popular_sites_internals_ui.cc:32:
web_ui->AddMessageHandler(new PopularSitesInternalsMessageHandler);
If PopularSitesInternalsMessageHandler is not used anywhere else, consider
moving it to an anonymous namespace in this file.

[Marc Treib, 2015-09-22 11:13:56]

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
File chrome/browser/android/popular_sites.cc (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
chrome/browser/android/popular_sites.cc:164: static bool overwrite = true;
On 2015/09/11 16:00:05, Bernhard Bauer wrote:
> Given that this does now not directly control anymore whether the file will be
> overwritten, would it make sense to rename it to something like |first_time|?

Done.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
File chrome/browser/android/popular_sites.h (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/android/popu...
chrome/browser/android/popular_sites.h:59: bool force_download,
On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> Add a comment?

Done.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.css (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.css:6: font-size: larger;
On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> Do we really use "larger" as a value?

Well, it's a valid css value. I don't know of any guidelines about using it in
Chrome, but since this an internal debugging page, I think it's fine.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:27: <form id="download">
On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> I don't think you even need the <form> if you never submit it.

But I do submit it?!

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:45: <div id='sites'
class="section">
On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> Use double quotes.

Done.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:56: <script
src="chrome://resources/js/jstemplate_compiled.js"></script>
On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> I think you could move these scripts to the head, as they will only run once
the
> DOM has loaded anyway.

Done.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/ui/webui/pop...
File chrome/browser/ui/webui/popular_sites_internals_ui.cc (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/ui/webui/pop...
chrome/browser/ui/webui/popular_sites_internals_ui.cc:32:
web_ui->AddMessageHandler(new PopularSitesInternalsMessageHandler);
On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> If PopularSitesInternalsMessageHandler is not used anywhere else, consider
> moving it to an anonymous namespace in this file.

All the other "internals" sites seem to have it in a separate file, and I'd like
to stay consistent.

[Bernhard Bauer, 2015-09-22 13:49:21]

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.css (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.css:6: font-size: larger;
On 2015/09/22 11:13:56, Marc Treib wrote:
> On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> > Do we really use "larger" as a value?
> 
> Well, it's a valid css value. I don't know of any guidelines about using it in
> Chrome, but since this an internal debugging page, I think it's fine.

Just because it's an internal debugging page doesn't mean that guidelines don't
apply. Basically, "larger" only makes sense together with a previous font-size
declaration, and then you could just set the absolute size (maybe as one of
"small", "large", etc. if that's how the size is defined at the top level).

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:27: <form id="download">
On 2015/09/22 11:13:56, Marc Treib wrote:
> On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> > I don't think you even need the <form> if you never submit it.
> 
> But I do submit it?!

Huh. I guess I'm so used to this being done without forms (i.e. by registering a
click event on the button instead of submit)... ☺

[Marc Treib, 2015-09-22 14:02:23]

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.css (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.css:6: font-size: larger;
On 2015/09/22 13:49:21, Bernhard Bauer wrote:
> On 2015/09/22 11:13:56, Marc Treib wrote:
> > On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> > > Do we really use "larger" as a value?
> > 
> > Well, it's a valid css value. I don't know of any guidelines about using it
in
> > Chrome, but since this an internal debugging page, I think it's fine.
> 
> Just because it's an internal debugging page doesn't mean that guidelines
don't
> apply. Basically, "larger" only makes sense together with a previous font-size
> declaration, and then you could just set the absolute size (maybe as one of
> "small", "large", etc. if that's how the size is defined at the top level).

As I said, I don't know of any guidelines about this. The previous font-size
comes from text_defaults.css here.
But whatever - I've put in an absolute size instead.

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:27: <form id="download">
On 2015/09/22 13:49:21, Bernhard Bauer wrote:
> On 2015/09/22 11:13:56, Marc Treib wrote:
> > On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> > > I don't think you even need the <form> if you never submit it.
> > 
> > But I do submit it?!
> 
> Huh. I guess I'm so used to this being done without forms (i.e. by registering
a
> click event on the button instead of submit)... ☺

Hm yeah, I guess that would be a bit simpler. I just copied the form approach
from somewhere.
Any particular reason to do it one or the other way?

[Bernhard Bauer, 2015-09-22 14:10:23]

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:27: <form id="download">
On 2015/09/22 14:02:22, Marc Treib wrote:
> On 2015/09/22 13:49:21, Bernhard Bauer wrote:
> > On 2015/09/22 11:13:56, Marc Treib wrote:
> > > On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> > > > I don't think you even need the <form> if you never submit it.
> > > 
> > > But I do submit it?!
> > 
> > Huh. I guess I'm so used to this being done without forms (i.e. by
registering
> a
> > click event on the button instead of submit)... ☺
> 
> Hm yeah, I guess that would be a bit simpler. I just copied the form approach
> from somewhere.
> Any particular reason to do it one or the other way?

Hm... a form gives you:
* A convenient way to get all the values from its input fields as key-value
pairs
* The option to send these values to a given URL
We don't need either of these, so I think all other things being equal, removing
the <form> saves two lines of HTML ☺

[Marc Treib, 2015-09-22 14:20:34]

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
File chrome/browser/resources/popular_sites_internals.html (right):

https://codereview.chromium.org/1340493002/diff/1/chrome/browser/resources/po...
chrome/browser/resources/popular_sites_internals.html:27: <form id="download">
On 2015/09/22 14:10:23, Bernhard Bauer wrote:
> On 2015/09/22 14:02:22, Marc Treib wrote:
> > On 2015/09/22 13:49:21, Bernhard Bauer wrote:
> > > On 2015/09/22 11:13:56, Marc Treib wrote:
> > > > On 2015/09/11 16:00:06, Bernhard Bauer wrote:
> > > > > I don't think you even need the <form> if you never submit it.
> > > > 
> > > > But I do submit it?!
> > > 
> > > Huh. I guess I'm so used to this being done without forms (i.e. by
> registering
> > a
> > > click event on the button instead of submit)... ☺
> > 
> > Hm yeah, I guess that would be a bit simpler. I just copied the form
approach
> > from somewhere.
> > Any particular reason to do it one or the other way?
> 
> Hm... a form gives you:
> * A convenient way to get all the values from its input fields as key-value
> pairs
> * The option to send these values to a given URL
> We don't need either of these, so I think all other things being equal,
removing
> the <form> saves two lines of HTML ☺

Done.

[Bernhard Bauer, 2015-09-22 14:24:21]

lgtm

[Marc Treib, 2015-09-22 14:32:35]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-09-22 14:32:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340493002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340493002/60001

[commit-bot: I haz the power, 2015-09-22 15:24:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 15:24:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2015-09-22 15:29:48]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-09-22 15:31:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340493002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340493002/60001

[commit-bot: I haz the power, 2015-09-22 16:20:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-22 16:22:10]

Patchset 4 (id:??) landed as
https://crrev.com/d73c9ad988575309c5e627df59dbe7a2af768049
Cr-Commit-Position: refs/heads/master@{#350168}