Initial WebUI for DOM Distiller.

Initial WebUI for DOM Distiller.

The DOM Distiller component will contain code for an experimental
prototype for distilling the core part of a web page.

To enable this feature, use the command line flag --enable-distiller.

The webui/ folder depends on content, but given iOS at the time being
supports the usage of WebUI, it is kept a a top level folder in the
component instead of in content/.

This commit is relanding r223528 since it was reverted in r223540.
- Added missing GYP dependency.

TBR=sky@chromium.org
BUG=288015
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223528

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223759

[nyquist, 2013-09-09 03:41:25]

PTAL.
joi: components/, tools/gritsettings/
jhawkins: chrome/browser/ui/webui/
stuartmorgan: components/ and iOS related issues such as resources.

From what I can tell, this is the first time we try to put the webui of a
component in the component itself. Please advice on this.

[Jói, 2013-09-09 08:29:58]

Mostly looks good, I have a few comments below.

I looked at the whole change, but note that I'm not the best reviewer for the
WebUI implementation itself, I don't have much experience with that.

Cheers,
Jói

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/DEPS
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/D...
components/dom_distiller/DEPS:4: # The dom distiller is a layered component;
subdirectories must explicitly
Perhaps include a link to the layered components design doc.

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/D...
components/dom_distiller/DEPS:8: #"+components/dom_distiller/core",
?

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
File components/dom_distiller/webui/DEPS (right):

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
components/dom_distiller/webui/DEPS:3: "+content/public/browser",
Since this folder depends on content, should it be under
//components/dom_distiller/content/webui?

[nyquist, 2013-09-10 01:56:53]

joi: PTAL

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/DEPS
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/D...
components/dom_distiller/DEPS:4: # The dom distiller is a layered component;
subdirectories must explicitly
On 2013/09/09 08:29:58, Jói wrote:
> Perhaps include a link to the layered components design doc.

Done.

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/D...
components/dom_distiller/DEPS:8: #"+components/dom_distiller/core",
On 2013/09/09 08:29:58, Jói wrote:
> ?

Removed. Will be added in a later CL.

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
File components/dom_distiller/webui/DEPS (right):

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
components/dom_distiller/webui/DEPS:3: "+content/public/browser",
On 2013/09/09 08:29:58, Jói wrote:
> Since this folder depends on content, should it be under
> //components/dom_distiller/content/webui?

Added a more thorough explanation after talking to stuartmorgan@. I am fine
either way regarding a strict per-header-file approach or a full inclusion of
content/public/browser. Stuart argued that iOS tries to not be a nuisance to
engineers doing refactorings, and therefore wants to err on the side of allowing
more things.

[Jói, 2013-09-10 07:23:40]

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
File components/dom_distiller/webui/DEPS (right):

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
components/dom_distiller/webui/DEPS:3: "+content/public/browser",
On 2013/09/10 01:56:53, nyquist wrote:
> On 2013/09/09 08:29:58, Jói wrote:
> > Since this folder depends on content, should it be under
> > //components/dom_distiller/content/webui?
> 
> Added a more thorough explanation after talking to stuartmorgan@. I am fine
> either way regarding a strict per-header-file approach or a full inclusion of
> content/public/browser. Stuart argued that iOS tries to not be a nuisance to
> engineers doing refactorings, and therefore wants to err on the side of
allowing
> more things.

I'm not sure I understand. Does this mean the C++ code in the directory right
now will not be used on iOS, only the HTML/JS code? If that's the case then I
would leave the HTML/JS code here and move the C++ code to
//components/dom_distiller/content/browser/webui.

[nyquist, 2013-09-10 07:51:11]

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
File components/dom_distiller/webui/DEPS (right):

https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
components/dom_distiller/webui/DEPS:3: "+content/public/browser",
No, right now, the plan is to use //components/dom_distiller/webui on all
platforms, including iOS. That goes for both HTML/JS/CSS resources and the C++
code.

However, since the point of the //components/dom_distiller/content/ folder is
that it is supposed to be only used by non-iOS platforms, and we want the webui
to work on iOS, we put it in a directory "on the side" of the other top level
directories in the component.

When more of this feature lands, the main code will live in
//components/dom_distiller/core and there will be a content-version of whatever
is needed from content in //components/dom_distiller/content. And then when we
implements support for iOS, that code will live in
//components/dom_distiller/ios.

However, since WebUI lives in content, but it still supported on iOS, it does
not fit the normal folder structure of a layered component, so after discussing
with stuartmorgan@ we concluded that putting it in a separate folder was the
right thing to do.

