Add chrome://popular-sites-internals/ to iOS.

Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to a ~3K blowup on Android (it was probably ~2K
compressed and ~5K now). There's probably some extra savings because I
think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7713f244a63217c94cef663b5dbd153d00e7cdb6
Cr-Commit-Position: refs/heads/master@{#429310}

[sfiera, 2016-10-28 11:31:32]

Description was changed from

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The resources have been added to the main components, which means
    they'll be bundled on Desktop too, but unused.  I couldn't find a
    way to have grit include resources only on specific platforms.
  * The handler is not enabled on iOS yet; that's downstream.

BUG=659955
==========

to

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The resources have been added to the main components, which means
    they'll be bundled on Desktop too, but unused.  I couldn't find a
    way to have grit include resources only on specific platforms.
  * The handler is not enabled on iOS yet; that's downstream.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[sfiera, 2016-10-28 11:36:39]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 11:36:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 11:44:53]

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

[commit-bot: I haz the power, 2016-10-28 11:44:53]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[sfiera, 2016-10-28 11:48:23]

Description was changed from

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The resources have been added to the main components, which means
    they'll be bundled on Desktop too, but unused.  I couldn't find a
    way to have grit include resources only on specific platforms.
  * The handler is not enabled on iOS yet; that's downstream.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The resources have been added to the main components, which means
    they'll be bundled on Desktop too, but unused.  I couldn't find a
    way to have grit include resources only on specific platforms.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to ~5K of unused resources on Desktop, and a ~3K blowup on
Android (it was probably ~2K compressed). Minus some extra savings
because I think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[sfiera, 2016-10-28 11:56:38]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 11:56:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-10-28 13:00:43]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org, sdefresne@chromium.org

[sfiera, 2016-10-28 13:00:43]

bauerb: you're up for //chrome/browser/… and //components/ntp_tiles/…. I expect
you can't realistically review this before you're OOO, so if you don't get the
chance, don't worry about it and I'll redirect on Monday.
sdefresne: //components/resources/… and //ios/chrome/browser/…. If you can't
review this today, I'd appreciate a suggestion for an alternate ios reviewer,
since I know France is off next Mon+Tue.

[commit-bot: I haz the power, 2016-10-28 13:10:13]

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

[commit-bot: I haz the power, 2016-10-28 13:10:14]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-28 13:23:07]

https://codereview.chromium.org/2457033003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/popular_sites_internals_message_handler.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/popular_sites_internals_message_handler.cc:18: using
ntp_tiles::PopularSitesInternalsMessageHandlerImpl;
Do you need this?

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.cc:144:
std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue);
Use base::MakeUnique<>?

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.h (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:28: class
PopularSitesInternalsMessageHandlerInterface {
This is a bit weird:
1) Two classes defined in the same file, none of which match the filename
2) The ...Impl class does not actually implement the ...Interface class (and
...Interface is neither required in Chrome, nor common, but maybe I'm just
misunderstanding how this is supposed to work). Can you shed some light onto
that?

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/chro...
File ios/chrome/browser/chrome_url_constants.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/chro...
ios/chrome/browser/chrome_url_constants.cc:58: kChromeUIPhysicalWebHost,
kChromeUIPopularSitesInternalsHost,
Wait, this list actually is in two columns?
http://www.reactiongifs.com/r/but-why.gif

