New OOBE implementation skeleton using WUG toolkit.

New OOBE implementation skeleton using WUG toolkit.

BUG=459596
Committed: https://crrev.com/a7934fefa79ac08d3e6d8ca11814ff5d9af85381
Cr-Commit-Position: refs/heads/master@{#319245}

[Ivan Podogov, 2015-02-21 18:55:15]

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

[Nikita (slow), 2015-02-24 15:32:42]

ui/oobe/resources/oobe_ui.js: empty file?

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/oobe/oobe.cc (right):

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/oobe/oobe.cc:12: void Oobe::Register() {
nit: Add // static
comment

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/oobe/oobe.h (right):

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/oobe/oobe.h:15: explicit Oobe(Profile* profile);
Why do you need profile?

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/oobe/oobe.h:21: static void Register();
nit: Static declaration goes first.

https://codereview.chromium.org/941973003/diff/60001/tools/gritsettings/resou...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/941973003/diff/60001/tools/gritsettings/resou...
tools/gritsettings/resource_ids:219: "includes": [28310],
Might increase a bit so that it doesn't have a potential conflict with
login_resorces.

Are you intending to place login MD implementation resources in
oobe_ui_resources.grd as well?

https://codereview.chromium.org/941973003/diff/60001/ui/oobe/OWNERS
File ui/oobe/OWNERS (right):

https://codereview.chromium.org/941973003/diff/60001/ui/oobe/OWNERS#newcode6
ui/oobe/OWNERS:6: noms@chromium.org
nit: remove noms@

[Ivan Podogov, 2015-02-24 16:33:20]

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/oobe/oobe.cc (right):

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/oobe/oobe.cc:12: void Oobe::Register() {
On 2015/02/24 15:32:41, Nikita wrote:
> nit: Add // static
> comment

Done.

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/oobe/oobe.h (right):

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/oobe/oobe.h:15: explicit Oobe(Profile* profile);
On 2015/02/24 15:32:41, Nikita wrote:
> Why do you need profile?

Leftover from some old test. Removed.

https://codereview.chromium.org/941973003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/oobe/oobe.h:21: static void Register();
On 2015/02/24 15:32:41, Nikita wrote:
> nit: Static declaration goes first.

Done.

https://codereview.chromium.org/941973003/diff/60001/tools/gritsettings/resou...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/941973003/diff/60001/tools/gritsettings/resou...
tools/gritsettings/resource_ids:219: "includes": [28310],
On 2015/02/24 15:32:41, Nikita wrote:
> Might increase a bit so that it doesn't have a potential conflict with
> login_resorces.
> 
> Are you intending to place login MD implementation resources in
> oobe_ui_resources.grd as well?

We are really tight in terms of resource IDs, and login_resources only use one
ID. We discussed it with Pavel and decided it's a fair trade-off.

https://codereview.chromium.org/941973003/diff/60001/ui/oobe/OWNERS
File ui/oobe/OWNERS (right):

https://codereview.chromium.org/941973003/diff/60001/ui/oobe/OWNERS#newcode6
ui/oobe/OWNERS:6: noms@chromium.org
On 2015/02/24 15:32:41, Nikita wrote:
> nit: remove noms@

Done.

[blundell, 2015-02-25 09:46:07]

What in this CL were you looking for me to review?

[Nikita (slow), 2015-02-25 11:51:19]

lgtm

https://codereview.chromium.org/941973003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/oobe/oobe.h (right):

https://codereview.chromium.org/941973003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/oobe/oobe.h:11: class Oobe : public gen::OobeViewModel {
nit: Class comment is missing.

[dzhioev (left Google), 2015-02-25 16:55:33]

dzhioev@chromium.org changed reviewers:
- blundell@chromium.org

[Nikita (slow), 2015-03-03 12:55:04]

https://codereview.chromium.org/941973003/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/oobe/oobe.h (right):

https://codereview.chromium.org/941973003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/oobe/oobe.h:11: class Oobe : public gen::OobeViewModel {
nit: namespace is missing

[Ivan Podogov, 2015-03-03 16:44:31]

ginkage@chromium.org changed reviewers:
+ jhawkins@chromium.org

[Ivan Podogov, 2015-03-03 16:48:32]

jhawkins@, need your review for chrome/BUILD.gn and
chrome/browser/browser_resources.grd

[dzhioev (left Google), 2015-03-03 16:48:54]

https://codereview.chromium.org/941973003/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/oobe/oobe.h (right):

https://codereview.chromium.org/941973003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/oobe/oobe.h:11: class Oobe : public gen::OobeViewModel {
On 2015/03/03 12:55:04, Nikita wrote:
> nit: namespace is missing

Done.

[dzhioev (left Google), 2015-03-03 16:49:13]

LGTM

[James Hawkins, 2015-03-03 16:53:05]

lgtm

[dzhioev (left Google), 2015-03-03 17:05:37]

The CQ bit was checked by dzhioev@chromium.org

[dzhioev (left Google), 2015-03-03 17:05:38]

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/941973003/#ps180001
(title: "Rebase, repatch.")

[commit-bot: I haz the power, 2015-03-03 17:06:42]

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

[commit-bot: I haz the power, 2015-03-03 17:28:52]

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

[commit-bot: I haz the power, 2015-03-03 17:28:54]

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

[Ivan Podogov, 2015-03-04 07:52:58]

ginkage@chromium.org changed reviewers:
+ avi@chromium.org, thakis@chromium.org

[Ivan Podogov, 2015-03-04 07:55:18]

Added DEPS needs review from:
thakis@ for +base/strings
avi@ for +content/public/browser

[Ivan Podogov, 2015-03-04 14:46:09]

ginkage@chromium.org changed reviewers:
+ sadrul@chromium.org

[Ivan Podogov, 2015-03-04 14:46:40]

sadrul@, please review /ui.

[Avi (use Gerrit), 2015-03-04 16:20:30]

On 2015/03/04 07:55:18, Ivan Podogov wrote:
> Added DEPS needs review from:
> thakis@ for +base/strings
> avi@ for +content/public/browser

I guess? You are adding the new module so you know what the dependencies should
be and how it lives in the dependency graph.

LGTM

[Nico, 2015-03-04 18:22:29]

rs-lgtm

[sadrul, 2015-03-05 10:03:25]

lgtm

https://codereview.chromium.org/941973003/diff/180001/ui/oobe/oobe_md_ui.h
File ui/oobe/oobe_md_ui.h (right):

https://codereview.chromium.org/941973003/diff/180001/ui/oobe/oobe_md_ui.h#ne...
ui/oobe/oobe_md_ui.h:23: explicit OobeMdUI(content::WebUI* web_ui, const
std::string& host);
no explicit

[Ivan Podogov, 2015-03-05 10:11:47]

The CQ bit was checked by ginkage@chromium.org

[Ivan Podogov, 2015-03-05 10:11:48]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
nkostylev@chromium.org, sadrul@chromium.org, thakis@chromium.org,
dzhioev@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/941973003/#ps200001
(title: "No explicit, gn format.")

[commit-bot: I haz the power, 2015-03-05 10:12:53]

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

[commit-bot: I haz the power, 2015-03-05 11:17:34]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-05 11:18:18]

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

[Lei Zhang, 2015-03-05 19:05:38]

On 2015/03/05 11:18:18, I haz the power (commit-bot) wrote:
> Patchset 11 (id:??) landed as
> https://crrev.com/a7934fefa79ac08d3e6d8ca11814ff5d9af85381
> Cr-Commit-Position: refs/heads/master@{#319245}

Unfortunately we have CroS Builder valgrind failing after this CL, so I'm going
to manually revert this.

http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20Buil...

gyp: Key 'export_h' repeated at level 2 with key path 'variables' while reading
/b/build/slave/chromium-rel-chromeos-valgrind-builder/build/src/components/webui_generator/generator/wug.gypi
while reading includes of
/b/build/slave/chromium-rel-chromeos-valgrind-builder/build/src/ui/oobe/oobe.gyp

The bot sets GYP_DEFINES=build_for_tool=memcheck chromeos=1

[Lei Zhang, 2015-03-05 19:09:58]

On 2015/03/05 19:05:38, Lei Zhang wrote:
> On 2015/03/05 11:18:18, I haz the power (commit-bot) wrote:
> > Patchset 11 (id:??) landed as
> > https://crrev.com/a7934fefa79ac08d3e6d8ca11814ff5d9af85381
> > Cr-Commit-Position: refs/heads/master@{#319245}
> 
> Unfortunately we have CroS Builder valgrind failing after this CL, so I'm
going
> to manually revert this.
> 
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20Buil...
> 
> gyp: Key 'export_h' repeated at level 2 with key path 'variables' while
reading
>
/b/build/slave/chromium-rel-chromeos-valgrind-builder/build/src/components/webui_generator/generator/wug.gypi
> while reading includes of
>
/b/build/slave/chromium-rel-chromeos-valgrind-builder/build/src/ui/oobe/oobe.gyp
> 
> The bot sets GYP_DEFINES=build_for_tool=memcheck chromeos=1

But it may just be a typo in wug.gypi. Let me see if I can just do a 1 line
fix...