On 2013/09/10 07:23:40, Jói wrote:
> On 2013/09/10 01:56:53, nyquist wrote:
> > On 2013/09/09 08:29:58, Jói wrote:
> > > Since this folder depends on content, should it be under
> > > //components/dom_distiller/content/webui?
> > 
> > Added a more thorough explanation after talking to stuartmorgan@. I am fine
> > either way regarding a strict per-header-file approach or a full inclusion
of
> > content/public/browser. Stuart argued that iOS tries to not be a nuisance to
> > engineers doing refactorings, and therefore wants to err on the side of
> allowing
> > more things.
> 
> I'm not sure I understand. Does this mean the C++ code in the directory right
> now will not be used on iOS, only the HTML/JS code? If that's the case then I
> would leave the HTML/JS code here and move the C++ code to
> //components/dom_distiller/content/browser/webui.

[Jói, 2013-09-10 08:14:55]

OK, LGTM.

It's up to you how to write the DEPS file for your webui directory,
but I would suggest once the implementation is relatively stable, it
will help keep you safe from accidental dependency creep if you
restrict the allowed dependencies to just those things that are
supported by iOS for WebUI.

Cheers,
Jói


On Tue, Sep 10, 2013 at 9:51 AM,  <nyquist@chromium.org> wrote:
>
>
https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
> File components/dom_distiller/webui/DEPS (right):
>
>
https://codereview.chromium.org/23503042/diff/7001/components/dom_distiller/w...
> components/dom_distiller/webui/DEPS:3: "+content/public/browser",
> No, right now, the plan is to use //components/dom_distiller/webui on
> all platforms, including iOS. That goes for both HTML/JS/CSS resources
> and the C++ code.
>
> However, since the point of the //components/dom_distiller/content/
> folder is that it is supposed to be only used by non-iOS platforms, and
> we want the webui to work on iOS, we put it in a directory "on the side"
> of the other top level directories in the component.
>
> When more of this feature lands, the main code will live in
> //components/dom_distiller/core and there will be a content-version of
> whatever is needed from content in //components/dom_distiller/content.
> And then when we implements support for iOS, that code will live in
> //components/dom_distiller/ios.
>
> However, since WebUI lives in content, but it still supported on iOS, it
> does not fit the normal folder structure of a layered component, so
> after discussing with stuartmorgan@ we concluded that putting it in a
> separate folder was the right thing to do.
>
>
> On 2013/09/10 07:23:40, Jói wrote:
>>
>> On 2013/09/10 01:56:53, nyquist wrote:
>> > On 2013/09/09 08:29:58, Jói wrote:
>> > > Since this folder depends on content, should it be under
>> > > //components/dom_distiller/content/webui?
>> >
>> > Added a more thorough explanation after talking to stuartmorgan@. I
>
> am fine
>>
>> > either way regarding a strict per-header-file approach or a full
>
> inclusion of
>>
>> > content/public/browser. Stuart argued that iOS tries to not be a
>
> nuisance to
>>
>> > engineers doing refactorings, and therefore wants to err on the side
>
> of
>>
>> allowing
>> > more things.
>
>
>> I'm not sure I understand. Does this mean the C++ code in the
>
> directory right
>>
>> now will not be used on iOS, only the HTML/JS code? If that's the case
>
> then I
>>
>> would leave the HTML/JS code here and move the C++ code to
>> //components/dom_distiller/content/browser/webui.
>
>
> https://codereview.chromium.org/23503042/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2013-09-10 21:41:15]

https://codereview.chromium.org/23503042/diff/47001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/23503042/diff/47001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:163: // Special case
for DOM distiller.
you should not put this in between the other two extensions webuis.

[nyquist, 2013-09-10 22:11:41]

https://codereview.chromium.org/23503042/diff/47001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/23503042/diff/47001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:163: // Special case
for DOM distiller.
On 2013/09/10 21:41:16, Evan Stade wrote:
> you should not put this in between the other two extensions webuis.

Done.

[Evan Stade, 2013-09-11 01:31:55]

I'm not sure if I'm supposed to be reviewing this.

[nyquist, 2013-09-11 01:32:41]

estade: PTAL

[Evan Stade, 2013-09-11 01:44:01]