[sdefresne, 2016-10-28 13:34:03]

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.cc:142:
std::unique_ptr<base::ListValue> sites_list(new base::ListValue);
Use base::MakeUnique<>

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.h (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:12: #include
"base/callback.h"
#include "base/callback_forward.h" should be enough for the header (you'll need
to include "base/callback.h" in implementation).

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:28: class
PopularSitesInternalsMessageHandlerInterface {
Is this an interface implemented by embedder (desktop, ios, ...) and injected in
the component? If so, the convention for other components  is to call them
FooClient.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:29: public:
The class needs a virtual destructor.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:30: virtual
base::SequencedWorkerPool* GetBlockingPool() = 0;
Can you document this class and its methods?

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:43: }
I would recommend making the class non-copyable since it has virtual methods.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:46: class
PopularSitesInternalsMessageHandlerImpl {
Can you document this class and its methods?

https://codereview.chromium.org/2457033003/diff/40001/components/resources/nt...
File components/resources/ntp_tiles_resources.grdp (right):

https://codereview.chromium.org/2457033003/diff/40001/components/resources/nt...
components/resources/ntp_tiles_resources.grdp:3: <include
name="IDR_POPULAR_SITES_INTERNALS_HTML"
file="../ntp_tiles/webui/resources/popular_sites_internals.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
Can't you use <if expr="blah"> here?

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/chro...
File ios/chrome/browser/chrome_url_constants.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/chro...
ios/chrome/browser/chrome_url_constants.cc:58: kChromeUIPhysicalWebHost,
kChromeUIPopularSitesInternalsHost,
On 2016/10/28 13:23:07, Bernhard Bauer wrote:
> Wait, this list actually is in two columns?
> http://www.reactiongifs.com/r/but-why.gif

This is how clang-format format the list and //ios requires code to be formatted
with clang-format.

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/ui/w...
File ios/chrome/browser/ui/webui/popular_sites_internals_ui.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/ui/w...
ios/chrome/browser/ui/webui/popular_sites_internals_ui.cc:52: : handler_(new
PopularSitesInternalsMessageHandlerImpl(this)) {}
Use base::MakeUnique<>

[sfiera, 2016-10-28 13:38:14]

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.h (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:28: class
PopularSitesInternalsMessageHandlerInterface {
On 2016/10/28 13:23:07, Bernhard Bauer wrote:
> This is a bit weird:
> 1) Two classes defined in the same file, none of which match the filename
> 2) The ...Impl class does not actually implement the ...Interface class (and
> ...Interface is neither required in Chrome, nor common, but maybe I'm just
> misunderstanding how this is supposed to work). Can you shed some light onto
> that?

Basically, the problem is that the WebUI handlers for content and for ios have
the same methods (or almost the same), but don't share an interface.

So there are two "PopularSitesInternalsMessageHandler" classes, one for content
(under //chrome), and one for ios (under //ios/chrome). They both implement
…Interface, which is the message handler methods, plus a bit more factory stuff
for popular sites specifically. Then they both use …Impl for their
implementation, except for the stuff that's actually different on the platforms.

This file could be popular_sites_internals_message_handler_impl.h.

https://codereview.chromium.org/2457033003/diff/40001/components/resources/nt...
File components/resources/ntp_tiles_resources.grdp (right):

https://codereview.chromium.org/2457033003/diff/40001/components/resources/nt...
components/resources/ntp_tiles_resources.grdp:3: <include
name="IDR_POPULAR_SITES_INTERNALS_HTML"
file="../ntp_tiles/webui/resources/popular_sites_internals.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
On 2016/10/28 13:34:03, sdefresne wrote:
> Can't you use <if expr="blah"> here?

I'll give it a try.

[sfiera, 2016-10-28 14:12:43]

https://codereview.chromium.org/2457033003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/popular_sites_internals_message_handler.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/popular_sites_internals_message_handler.cc:18: using
ntp_tiles::PopularSitesInternalsMessageHandlerImpl;
On 2016/10/28 13:23:07, Bernhard Bauer wrote:
> Do you need this?

Removed.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.cc:142:
std::unique_ptr<base::ListValue> sites_list(new base::ListValue);
On 2016/10/28 13:34:02, sdefresne wrote:
> Use base::MakeUnique<>

Done. (this is moved code, but might as well improve it too)

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.cc:144:
std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue);
On 2016/10/28 13:23:07, Bernhard Bauer wrote:
> Use base::MakeUnique<>?

Done.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.h (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:12: #include
"base/callback.h"
On 2016/10/28 13:34:02, sdefresne wrote:
> #include "base/callback_forward.h" should be enough for the header (you'll
need
> to include "base/callback.h" in implementation).

Done.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:28: class
PopularSitesInternalsMessageHandlerInterface {
On 2016/10/28 13:38:14, sfiera wrote:
> On 2016/10/28 13:23:07, Bernhard Bauer wrote:
> > This is a bit weird:
> > 1) Two classes defined in the same file, none of which match the filename
> > 2) The ...Impl class does not actually implement the ...Interface class (and
> > ...Interface is neither required in Chrome, nor common, but maybe I'm just
> > misunderstanding how this is supposed to work). Can you shed some light onto
> > that?
> 
> Basically, the problem is that the WebUI handlers for content and for ios have
> the same methods (or almost the same), but don't share an interface.
> 
> So there are two "PopularSitesInternalsMessageHandler" classes, one for
content
> (under //chrome), and one for ios (under //ios/chrome). They both implement
> …Interface, which is the message handler methods, plus a bit more factory
stuff
> for popular sites specifically. Then they both use …Impl for their
> implementation, except for the stuff that's actually different on the
platforms.
> 
> This file could be popular_sites_internals_message_handler_impl.h.

Renamed to Client.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:29: public:
On 2016/10/28 13:34:02, sdefresne wrote:
> The class needs a virtual destructor.

Done.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:30: virtual
base::SequencedWorkerPool* GetBlockingPool() = 0;
On 2016/10/28 13:34:02, sdefresne wrote:
> Can you document this class and its methods?

Done.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:43: }
On 2016/10/28 13:34:03, sdefresne wrote:
> I would recommend making the class non-copyable since it has virtual methods.

