Initial implementation of WebUI generator (WUG) toolkit.

WUG (Web UI generator) is an utility toolkit which
facilitates WebUI creation.

All parts of WUG are implemented:
* Base classes for view (View) and view-model (ViewModel)
* WebUIView -- specialization of View for use in WebUI.
* View's CC, HTML and JS code generator
* Templates for GYP and GN which help to generate code and
  produce components from generated code.

Tests to follow.

TEST=none
TBR=avi@chromium.org,mark@chromium.org
BUG=459230
Committed: https://crrev.com/c1c2a503cdd7a68e7c25d36213a15c97e62a9ba7
Cr-Commit-Position: refs/heads/master@{#318884}

[dzhioev (left Google), 2015-02-19 19:38:22]

dzhioev@chromium.org changed reviewers:
+ blundell@chromium.org, nkostylev@chromium.org

[dzhioev (left Google), 2015-02-19 19:44:34]

Hello, please review.

blundell@: components/ owner's review
nkostylev@: components/wug review.

[blundell, 2015-02-20 07:09:00]

This is a large amount of code to drop in, especially with such a minimal CL
description :(. Could you at least expand the CL description to provide a full
from-scratch explanation of what this feature is doing and who is/will be using
it?

[blundell, 2015-02-20 07:09:38]

Also, webui_generator would be a more clear name for the component than wug.

[blundell, 2015-02-20 07:11:51]

On 2015/02/20 07:09:00, blundell wrote:
> This is a large amount of code to drop in, especially with such a minimal CL
> description :(. Could you at least expand the CL description to provide a full
> from-scratch explanation of what this feature is doing and who is/will be
using
> it?

(Sorry for the tone of that message; I didn't intend it to be so negative).

[Nikita (slow), 2015-02-24 13:21:09]

On 2015/02/20 07:09:38, blundell wrote:
> Also, webui_generator would be a more clear name for the component than wug.

I propose webui_gen since gen is commonly used as a short version for
generator/generated.

[blundell, 2015-02-24 13:31:27]

On 2015/02/24 13:21:09, Nikita wrote:
> On 2015/02/20 07:09:38, blundell wrote:
> > Also, webui_generator would be a more clear name for the component than wug.
> 
> I propose webui_gen since gen is commonly used as a short version for
> generator/generated.

AFAIK "gen" is usually used to stand for "generated" in Chromium; I fear that
"webui_gen" could be confusing as it could give the impression that it is
generated code.

[Nikita (slow), 2015-02-24 15:22:44]

mostly nits

Please update CL description and include link to the document even though it is
still in progress.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/context.html (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/context.html:7: <script src="context.js"></script>
nit: fix indent

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/webui-view.html (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.html:7: 
nit: drop empty line

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/webui-view.js (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:33: * Internal storage of |this.context|.
Short name has been choosen for
nit: You can make this comment shorter since you're already using short name for
this variable :)

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:42: * TODO(dzhioev): make binding
two-way.
Since this is purely an optimization I suggest removing this TODO. You can file
a bug though.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:58: console.error('"wugid" attribute is
missing.');
nit: Should you return here and do not initialize domReady?

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:117: *   this.addContextObserver('key',
this.onKeyChanged_);
Not clear whether this example on how caller should look like.
.. or how it should not look like.

Please clarify comment.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc
File components/wug/view.cc (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:27: if (!IsRootView())
nit: insert empty line.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:32: for (auto& id_child : children_) {
nit: insert empty line, drop {}

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:35: if (children_.empty())
nit: insert empty line.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:57: return it->second;
nit: insert empty line.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:63: ready_ = true;
nit: insert empty line.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h
File components/wug/view.h (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:21: class WUG_EXPORT View {
nit: Add comment.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:21: class WUG_EXPORT View {
I suggest renaming View to smth else.
Otherwise it might get confused with src/ui/views/view.h

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:23: View(const std::string& id);
explicit

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:27: bool IsRootView() const;
nit: add comment i.e. what "root view" means.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:35: const std::string& path() { return path_; }
nit: Add comment. Not clear what "path" means.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:43: virtual void OnReady();
nit: Rename to OnViewAndChildrenReady()

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:51: bool ready() { return ready_; }
nit: Should this be public instead?

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:68: std::string path_;
nit: comment

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:69: int ready_children_;
nit: comment

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:77: DISALLOW_COPY_AND_ASSIGN(View);
nit: insert empty line

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
File components/wug/view_model.cc (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
components/wug/view_model.cc:100: base::DictionaryValue diff;
nit: insert empty line

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_model.h
File components/wug/view_model.h (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
components/wug/view_model.h:49: class ContextEditor {
How is this different than
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
components/wug/view_model.h:102: DISALLOW_COPY_AND_ASSIGN(ViewModel);
nit: insert empty line

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
File components/wug/web_ui_view.cc (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:19: const char kMethodContextChanged[] =
"contextChanged";
nit: Should this be the same as kContextChangedCallback?

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:57: if (view_bound_) {
nit: drop {}

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:62: pending_context_changes_->ApplyChanges(diff,
nullptr);
nit: insert empty line

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:84: if (it == resources->end()) {
nit: drop {}

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:87: got_data_callback.Run(it->second.get());
nit: insert empty line

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:103: if (pending_context_changes_) {
nit: drop {}

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:106: GetViewModel()->OnViewBound();
nit: insert empty line

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
File components/wug/web_ui_view.h (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.h:37: 
nit: Add comment

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.h:49: using ResourcesMap =
typedef?

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.h:77: private:
nit: Please add comments for methods

https://codereview.chromium.org/928163002/diff/80001/components/wug_strings.grdp
File components/wug_strings.grdp (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug_strings.g...
components/wug_strings.grdp:2: <grit-part>
nit: Maybe we should have a short comment in this file on what these strings are
used for?

[Nikita (slow), 2015-02-24 15:23:49]

FYI I didn't look over Python files - please find another reviewer.

I'll go over gypi/gn files later.

[dzhioev (left Google), 2015-02-25 15:40:16]

Hello Colin,

sorry for a poor description, I didn't expect that you are
going to review components/wug interiors. I updated the
description a little.

WUG is a small toolkit that we are going to use in the new
implementation of OOBE UI. You can find more information in
document http://go/wug-howto (WIP).

Here is a CL [1] where I implemented DemoUI with a help of WUG. [2] is a video
with demonstration. 

[1]: http://codereview.chromium.org/925333002/
[2]: http://youtu.be/VBfzj8kWBYs

[blundell, 2015-02-25 15:48:01]

Well, it's up to Nikita to judge on the scope of the CL :). I do think the
component should be called webui_generator though. Clarity is most important to
optimize for (and hey, it's a lot shorter than web_contents_delegate_android).

Please ping me again once Nikita has LGTM'd and the component name has changed
and I'll do a top-level owners review. (and/or we can bikeshed about the
component name ;).

[Nikita (slow), 2015-02-25 18:21:59]

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/declaration.py (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:19: DECLARATION_SCHEMA = (dict, {
As discussed, add optional comment field (or add TODO).

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:31: 'events': (False, (list, unicode)),
As discussed, make this list of dicts.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:32: 'strings': (False, (list, unicode))
As discussed, make this list of dicts.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:143: for import_path in
self.data['imports']:
Add TODO to eliminate possibility of circular references.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:150:
known_declarations.GetByType(child_data['type'])
nit: Check that child declaration is known.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:155: if 'namespace' not in self.data:
As discussed, remove namespace.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:170: def namespace(self):
nit: drop this one

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:231: return '%s-view' %
self.type.replace('_', '-')
Please use single naming guideline.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/gen_sources.py (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/gen_sources.py:18: declaration =
Declaration(declaration_path)
Add file name to exception message.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/wug.gni (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/wug.gni:16: # should be 'full_path_to_file_json'. This
is needed to properly handle
nit: update comment

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/wug.gypi (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/wug.gypi:22: # full path (relative to src), by
replacing every nonalphanumeric character with
nit: update comment

[dzhioev (left Google), 2015-02-26 14:01:40]

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/context.html (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/context.html:7: <script src="context.js"></script>
On 2015/02/24 15:22:41, Nikita wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/webui-view.html (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.html:7: 
On 2015/02/24 15:22:42, Nikita wrote:
> nit: drop empty line
This line was left intentionally to separate <link>s block with the rest.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/webui-view.js (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:33: * Internal storage of |this.context|.
Short name has been choosen for
On 2015/02/24 15:22:42, Nikita wrote:
> nit: You can make this comment shorter since you're already using short name
for
> this variable :)

What do you mean? this.C and this.context are different objects.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:42: * TODO(dzhioev): make binding
two-way.
On 2015/02/24 15:22:42, Nikita wrote:
> Since this is purely an optimization I suggest removing this TODO. You can
file
> a bug though.

Done. Removed comment.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:58: console.error('"wugid" attribute is
missing.');
On 2015/02/24 15:22:42, Nikita wrote:
> nit: Should you return here and do not initialize domReady?

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:117: *   this.addContextObserver('key',
this.onKeyChanged_);
On 2015/02/24 15:22:42, Nikita wrote:
> Not clear whether this example on how caller should look like.
> .. or how it should not look like.
> 
> Please clarify comment.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc
File components/wug/view.cc (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:27: if (!IsRootView())
On 2015/02/24 15:22:42, Nikita wrote:
> nit: insert empty line.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:32: for (auto& id_child : children_) {
On 2015/02/24 15:22:42, Nikita wrote:
> nit: insert empty line, drop {}

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:35: if (children_.empty())
On 2015/02/24 15:22:42, Nikita wrote:
> nit: insert empty line.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:57: return it->second;
On 2015/02/24 15:22:42, Nikita wrote:
> nit: insert empty line.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.cc#n...
components/wug/view.cc:63: ready_ = true;
On 2015/02/24 15:22:42, Nikita wrote:
> nit: insert empty line.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h
File components/wug/view.h (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:21: class WUG_EXPORT View {
On 2015/02/24 15:22:43, Nikita wrote:
> I suggest renaming View to smth else.
> Otherwise it might get confused with src/ui/views/view.h

I don't think that it is a problem.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:21: class WUG_EXPORT View {
On 2015/02/24 15:22:42, Nikita wrote:
> nit: Add comment.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:23: View(const std::string& id);
On 2015/02/24 15:22:43, Nikita wrote:
> explicit

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:27: bool IsRootView() const;
On 2015/02/24 15:22:43, Nikita wrote:
> nit: add comment i.e. what "root view" means.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:35: const std::string& path() { return path_; }
On 2015/02/24 15:22:43, Nikita wrote:
> nit: Add comment. Not clear what "path" means.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:43: virtual void OnReady();
On 2015/02/24 15:22:42, Nikita wrote:
> nit: Rename to OnViewAndChildrenReady()

Changed comment instead.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:51: bool ready() { return ready_; }
On 2015/02/24 15:22:43, Nikita wrote:
> nit: Should this be public instead?
I don't think so. Made it 'const'.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:68: std::string path_;
On 2015/02/24 15:22:43, Nikita wrote:
> nit: comment

Already added comment to path()

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:69: int ready_children_;
On 2015/02/24 15:22:42, Nikita wrote:
> nit: comment

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view.h#ne...
components/wug/view.h:77: DISALLOW_COPY_AND_ASSIGN(View);
On 2015/02/24 15:22:43, Nikita wrote:
> nit: insert empty line

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
File components/wug/view_model.cc (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
components/wug/view_model.cc:100: base::DictionaryValue diff;
On 2015/02/24 15:22:43, Nikita wrote:
> nit: insert empty line 

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_model.h
File components/wug/view_model.h (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
components/wug/view_model.h:49: class ContextEditor {
On 2015/02/24 15:22:43, Nikita wrote:
> How is this different than
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

This one use ViewModel instead of BaseScreen to commit changes.

https://codereview.chromium.org/928163002/diff/80001/components/wug/view_mode...
components/wug/view_model.h:102: DISALLOW_COPY_AND_ASSIGN(ViewModel);
On 2015/02/24 15:22:43, Nikita wrote:
> nit: insert empty line

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
File components/wug/web_ui_view.cc (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:19: const char kMethodContextChanged[] =
"contextChanged";
On 2015/02/24 15:22:43, Nikita wrote:
> nit: Should this be the same as kContextChangedCallback?

No, it shouldn't

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:57: if (view_bound_) {
On 2015/02/24 15:22:43, Nikita wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:62: pending_context_changes_->ApplyChanges(diff,
nullptr);
On 2015/02/24 15:22:44, Nikita wrote:
> nit: insert empty line

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:84: if (it == resources->end()) {
On 2015/02/24 15:22:43, Nikita wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:87: got_data_callback.Run(it->second.get());
On 2015/02/24 15:22:43, Nikita wrote:
> nit: insert empty line

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:103: if (pending_context_changes_) {
On 2015/02/24 15:22:43, Nikita wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.cc:106: GetViewModel()->OnViewBound();
On 2015/02/24 15:22:43, Nikita wrote:
> nit: insert empty line

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
File components/wug/web_ui_view.h (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.h:37: 
On 2015/02/24 15:22:44, Nikita wrote:
> nit: Add comment

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.h:49: using ResourcesMap =
On 2015/02/24 15:22:44, Nikita wrote:
> typedef?

'using' is a preferred way for making type definitions. See
https://chromium-cpp.appspot.com/

https://codereview.chromium.org/928163002/diff/80001/components/wug/web_ui_vi...
components/wug/web_ui_view.h:77: private:
On 2015/02/24 15:22:44, Nikita wrote:
> nit: Please add comments for methods

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug_strings.grdp
File components/wug_strings.grdp (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug_strings.g...
components/wug_strings.grdp:2: <grit-part>
On 2015/02/24 15:22:44, Nikita wrote:
> nit: Maybe we should have a short comment in this file on what these strings
are
> used for?

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/declaration.py (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:19: DECLARATION_SCHEMA = (dict, {
On 2015/02/25 18:21:59, Nikita wrote:
> As discussed, add optional comment field (or add TODO).

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:31: 'events': (False, (list, unicode)),
On 2015/02/25 18:21:59, Nikita wrote:
> As discussed, make this list of dicts.

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:32: 'strings': (False, (list, unicode))
On 2015/02/25 18:21:59, Nikita wrote:
> As discussed, make this list of dicts.

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:143: for import_path in
self.data['imports']:
On 2015/02/25 18:21:59, Nikita wrote:
> Add TODO to eliminate possibility of circular references.

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:150:
known_declarations.GetByType(child_data['type'])
On 2015/02/25 18:21:59, Nikita wrote:
> nit: Check that child declaration is known.

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:155: if 'namespace' not in self.data:
On 2015/02/25 18:21:59, Nikita wrote:
> As discussed, remove namespace.

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:170: def namespace(self):
On 2015/02/25 18:21:59, Nikita wrote:
> nit: drop this one

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/declaration.py:231: return '%s-view' %
self.type.replace('_', '-')
On 2015/02/25 18:21:59, Nikita wrote:
> Please use single naming guideline.

I started discussion on chromium-polymer@chromium.org, waiting for decision.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/gen_sources.py (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/gen_sources.py:18: declaration =
Declaration(declaration_path)
On 2015/02/25 18:21:59, Nikita wrote:
> Add file name to exception message.

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/wug.gni (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/wug.gni:16: # should be 'full_path_to_file_json'. This
is needed to properly handle
On 2015/02/25 18:21:59, Nikita wrote:
> nit: update comment

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/wug.gypi (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/wug.gypi:22: # full path (relative to src), by
replacing every nonalphanumeric character with
On 2015/02/25 18:21:59, Nikita wrote:
> nit: update comment

Done.

[Nikita (slow), 2015-02-26 14:18:21]

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/webui-view.js (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:33: * Internal storage of |this.context|.
Short name has been choosen for
On 2015/02/26 14:01:37, dzhioev wrote:
> On 2015/02/24 15:22:42, Nikita wrote:
> > nit: You can make this comment shorter since you're already using short name
> for
> > this variable :)
> 
> What do you mean? this.C and this.context are different objects.

I suggested removing reasoning for using short name.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:37: * changes in the native part appear
in C automticaly. Reverse is not true,
nit: automatically

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/web_ui_view.py (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/web_ui_view.py:32: protected:
nit: insert empty line.

[Nikita (slow), 2015-02-26 14:28:58]

lgtm

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h
File components/wug/view.h (right):

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:22: * Base block of the UI. Every View instance is
associated with single
nit: please mention what type of UI.
There's no mention about WebUI here.

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:84: // Number of ready child views.
nit: number of child views that are ready.

[Denis Kuznetsov (DE-MUC), 2015-02-26 14:35:22]

antrim@chromium.org changed reviewers:
+ antrim@chromium.org

[Denis Kuznetsov (DE-MUC), 2015-02-26 14:35:23]

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h
File components/wug/view.h (right):

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:31: virtual void Init();
It is not obvious that  OnReady should be called as a result of Init call,
should be documented on Init().
Generally, all virtual methods should have some contract that declares how
someone should override them.

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:49: View* GetChild(const std::string& id) const;
Add comment to method, including specifics on how missing children are handled.

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:76: void OnChildReady(View* child);
Need a bit more details on "Ready" contract:
 - Can we  (dynamically) add children after Init is called? 
 - After whole tree is ready?

[dzhioev (left Google), 2015-03-02 11:02:34]

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
File components/wug/resources/webui-view.js (right):

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:33: * Internal storage of |this.context|.
Short name has been choosen for
On 2015/02/26 14:18:20, Nikita wrote:
> On 2015/02/26 14:01:37, dzhioev wrote:
> > On 2015/02/24 15:22:42, Nikita wrote:
> > > nit: You can make this comment shorter since you're already using short
name
> > for
> > > this variable :)
> > 
> > What do you mean? this.C and this.context are different objects.
> 
> I suggested removing reasoning for using short name.

Done.

https://codereview.chromium.org/928163002/diff/80001/components/wug/resources...
components/wug/resources/webui-view.js:37: * changes in the native part appear
in C automticaly. Reverse is not true,
On 2015/02/26 14:18:20, Nikita wrote:
> nit: automatically

Done.

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
File components/wug/generator/web_ui_view.py (right):

https://codereview.chromium.org/928163002/diff/100001/components/wug/generato...
components/wug/generator/web_ui_view.py:32: protected:
On 2015/02/26 14:18:20, Nikita wrote:
> nit: insert empty line.

Done.

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h
File components/wug/view.h (right):

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:22: * Base block of the UI. Every View instance is
associated with single
On 2015/02/26 14:28:58, Nikita wrote:
> nit: please mention what type of UI.
> There's no mention about WebUI here.

Done.

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:31: virtual void Init();
On 2015/02/26 14:35:23, Denis Kuznetsov wrote:
> It is not obvious that  OnReady should be called as a result of Init call,
> should be documented on Init().
> Generally, all virtual methods should have some contract that declares how
> someone should override them.

Done.

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:49: View* GetChild(const std::string& id) const;
On 2015/02/26 14:35:23, Denis Kuznetsov wrote:
> Add comment to method, including specifics on how missing children are
handled.

Done.

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:76: void OnChildReady(View* child);
On 2015/02/26 14:35:23, Denis Kuznetsov wrote:
> Need a bit more details on "Ready" contract:
>  - Can we  (dynamically) add children after Init is called? 
>  - After whole tree is ready?

Added to comments here and near AddChild().

https://codereview.chromium.org/928163002/diff/120001/components/wug/view.h#n...
components/wug/view.h:84: // Number of ready child views.
On 2015/02/26 14:28:58, Nikita wrote:
> nit: number of child views that are ready.

Done.

[dzhioev (left Google), 2015-03-02 11:23:35]

On 2015/02/25 15:48:01, blundell wrote:
> Well, it's up to Nikita to judge on the scope of the CL :). I do think the
> component should be called webui_generator though. Clarity is most important
to
> optimize for (and hey, it's a lot shorter than web_contents_delegate_android).
> 
> Please ping me again once Nikita has LGTM'd and the component name has changed
> and I'll do a top-level owners review. (and/or we can bikeshed about the
> component name ;).

Hello Colin,

I propose to rename only:
* component's name to web_ui_generator
* components/wug to components/web_ui_generator
* components/wug{.gypi,_strings.grdp} to
components/web_ui_generator{.gypi,_strings.gypi}

at the same time I'll keep using acronym for:
* namespaces
* build files with templates (wug.gypi and wug.gni)
* names for generated targets ("foo_bar_wug_generated")

Are you fine with that?

[blundell, 2015-03-02 12:27:37]

On 2015/03/02 11:23:35, dzhioev wrote:
> On 2015/02/25 15:48:01, blundell wrote:
> > Well, it's up to Nikita to judge on the scope of the CL :). I do think the
> > component should be called webui_generator though. Clarity is most important
> to
> > optimize for (and hey, it's a lot shorter than
web_contents_delegate_android).
> > 
> > Please ping me again once Nikita has LGTM'd and the component name has
changed
> > and I'll do a top-level owners review. (and/or we can bikeshed about the
> > component name ;).
> 
> Hello Colin,
> 
> I propose to rename only:
> * component's name to web_ui_generator
> * components/wug to components/web_ui_generator
> * components/wug{.gypi,_strings.grdp} to
> components/web_ui_generator{.gypi,_strings.gypi}
> 
> at the same time I'll keep using acronym for:
> * namespaces
> * build files with templates (wug.gypi and wug.gni)
> * names for generated targets ("foo_bar_wug_generated")
> 
> Are you fine with that?

This organization would be inconsistent with the guidelines for components; I
don't see a strong reason why this component should be different than any other.
If you feel strongly about wanting to avoid "webui_generator" everywhere, I
suggest that you start an email thread with all of the components OWNERS to see
what others think.

Thanks,

Colin

[dzhioev (left Google), 2015-03-02 12:44:49]

On 2015/03/02 12:27:37, blundell wrote:
> On 2015/03/02 11:23:35, dzhioev wrote:
> > On 2015/02/25 15:48:01, blundell wrote:
> > > Well, it's up to Nikita to judge on the scope of the CL :). I do think the
> > > component should be called webui_generator though. Clarity is most
important
> > to
> > > optimize for (and hey, it's a lot shorter than
> web_contents_delegate_android).
> > > 
> > > Please ping me again once Nikita has LGTM'd and the component name has
> changed
> > > and I'll do a top-level owners review. (and/or we can bikeshed about the
> > > component name ;).
> > 
> > Hello Colin,
> > 
> > I propose to rename only:
> > * component's name to web_ui_generator
> > * components/wug to components/web_ui_generator
> > * components/wug{.gypi,_strings.grdp} to
> > components/web_ui_generator{.gypi,_strings.gypi}
> > 
> > at the same time I'll keep using acronym for:
> > * namespaces
> > * build files with templates (wug.gypi and wug.gni)
> > * names for generated targets ("foo_bar_wug_generated")
> > 
> > Are you fine with that?
> 
> This organization would be inconsistent with the guidelines for components; I
> don't see a strong reason why this component should be different than any
other.
> If you feel strongly about wanting to avoid "webui_generator" everywhere, I
> suggest that you start an email thread with all of the components OWNERS to
see
> what others think.
> 
> Thanks,
> 
> Colin
I'm OK with renaming the namespace as well. Is it enough?

[blundell, 2015-03-02 14:26:12]

On 2015/03/02 12:44:49, dzhioev wrote:
> On 2015/03/02 12:27:37, blundell wrote:
> > On 2015/03/02 11:23:35, dzhioev wrote:
> > > On 2015/02/25 15:48:01, blundell wrote:
> > > > Well, it's up to Nikita to judge on the scope of the CL :). I do think
the
> > > > component should be called webui_generator though. Clarity is most
> important
> > > to
> > > > optimize for (and hey, it's a lot shorter than
> > web_contents_delegate_android).
> > > > 
> > > > Please ping me again once Nikita has LGTM'd and the component name has
> > changed
> > > > and I'll do a top-level owners review. (and/or we can bikeshed about the
> > > > component name ;).
> > > 
> > > Hello Colin,
> > > 
> > > I propose to rename only:
> > > * component's name to web_ui_generator
> > > * components/wug to components/web_ui_generator
> > > * components/wug{.gypi,_strings.grdp} to
> > > components/web_ui_generator{.gypi,_strings.gypi}
> > > 
> > > at the same time I'll keep using acronym for:
> > > * namespaces
> > > * build files with templates (wug.gypi and wug.gni)
> > > * names for generated targets ("foo_bar_wug_generated")
> > > 
> > > Are you fine with that?
> > 
> > This organization would be inconsistent with the guidelines for components;
I
> > don't see a strong reason why this component should be different than any
> other.
> > If you feel strongly about wanting to avoid "webui_generator" everywhere, I
> > suggest that you start an email thread with all of the components OWNERS to
> see
> > what others think.
> > 
> > Thanks,
> > 
> > Colin
> I'm OK with renaming the namespace as well. Is it enough?

Yes, I'm OK with that. I personally feel that

webui_generator("my_webui") {
}

is more clear than

wug("my_webui") {
}

but that's more in the domain of the feature itself than part of the structure
of the feature as a component at that point, so my opinion is just an opinion.

[dzhioev (left Google), 2015-03-02 17:19:50]

On 2015/03/02 14:26:12, blundell wrote:
> On 2015/03/02 12:44:49, dzhioev wrote:
> > On 2015/03/02 12:27:37, blundell wrote:
> > > On 2015/03/02 11:23:35, dzhioev wrote:
> > > > On 2015/02/25 15:48:01, blundell wrote:
> > > > > Well, it's up to Nikita to judge on the scope of the CL :). I do think
> the
> > > > > component should be called webui_generator though. Clarity is most
> > important
> > > > to
> > > > > optimize for (and hey, it's a lot shorter than
> > > web_contents_delegate_android).
> > > > > 
> > > > > Please ping me again once Nikita has LGTM'd and the component name has
> > > changed
> > > > > and I'll do a top-level owners review. (and/or we can bikeshed about
the
> > > > > component name ;).
> > > > 
> > > > Hello Colin,
> > > > 
> > > > I propose to rename only:
> > > > * component's name to web_ui_generator
> > > > * components/wug to components/web_ui_generator
> > > > * components/wug{.gypi,_strings.grdp} to
> > > > components/web_ui_generator{.gypi,_strings.gypi}
> > > > 
> > > > at the same time I'll keep using acronym for:
> > > > * namespaces
> > > > * build files with templates (wug.gypi and wug.gni)
> > > > * names for generated targets ("foo_bar_wug_generated")
> > > > 
> > > > Are you fine with that?
> > > 
> > > This organization would be inconsistent with the guidelines for
components;
> I
> > > don't see a strong reason why this component should be different than any
> > other.
> > > If you feel strongly about wanting to avoid "webui_generator" everywhere,
I
> > > suggest that you start an email thread with all of the components OWNERS
to
> > see
> > > what others think.
> > > 
> > > Thanks,
> > > 
> > > Colin
> > I'm OK with renaming the namespace as well. Is it enough?
> 
> Yes, I'm OK with that. I personally feel that
> 
> webui_generator("my_webui") {
> }
> 
> is more clear than
> 
> wug("my_webui") {
> }
> 
> but that's more in the domain of the feature itself than part of the structure
> of the feature as a component at that point, so my opinion is just an opinion.

Done.

Renamed component, namespace, etc. 

I don't want to rename files with generators because I think
'//components/webui_generator/generator/webui_generator.gni' is too verbose.

[blundell, 2015-03-03 08:36:26]

//components LGTM

https://codereview.chromium.org/928163002/diff/180001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/928163002/diff/180001/components/BUILD.gn#new...
components/BUILD.gn:113: "//components/webui_generator",
This component shouldn't be built on iOS since it depends on //content. Same
comment for components.gyp.

[blundell, 2015-03-03 08:37:06]

https://codereview.chromium.org/928163002/diff/180001/components/webui_genera...
File components/webui_generator_strings.grdp (right):

https://codereview.chromium.org/928163002/diff/180001/components/webui_genera...
components/webui_generator_strings.grdp:9: </grit-part>
Hmm, why this empty file?

[dzhioev (left Google), 2015-03-03 09:23:50]

https://codereview.chromium.org/928163002/diff/180001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/928163002/diff/180001/components/BUILD.gn#new...
components/BUILD.gn:113: "//components/webui_generator",
On 2015/03/03 08:36:25, blundell wrote:
> This component shouldn't be built on iOS since it depends on //content. Same
> comment for components.gyp.

Done here and in components.gyp.

https://codereview.chromium.org/928163002/diff/180001/components/webui_genera...
File components/webui_generator_strings.grdp (right):

https://codereview.chromium.org/928163002/diff/180001/components/webui_genera...
components/webui_generator_strings.grdp:9: </grit-part>
On 2015/03/03 08:37:05, blundell wrote:
> Hmm, why this empty file?

This is a file, where strings for generated components will be defined. For now
we don't have any, so this file is empty.

[blundell, 2015-03-03 09:34:01]

Still LGTM

[Denis Kuznetsov (DE-MUC), 2015-03-03 11:07:23]

lgtm

[Denis Kuznetsov (DE-MUC), 2015-03-03 11:07:26]

lgtm

[dzhioev (left Google), 2015-03-03 11:22:19]

The CQ bit was checked by dzhioev@chromium.org

[dzhioev (left Google), 2015-03-03 11:22:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/928163002/#ps200001
(title: "-iOS")

[commit-bot: I haz the power, 2015-03-03 11:23:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/928163002/200001

[commit-bot: I haz the power, 2015-03-03 11:28:24]

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

[commit-bot: I haz the power, 2015-03-03 11:28:25]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[dzhioev (left Google), 2015-03-03 13:30:38]

The CQ bit was checked by dzhioev@chromium.org

[dzhioev (left Google), 2015-03-03 13:30:39]

The patchset sent to the CQ was uploaded after l-g-t-m from antrim@chromium.org,
nkostylev@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/928163002/#ps220001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-03-03 13:31:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/928163002/220001

[dzhioev (left Google), 2015-03-03 13:44:06]

On 2015/03/03 13:31:27, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/928163002/220001

TBRed base/ and content/ owners for added DEPS file.

[commit-bot: I haz the power, 2015-03-03 16:31:56]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-03 16:32:48]

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