https://codereview.chromium.org/23503042/diff/52001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/23503042/diff/52001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:184:
WebUIController* NewWebUI<dom_distiller::DomDistillerUI>(WebUI* web_ui,
I actually don't see why you even need this. The url and host arguments are
unused.

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:21: 
^H

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:24:
content::WebUIDataSource::Create(host);
you can hardcode the host here (perhaps a new url constants file is in order for
WebUIs in components).

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:29:
IDR_ABOUT_DOM_DISTILLER_JS);
args should be aligned horizontally

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:42:
DomDistillerUI::~DomDistillerUI() {
{} on same line

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.h (right):

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.h:18: virtual ~DomDistillerUI();
\n

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
File components/dom_distiller/webui/resources/about_dom_distiller.js (right):

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:18: var linkHref
= document.createAttribute('href');
setAttribute()

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:19: var text =
document.createTextNode(entries[i].title);
innerText =

[cjhopman, 2013-09-12 00:41:00]

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:24:
content::WebUIDataSource::Create(host);
On 2013/09/11 01:44:01, Evan Stade wrote:
> you can hardcode the host here (perhaps a new url constants file is in order
for
> WebUIs in components).

We also know that url.host() == host here, we could just use that.

[stuartmorgan, 2013-09-12 05:25:23]

Overall structure looks good; my only non-nit concern is about the bundling of
resources.

https://codereview.chromium.org/23503042/diff/65001/chrome/chrome_repack_reso...
File chrome/chrome_repack_resources.gypi (right):

https://codereview.chromium.org/23503042/diff/65001/chrome/chrome_repack_reso...
chrome/chrome_repack_resources.gypi:9:
'<(SHARED_INTERMEDIATE_DIR)/components/components_resources.pak',
This structure worries me; an important aspect of componentization is the idea
that specific pieces can be included or not, per build/platform. This doesn't
seem to allow for that... or is the idea that the building of
components_resources.pak has ifdefs?

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/DEPS (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/DEPS:5: # also supposed to be working on iOS, and
there is currently no plan for
How about "no concrete plan"? (This sounds like we don't plan to solve this,
when in reality we do plan to, it's just a ways off, and we don't know what form
the solution will take yet.)

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/DEPS:8: # To ensure this DEPS rule is not a
nuisance to engineers refactorings the
refactoring

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_handler.cc:30:
scoped_ptr<base::DictionaryValue> entry1(new base::DictionaryValue());
These are temporary placeholders I assume? If so, a comment would be helpful.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:22: // Setup WebUIDataSource.
Pet-peeve nit: Setup is a noun; the verb is set up.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.h (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.h:15: explicit
DomDistillerUI(content::WebUI* web_ui,
Don't we only put explicit on single-argument constructors?

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/resources/about_dom_distiller.js (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:15: for (var
i=0; i < entries.length; i++) {
Spaces around =

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:30: /* All the
work we do onload. */
on load

[nyquist, 2013-09-12 17:42:11]

estade, stuartmorgan: PTAL

https://codereview.chromium.org/23503042/diff/52001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/23503042/diff/52001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:184:
WebUIController* NewWebUI<dom_distiller::DomDistillerUI>(WebUI* web_ui,
On 2013/09/11 01:44:01, Evan Stade wrote:
> I actually don't see why you even need this. The url and host arguments are
> unused.

Moved the constant to the component. The plan is to use the url if the request
is towards something like dom-distiller/entry/SOME_ID. Is it OK to keep it for
that? Or should we go through the WebContents to find the current URL instead of
depending on it being injected here?

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:21: 
On 2013/09/11 01:44:01, Evan Stade wrote:
> ^H

Done.

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:24:
content::WebUIDataSource::Create(host);
On 2013/09/12 00:41:00, cjhopman wrote:
> On 2013/09/11 01:44:01, Evan Stade wrote:
> > you can hardcode the host here (perhaps a new url constants file is in order
> for
> > WebUIs in components).
> 
> We also know that url.host() == host here, we could just use that.
> 

Removed the host argument, but kept the URL for now since the plan is to use it
for chrome://dom-distiller/entry/SOME_ID. Is that OK?

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:29:
IDR_ABOUT_DOM_DISTILLER_JS);
On 2013/09/11 01:44:01, Evan Stade wrote:
> args should be aligned horizontally

Done.

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:42:
DomDistillerUI::~DomDistillerUI() {
On 2013/09/11 01:44:01, Evan Stade wrote:
> {} on same line

Done.

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.h (right):

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.h:18: virtual ~DomDistillerUI();
On 2013/09/11 01:44:01, Evan Stade wrote:
> \n

Done.

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
File components/dom_distiller/webui/resources/about_dom_distiller.js (right):

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:18: var linkHref
= document.createAttribute('href');
On 2013/09/11 01:44:01, Evan Stade wrote:
> setAttribute()

Done.

https://codereview.chromium.org/23503042/diff/52001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:19: var text =
document.createTextNode(entries[i].title);
On 2013/09/11 01:44:01, Evan Stade wrote:
> innerText =

Done.

https://codereview.chromium.org/23503042/diff/65001/chrome/chrome_repack_reso...
File chrome/chrome_repack_resources.gypi (right):

https://codereview.chromium.org/23503042/diff/65001/chrome/chrome_repack_reso...
chrome/chrome_repack_resources.gypi:9:
'<(SHARED_INTERMEDIATE_DIR)/components/components_resources.pak',
On 2013/09/12 05:25:24, stuartmorgan wrote:
> This structure worries me; an important aspect of componentization is the idea
> that specific pieces can be included or not, per build/platform. This doesn't
> seem to allow for that... or is the idea that the building of
> components_resources.pak has ifdefs?

Changed this to be a separate pak file for only dom_distiller instead.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/DEPS (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/DEPS:5: # also supposed to be working on iOS, and
there is currently no plan for
On 2013/09/12 05:25:24, stuartmorgan wrote:
> How about "no concrete plan"? (This sounds like we don't plan to solve this,
> when in reality we do plan to, it's just a ways off, and we don't know what
form
> the solution will take yet.)

Done.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/DEPS:8: # To ensure this DEPS rule is not a
nuisance to engineers refactorings the
On 2013/09/12 05:25:24, stuartmorgan wrote:
> refactoring

Done.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_handler.cc:30:
scoped_ptr<base::DictionaryValue> entry1(new base::DictionaryValue());
On 2013/09/12 05:25:24, stuartmorgan wrote:
> These are temporary placeholders I assume? If so, a comment would be helpful.

Done.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:22: // Setup WebUIDataSource.
On 2013/09/12 05:25:24, stuartmorgan wrote:
> Pet-peeve nit: Setup is a noun; the verb is set up.

Done.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.h (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.h:15: explicit
DomDistillerUI(content::WebUI* web_ui,
On 2013/09/12 05:25:24, stuartmorgan wrote:
> Don't we only put explicit on single-argument constructors?

Done.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
File components/dom_distiller/webui/resources/about_dom_distiller.js (right):

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:15: for (var
i=0; i < entries.length; i++) {
On 2013/09/12 05:25:24, stuartmorgan wrote:
> Spaces around =

Done.

https://codereview.chromium.org/23503042/diff/65001/components/dom_distiller/...
components/dom_distiller/webui/resources/about_dom_distiller.js:30: /* All the
work we do onload. */
On 2013/09/12 05:25:24, stuartmorgan wrote:
> on load

Done.

[Evan Stade, 2013-09-13 21:07:05]

https://codereview.chromium.org/23503042/diff/63001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/63001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:19: const GURL& url)
regardless of whether you end up passing it via the constructor or checking the
URL of the web contents, I think you should avoid adding this here until it's
actually used.

In the long run, is there any reason not to get it from the web contents
(without having the code in place yet, it's hard to know when or how you're
planning to use the ID)

[nyquist, 2013-09-13 21:40:27]

https://codereview.chromium.org/23503042/diff/63001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/63001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:19: const GURL& url)
On 2013/09/13 21:07:05, Evan Stade wrote:
> regardless of whether you end up passing it via the constructor or checking
the
> URL of the web contents, I think you should avoid adding this here until it's
> actually used.
> 
> In the long run, is there any reason not to get it from the web contents
> (without having the code in place yet, it's hard to know when or how you're
> planning to use the ID)

I'll remove this for now.

[nyquist, 2013-09-13 22:21:04]

estade: PTAL
stuartmorgan: PTAL

https://codereview.chromium.org/23503042/diff/63001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/23503042/diff/63001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.cc:19: const GURL& url)
On 2013/09/13 21:40:27, nyquist wrote:
> On 2013/09/13 21:07:05, Evan Stade wrote:
> > regardless of whether you end up passing it via the constructor or checking
> the
> > URL of the web contents, I think you should avoid adding this here until
it's
> > actually used.
> > 
> > In the long run, is there any reason not to get it from the web contents
> > (without having the code in place yet, it's hard to know when or how you're
> > planning to use the ID)
> 
> I'll remove this for now.

Done.

[Evan Stade, 2013-09-14 00:57:49]

lgtm

[nyquist, 2013-09-16 21:23:40]

tim: FYI

[stuartmorgan, 2013-09-16 21:29:12]

https://codereview.chromium.org/23503042/diff/54001/components/component_stri...
File components/component_strings.grd (right):

https://codereview.chromium.org/23503042/diff/54001/components/component_stri...
components/component_strings.grd:171: <part file="dom_distiller_strings.grdp" />
Strings has the same problem as resources, so maybe that doesn't help us much
yet? But at least we have a model that we could potentially move strings toward,
and at least we're closer to a split model.

https://codereview.chromium.org/23503042/diff/54001/tools/gritsettings/resour...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/23503042/diff/54001/tools/gritsettings/resour...
tools/gritsettings/resource_ids:217: "components/dom_distiller_resources.grd": {
If we do each component separately, I guess we're going to have to rethink the
ranges we use. We can tackle that when the time comes though.

[tim (not reviewing), 2013-09-17 00:02:17]

LGTM with nits

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
File components/dom_distiller/README (right):

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
components/dom_distiller/README:6: The webui/ folder contains the WebUI code and
resources for the debug page.
Maybe describe the other folder structure as well?

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
File components/dom_distiller/core/dom_distiller_constants.h (right):

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
components/dom_distiller/core/dom_distiller_constants.h:15: #endif  //
COMPONENTS_DOM_DISTILLER_WEBUI_DOM_DISTILLER_HANDLER_H_
endif guard comment is wrong, should be ..._DOM_DISTILLER_CONSTANTS_H_

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.h (right):

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.h:5: #ifndef
COMPONENTS_DOM_DISTILLER_WEBUI_DOM_DISTILLER_H_
guard comments are wrong, should be DOM_DISTILLER_UI_H_

[nyquist, 2013-09-17 00:16:10]

all done

https://codereview.chromium.org/23503042/diff/54001/components/component_stri...
File components/component_strings.grd (right):

https://codereview.chromium.org/23503042/diff/54001/components/component_stri...
components/component_strings.grd:171: <part file="dom_distiller_strings.grdp" />
On 2013/09/16 21:29:12, stuartmorgan wrote:
> Strings has the same problem as resources, so maybe that doesn't help us much
> yet? But at least we have a model that we could potentially move strings
toward,
> and at least we're closer to a split model.

Filed http://crbug.com/293045

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
File components/dom_distiller/README (right):

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
components/dom_distiller/README:6: The webui/ folder contains the WebUI code and
resources for the debug page.
On 2013/09/17 00:02:17, timsteele wrote:
> Maybe describe the other folder structure as well?

Done.

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
File components/dom_distiller/core/dom_distiller_constants.h (right):

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
components/dom_distiller/core/dom_distiller_constants.h:15: #endif  //
COMPONENTS_DOM_DISTILLER_WEBUI_DOM_DISTILLER_HANDLER_H_
On 2013/09/17 00:02:17, timsteele wrote:
> endif guard comment is wrong, should be ..._DOM_DISTILLER_CONSTANTS_H_

Done.

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
File components/dom_distiller/webui/dom_distiller_ui.h (right):

https://codereview.chromium.org/23503042/diff/54001/components/dom_distiller/...
components/dom_distiller/webui/dom_distiller_ui.h:5: #ifndef
COMPONENTS_DOM_DISTILLER_WEBUI_DOM_DISTILLER_H_
On 2013/09/17 00:02:17, timsteele wrote:
> guard comments are wrong, should be DOM_DISTILLER_UI_H_

Done.

https://codereview.chromium.org/23503042/diff/54001/tools/gritsettings/resour...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/23503042/diff/54001/tools/gritsettings/resour...
tools/gritsettings/resource_ids:217: "components/dom_distiller_resources.grd": {
On 2013/09/16 21:29:12, stuartmorgan wrote:
> If we do each component separately, I guess we're going to have to rethink the
> ranges we use. We can tackle that when the time comes though.

Agreed. Maybe a tool could be written to identify what the over-subscription is
for different .grd files, so it would be at least easy to find new places to
take numbers from.

[commit-bot: I haz the power, 2013-09-17 00:18:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/23503042/114001

[commit-bot: I haz the power, 2013-09-17 00:43:38]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-17 01:17:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/23503042/114001

[commit-bot: I haz the power, 2013-09-17 04:01:13]

Change committed as 223528

[commit-bot: I haz the power, 2013-09-17 20:26:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/23503042/121001

[commit-bot: I haz the power, 2013-09-17 20:42:51]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[nyquist, 2013-09-17 21:02:03]

sky: chrome/chrome_resources.gyp FYI

[commit-bot: I haz the power, 2013-09-17 21:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/23503042/121001

[commit-bot: I haz the power, 2013-09-18 01:16:54]

Change committed as 223759