Done.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:46: class
PopularSitesInternalsMessageHandlerImpl {
On 2016/10/28 13:34:02, sdefresne wrote:
> Can you document this class and its methods?

Done. (the public ones—most of them actually no longer needed to be public)

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/ui/w...
File ios/chrome/browser/ui/webui/popular_sites_internals_ui.cc (right):

https://codereview.chromium.org/2457033003/diff/40001/ios/chrome/browser/ui/w...
ios/chrome/browser/ui/webui/popular_sites_internals_ui.cc:52: : handler_(new
PopularSitesInternalsMessageHandlerImpl(this)) {}
On 2016/10/28 13:34:03, sdefresne wrote:
> Use base::MakeUnique<>

Done.

[sfiera, 2016-10-28 14:16:32]

Oh, and also, thanks for the quick response late on a Friday!

[sfiera, 2016-10-28 14:17:18]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 14:17:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-10-28 14:18:36]

Description was changed from

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The resources have been added to the main components, which means
    they'll be bundled on Desktop too, but unused.  I couldn't find a
    way to have grit include resources only on specific platforms.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to ~5K of unused resources on Desktop, and a ~3K blowup on
Android (it was probably ~2K compressed). Minus some extra savings
because I think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to a ~3K blowup on Android (it was probably ~2K
compressed and ~5K now). There's probably some extra savings because I
think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-28 15:28:50]

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

[commit-bot: I haz the power, 2016-10-28 15:28:50]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-10-28 16:56:38]

Thanks! Yes, ...Client makes the pattern much clearer.

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
File components/ntp_tiles/webui/popular_sites_handler.h (right):

https://codereview.chromium.org/2457033003/diff/40001/components/ntp_tiles/we...
components/ntp_tiles/webui/popular_sites_handler.h:43: }
On 2016/10/28 13:34:03, sdefresne wrote:
> I would recommend making the class non-copyable since it has virtual methods.

OOC, why are virtual methods the criterium here? What I've always heard was that
a non-trivial destructor means that copy and assignment constructors should
either be explicitly written, or explicitly disallowed
(https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming) ).

https://codereview.chromium.org/2457033003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/popular_sites_internals_message_handler.h (right):

https://codereview.chromium.org/2457033003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/popular_sites_internals_message_handler.h:16: class
PopularSitesInternalsMessageHandler
Nit: At the risk of embiggening the class name even more, a common pattern is to
call the chrome/ embedder implementation of a client Chrome...Client.

https://codereview.chromium.org/2457033003/diff/100001/components/ntp_tiles/w...
File components/ntp_tiles/webui/popular_sites_internals_message_handler_impl.h
(right):

https://codereview.chromium.org/2457033003/diff/100001/components/ntp_tiles/w...
components/ntp_tiles/webui/popular_sites_internals_message_handler_impl.h:28:
class PopularSitesInternalsMessageHandlerClient {
Move this to a separate file?

[sfiera, 2016-10-31 15:09:45]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 15:10:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-10-31 15:10:53]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 15:11:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-10-31 15:15:50]

sfiera@chromium.org changed reviewers:
+ jochen@chromium.org

[sfiera, 2016-10-31 15:15:51]

Reviewer changes
+jochen to replace bauerb@. Jochen, I see you're OOO until Wednesday, which is a
lot sooner than Bernhard will be back :)
sdefresne@, I assume you're back on Wednesday too, so I've left you on.

I've tried to minimize the diff between
{,ios/}chrome/browser/ui/webui/popular_sites_internals_ui.cc, and it's now
mostly [-Chrome-]{+IOS+} and [-content-]{+web+}.

[commit-bot: I haz the power, 2016-10-31 16:04:46]

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

[commit-bot: I haz the power, 2016-10-31 16:04:47]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-11-02 10:53:34]

lgtm with nits

https://codereview.chromium.org/2457033003/diff/140001/components/ntp_tiles/w...
File components/ntp_tiles/webui/popular_sites_internals_message_handler.h
(right):

https://codereview.chromium.org/2457033003/diff/140001/components/ntp_tiles/w...
components/ntp_tiles/webui/popular_sites_internals_message_handler.h:36:
PopularSitesInternalsMessageHandler(
explicit

https://codereview.chromium.org/2457033003/diff/140001/components/ntp_tiles/w...
components/ntp_tiles/webui/popular_sites_internals_message_handler.h:69: class
PopularSitesInternalsMessageHandlerClient {
can you move this into a separate header please?

[sdefresne, 2016-11-02 12:58:23]

lgtm

I would also second jochen comment about moving
PopularSitesInternalsMessageHandlerClient to a separate file.

[sfiera, 2016-11-02 13:30:06]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[sfiera, 2016-11-02 13:30:19]

https://codereview.chromium.org/2457033003/diff/140001/components/ntp_tiles/w...
File components/ntp_tiles/webui/popular_sites_internals_message_handler.h
(right):

https://codereview.chromium.org/2457033003/diff/140001/components/ntp_tiles/w...
components/ntp_tiles/webui/popular_sites_internals_message_handler.h:36:
PopularSitesInternalsMessageHandler(
On 2016/11/02 10:53:34, jochen (ooo Nov 1) wrote:
> explicit

Oops. Done, thanks.

https://codereview.chromium.org/2457033003/diff/140001/components/ntp_tiles/w...
components/ntp_tiles/webui/popular_sites_internals_message_handler.h:69: class
PopularSitesInternalsMessageHandlerClient {
On 2016/11/02 10:53:34, jochen wrote:
> can you move this into a separate header please?

Done.

[commit-bot: I haz the power, 2016-11-02 13:30:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-11-02 13:32:02]

sfiera@chromium.org changed reviewers:
+ pkasting@chromium.org

[sfiera, 2016-11-02 13:32:02]

+pkasting, your approval is needed as an owner of //components/url_formatter.
The code that depends on url_formatter is just moving, not new, though an edge
to it from ios is being added. Thanks!

[sfiera, 2016-11-02 13:33:28]

On 2016/11/02 13:32:02, sfiera wrote:
> +pkasting, your approval is needed as an owner of //components/url_formatter.
> The code that depends on url_formatter is just moving, not new, though an edge
> to it from ios is being added. Thanks!

…because of an addition to //components/ntp_tiles/DEPS

[commit-bot: I haz the power, 2016-11-02 14:43:43]

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

[commit-bot: I haz the power, 2016-11-02 14:43:45]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-11-02 17:02:23]

LGTM

[sfiera, 2016-11-02 17:03:55]

Thanks all! Submitting now.

[sfiera, 2016-11-02 17:04:06]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-11-02 17:04:06]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2457033003/#ps160001
(title: "Break out ...Client.")

[commit-bot: I haz the power, 2016-11-02 17:04:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 17:10:08]

Description was changed from

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to a ~3K blowup on Android (it was probably ~2K
compressed and ~5K now). There's probably some extra savings because I
think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to a ~3K blowup on Android (it was probably ~2K
compressed and ~5K now). There's probably some extra savings because I
think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-02 17:10:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-02 17:21:35]

Description was changed from

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to a ~3K blowup on Android (it was probably ~2K
compressed and ~5K now). There's probably some extra savings because I
think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add chrome://popular-sites-internals/ to iOS.

Move the resources and the bulk of the implementation into
//components/ntp_tiles. Leave behind the glue code for //content and add
the corresponding glue for iOS.

Some notes:
  * I couldn't figure out how to use gzipped resources for iOS, so I had
    to disable gzip on the resources.
  * The HTML is now flattened; this handles the <if/> tags. Apparently
    they were passed through to the browser before, which renders them
    ineffective.
  * The handler is not enabled on iOS yet; that's downstream.

This amounts to a ~3K blowup on Android (it was probably ~2K
compressed and ~5K now). There's probably some extra savings because I
think grit minifies the JS.

BUG=659955
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7713f244a63217c94cef663b5dbd153d00e7cdb6
Cr-Commit-Position: refs/heads/master@{#429310}
==========

[commit-bot: I haz the power, 2016-11-02 17:21:36]

Patchset 9 (id:??) landed as
https://crrev.com/7713f244a63217c94cef663b5dbd153d00e7cdb6
Cr-Commit-Position: refs/heads/master@{#429310}