Mojo-ifying chrome://version.

Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=566163

[dpapad, 2015-12-03 00:42:56]

Description was changed from

==========
Mojo-ifying chrome://version, WIP.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos (merging functionality to version_handler).
 - Modifying version_handler to use MOJO for communicating with JS.
 - Updating JS as necessary.
 - Updating *.gn and *.gyp,gypi files as necessary.

BUG=
==========

[dpapad, 2015-12-03 19:59:28]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos (merging functionality to version_handler).
 - Modifying version_handler to use MOJO for communicating with JS.
 - Updating JS as necessary.
 - Updating *.gn and *.gyp,gypi files as necessary.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating JS as necessary.
 - Updating *.gn and *.gyp,gypi files as necessary.

BUG=
==========

[dpapad, 2015-12-03 21:51:29]

Patchset #8 (id:140001) has been deleted

[dpapad, 2015-12-03 21:51:35]

Patchset #7 (id:120001) has been deleted

[dpapad, 2015-12-03 21:51:48]

Patchset #7 (id:160001) has been deleted

[dpapad, 2015-12-03 22:18:27]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating JS as necessary.
 - Updating *.gn and *.gyp,gypi files as necessary.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating *.gn and *.gyp,gypi files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used.
 - about_version.js is serving all other platforms, and is Mojo-ified as
   well.

BUG=
==========

[dpapad, 2015-12-03 22:18:52]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating *.gn and *.gyp,gypi files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used.
 - about_version.js is serving all other platforms, and is Mojo-ified as
   well.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating *.gn and *.gyp,gypi files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used and using that from IOS.
 - about_version.js is serving all other platforms, and is Mojo-ified as
   well.

BUG=
==========

[dpapad, 2015-12-03 22:19:28]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating *.gn and *.gyp,gypi files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used and using that from IOS.
 - about_version.js is serving all other platforms, and is Mojo-ified as
   well.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used and using that from IOS.
 - about_version.js is serving all other platforms, and is Mojo-ified as
   well.

BUG=
==========

[dpapad, 2015-12-03 22:44:36]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used and using that from IOS.
 - about_version.js is serving all other platforms, and is Mojo-ified as
   well.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=
==========

[dpapad, 2015-12-04 03:08:14]

dpapad@chromium.org changed reviewers:
+ sky@chromium.org

[dpapad, 2015-12-04 03:08:15]

@sky: Feel free to suggest any reviewers that should be included.

FYI, there is a unit_test that is still failing (ActiveTabTest.ChromeUrlGrants),
because initializing a MojoWebUIController in the context of a unit test crashes
(investigating). Also see CL description for more information.

[sky, 2015-12-04 16:35:52]

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:84: mojo::Array<mojo::String>
file_paths(0);
AFAICT the results for the file path are always two strings. Bundling two string
into an array is error prone. Is there a reason you didn't make the callback
take the two paths?

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:96: Unretained(this),
Why is it safe to use unretained here and other places you're changing? I don't
think this object is guaranteed to outlive the requests. I think you should use
a weakptr.

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (right):

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:24: explicit VersionHandler(
nit: no explicit

[sky, 2015-12-04 16:36:44]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=
==========

[sky, 2015-12-04 16:36:44]

sky@chromium.org changed reviewers:
+ estade@chromium.org

[sky, 2015-12-04 16:36:56]

+estade as I'm not an owner of components/version_ui

[dpapad, 2015-12-04 18:13:15]

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:84: mojo::Array<mojo::String>
file_paths(0);
On 2015/12/04 at 16:35:52, sky wrote:
> AFAICT the results for the file path are always two strings. Bundling two
string into an array is error prone. Is there a reason you didn't make the
callback take the two paths?

Done. Passing two parameters to the callback sounds much better.

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:96: Unretained(this),
On 2015/12/04 at 16:35:52, sky wrote:
> Why is it safe to use unretained here and other places you're changing? I
don't think this object is guaranteed to outlive the requests. I think you
should use a weakptr.

Done. That was a misunderstanding on my part of Unretained semantics.

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (right):

https://codereview.chromium.org/1486403002/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:24: explicit VersionHandler(
On 2015/12/04 at 16:35:52, sky wrote:
> nit: no explicit

Done.

[Evan Stade, 2015-12-04 19:50:26]

estade@chromium.org changed reviewers:
+ dbeam@chromium.org

[Evan Stade, 2015-12-04 19:50:28]

why is there no bug filed for this? I have very little context about this
change. Dan is probably a better reviewer.

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
components/version_ui/resources/about_version.js:64: if (cr.isChromeOS) {
why does this need to be guarded with this conditional?

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
File components/version_ui/version_handler_helper.h (right):

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
components/version_ui/version_handler_helper.h:18:
scoped_ptr<std::vector<std::string>> GetVariations();
I think C++11 move semantics means it's ok to just return std::vector

[dpapad, 2015-12-04 21:26:59]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=566163
==========

[dpapad, 2015-12-04 22:01:51]

@estade: Filed a bug which provides the context for this change.

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
components/version_ui/resources/about_version.js:64: if (cr.isChromeOS) {
On 2015/12/04 at 19:50:28, Evan Stade wrote:
> why does this need to be guarded with this conditional?

Changed it so that this is obsolete (inlined the methods in the promise
callback). It did not have to be guarded. I just did because this method was not
called when not in ChromeOS.

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
File components/version_ui/version_handler_helper.h (right):

https://codereview.chromium.org/1486403002/diff/260001/components/version_ui/...
components/version_ui/version_handler_helper.h:18:
scoped_ptr<std::vector<std::string>> GetVariations();
On 2015/12/04 at 19:50:28, Evan Stade wrote:
> I think C++11 move semantics means it's ok to just return std::vector

Done. Thanks, I really like this feature of C++ 11.

[Evan Stade, 2015-12-04 22:30:20]

thanks for the doc. This looks cool.

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:74: AsWeakPtr(),
why this change? AFAIK AsWeakPtr is used when external classes need a weak
reference, and a private factory is used when only |this| should have access (as
is the case here, I believe).

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
components/version_ui/resources/about_version.js:7:
'chrome/browser/ui/webui/version.mojom',
why is this not version.mojom.js (with the same change in version_ui.cc)?

mojo/public/js/constants.cc has a similar naming convention, but since these are
all js files it seems clearer to retain the .js suffix, especially as there's a
version.mojom file which this is NOT loading.

Perhaps Scott can comment if this is a general mojo pattern.

[dpapad, 2015-12-04 23:28:14]

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:74: AsWeakPtr(),
On 2015/12/04 at 22:30:20, Evan Stade wrote:
> why this change? AFAIK AsWeakPtr is used when external classes need a weak
reference, and a private factory is used when only |this| should have access (as
is the case here, I believe).

My understanding is that inheriting from base::SupportsWeakPtr and using
AsWeakPtr() is preferred because it requires less boilerplate (than having a
private weak_ptr_factory_), and that other than that those two patterns are
interchangeable (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).

I am fine with undoing this change, but it would be nice to know if my
understanding is invalid for future reference.

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
components/version_ui/resources/about_version.js:7:
'chrome/browser/ui/webui/version.mojom',
On 2015/12/04 at 22:30:20, Evan Stade wrote:
> why is this not version.mojom.js (with the same change in version_ui.cc)?
> 
> mojo/public/js/constants.cc has a similar naming convention, but since these
are all js files it seems clearer to retain the .js suffix, especially as
there's a version.mojom file which this is NOT loading.
> 
> Perhaps Scott can comment if this is a general mojo pattern.

I was just following the pattern, see the other two places were Mojo is used in
WebUI
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

I agree that it would be clearer to retain the '.js' suffix and I am fine
changing this if we don't mind about breaking the (emerging) pattern (I would
prefer not to edit site_engagement and omnibox in this CL to keep it smaller).

[Evan Stade, 2015-12-04 23:32:26]

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
components/version_ui/resources/about_version.js:7:
'chrome/browser/ui/webui/version.mojom',
On 2015/12/04 23:28:14, dpapad wrote:
> On 2015/12/04 at 22:30:20, Evan Stade wrote:
> > why is this not version.mojom.js (with the same change in version_ui.cc)?
> > 
> > mojo/public/js/constants.cc has a similar naming convention, but since these
> are all js files it seems clearer to retain the .js suffix, especially as
> there's a version.mojom file which this is NOT loading.
> > 
> > Perhaps Scott can comment if this is a general mojo pattern.
> 
> I was just following the pattern, see the other two places were Mojo is used
in
> WebUI
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
> 
> I agree that it would be clearer to retain the '.js' suffix and I am fine
> changing this if we don't mind about breaking the (emerging) pattern (I would
> prefer not to edit site_engagement and omnibox in this CL to keep it smaller).

now seems like a great time to set the right precedent

[Dan Beam, 2015-12-04 23:32:55]

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:74: AsWeakPtr(),
On 2015/12/04 23:28:13, dpapad wrote:
> On 2015/12/04 at 22:30:20, Evan Stade wrote:
> > why this change? AFAIK AsWeakPtr is used when external classes need a weak
> reference, and a private factory is used when only |this| should have access
(as
> is the case here, I believe).
> 
> My understanding is that inheriting from base::SupportsWeakPtr and using
> AsWeakPtr() is preferred because it requires less boilerplate (than having a
> private weak_ptr_factory_), and that other than that those two patterns are
> interchangeable (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
> 
> I am fine with undoing this change, but it would be nice to know if my
> understanding is invalid for future reference.

WeakPtrFactory is safer
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p...

[dpapad, 2015-12-04 23:42:26]

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:74: AsWeakPtr(),
On 2015/12/04 at 23:32:55, Dan Beam wrote:
> On 2015/12/04 23:28:13, dpapad wrote:
> > On 2015/12/04 at 22:30:20, Evan Stade wrote:
> > > why this change? AFAIK AsWeakPtr is used when external classes need a weak
> > reference, and a private factory is used when only |this| should have access
(as
> > is the case here, I believe).
> > 
> > My understanding is that inheriting from base::SupportsWeakPtr and using
> > AsWeakPtr() is preferred because it requires less boilerplate (than having a
> > private weak_ptr_factory_), and that other than that those two patterns are
> > interchangeable (see
> >
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
> > 
> > I am fine with undoing this change, but it would be nice to know if my
> > understanding is invalid for future reference.
> 
> WeakPtrFactory is safer
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p...

Reverted. Thanks for the link.

[sky, 2015-12-04 23:58:39]

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
components/version_ui/resources/about_version.js:7:
'chrome/browser/ui/webui/version.mojom',
On 2015/12/04 23:32:26, Evan Stade wrote:
> On 2015/12/04 23:28:14, dpapad wrote:
> > On 2015/12/04 at 22:30:20, Evan Stade wrote:
> > > why is this not version.mojom.js (with the same change in version_ui.cc)?
> > > 
> > > mojo/public/js/constants.cc has a similar naming convention, but since
these
> > are all js files it seems clearer to retain the .js suffix, especially as
> > there's a version.mojom file which this is NOT loading.
> > > 
> > > Perhaps Scott can comment if this is a general mojo pattern.
> > 
> > I was just following the pattern, see the other two places were Mojo is used
> in
> > WebUI
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > and
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
> > 
> > I agree that it would be clearer to retain the '.js' suffix and I am fine
> > changing this if we don't mind about breaking the (emerging) pattern (I
would
> > prefer not to edit site_engagement and omnibox in this CL to keep it
smaller).
> 
> now seems like a great time to set the right precedent

That's the id of the module, which need not match the file name. It was decided
to match the file name the mojom comes from and not the on disk file. I didn't
come up with the naming. I think aa or abarth likely came up with the naming. I
don't feel strongly that it should be .mojom vs .mojom.js. I suggest moving
naming discussion to a separate thread else you'll derail this patch.

[sky, 2015-12-05 00:06:59]

sky@chromium.org changed reviewers:
+ nasko@chromium.org

[sky, 2015-12-05 00:07:00]

LGTM - I believe we are also doing security reviews on new mojom files that are
exposed to the renderer. So, +nasko for that.

https://codereview.chromium.org/1486403002/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version.mojom (right):

https://codereview.chromium.org/1486403002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version.mojom:6: GetFilePaths() => (string execPath,
string profilePath);
nit: generally we go with c++ style for names in mojoms, so exec_path.

[Evan Stade, 2015-12-05 00:07:25]

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/280001/components/version_ui/...
components/version_ui/resources/about_version.js:7:
'chrome/browser/ui/webui/version.mojom',
On 2015/12/04 23:58:39, sky wrote:
> On 2015/12/04 23:32:26, Evan Stade wrote:
> > On 2015/12/04 23:28:14, dpapad wrote:
> > > On 2015/12/04 at 22:30:20, Evan Stade wrote:
> > > > why is this not version.mojom.js (with the same change in
version_ui.cc)?
> > > > 
> > > > mojo/public/js/constants.cc has a similar naming convention, but since
> these
> > > are all js files it seems clearer to retain the .js suffix, especially as
> > > there's a version.mojom file which this is NOT loading.
> > > > 
> > > > Perhaps Scott can comment if this is a general mojo pattern.
> > > 
> > > I was just following the pattern, see the other two places were Mojo is
used
> > in
> > > WebUI
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > > and
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....
> > > 
> > > I agree that it would be clearer to retain the '.js' suffix and I am fine
> > > changing this if we don't mind about breaking the (emerging) pattern (I
> would
> > > prefer not to edit site_engagement and omnibox in this CL to keep it
> smaller).
> > 
> > now seems like a great time to set the right precedent
> 
> That's the id of the module, which need not match the file name. It was
decided
> to match the file name the mojom comes from and not the on disk file. I didn't
> come up with the naming. I think aa or abarth likely came up with the naming.
I
> don't feel strongly that it should be .mojom vs .mojom.js. I suggest moving
> naming discussion to a separate thread else you'll derail this patch.

OK, I guess there's more precedent for this than I initially thought: 

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/comp...

I have no problem going with the flow here.

[dpapad, 2015-12-07 22:42:17]

dpapad@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[dpapad, 2015-12-07 22:42:18]

https://codereview.chromium.org/1486403002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/1486403002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/active_tab_unittest.cc:364: GURL
internal("chrome://about");
@rdevlin.cronin:Is this fix OK for this test?
Context: I am converting chrome://version to use Mojo in this CL, and
instantiating a MojoWebUIController fails in the context of a unit test (error
pasted below).
#0 0x7f1ab8bd90bb base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7f1ab29a7340 <unknown>
#2 0x000002a6a5d3 content::ServiceRegistry::AddService<>()
#3 0x000002a6a59a MojoWebUIController<>::RenderViewCreated()
#4 0x7f1ab735dc24 content::RenderFrameHostManager::UpdateStateForNavigate()
#5 0x7f1ab735d71e content::RenderFrameHostManager::Navigate()
#6 0x7f1ab734ee50 content::NavigatorImpl::NavigateToEntry()

If the exact chrome URL is not relevant here, this change should be OK.

@sky, rdevlin.cronin: Eventually this case will come up again (as more WebUI is
converted to use Mojo), so a better solution would be needed. I see an example
usage of TestChromeWebUIControllerFactory at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/w...,
is this the recommended way?

[Devlin, 2015-12-07 22:47:52]

https://codereview.chromium.org/1486403002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/1486403002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/active_tab_unittest.cc:364: GURL
internal("chrome://about");
On 2015/12/07 22:42:18, dpapad wrote:
> @rdevlin.cronin:Is this fix OK for this test?
> Context: I am converting chrome://version to use Mojo in this CL, and
> instantiating a MojoWebUIController fails in the context of a unit test (error
> pasted below).
> #0 0x7f1ab8bd90bb base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #1 0x7f1ab29a7340 <unknown>
> #2 0x000002a6a5d3 content::ServiceRegistry::AddService<>()
> #3 0x000002a6a59a MojoWebUIController<>::RenderViewCreated()
> #4 0x7f1ab735dc24 content::RenderFrameHostManager::UpdateStateForNavigate()
> #5 0x7f1ab735d71e content::RenderFrameHostManager::Navigate()
> #6 0x7f1ab734ee50 content::NavigatorImpl::NavigateToEntry()
> 
> If the exact chrome URL is not relevant here, this change should be OK.
> 
> @sky, rdevlin.cronin: Eventually this case will come up again (as more WebUI
is
> converted to use Mojo), so a better solution would be needed. I see an example
> usage of TestChromeWebUIControllerFactory at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/w...,
> is this the recommended way?

As long as the test passes, this url should be fine.

I'm not particularly familiar with mojo, so I'll let Sky answer the second part.

[sky, 2015-12-08 00:19:56]

https://codereview.chromium.org/1486403002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/1486403002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/active_tab_unittest.cc:364: GURL
internal("chrome://about");
On 2015/12/07 22:42:18, dpapad wrote:
> @rdevlin.cronin:Is this fix OK for this test?
> Context: I am converting chrome://version to use Mojo in this CL, and
> instantiating a MojoWebUIController fails in the context of a unit test (error
> pasted below).
> #0 0x7f1ab8bd90bb base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #1 0x7f1ab29a7340 <unknown>
> #2 0x000002a6a5d3 content::ServiceRegistry::AddService<>()
> #3 0x000002a6a59a MojoWebUIController<>::RenderViewCreated()
> #4 0x7f1ab735dc24 content::RenderFrameHostManager::UpdateStateForNavigate()
> #5 0x7f1ab735d71e content::RenderFrameHostManager::Navigate()
> #6 0x7f1ab734ee50 content::NavigatorImpl::NavigateToEntry()
> 
> If the exact chrome URL is not relevant here, this change should be OK.
> 
> @sky, rdevlin.cronin: Eventually this case will come up again (as more WebUI
is
> converted to use Mojo), so a better solution would be needed. I see an example
> usage of TestChromeWebUIControllerFactory at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/w...,
> is this the recommended way?

I believe using a WebUIBrowserTest is likely much heavier than a unit test, and
susceptible to more flake. I'm not sure what the right answer is. It may be to
make ChromeRenderViewHostTestHarness work with mojo, but I'm not positive. I
recommend filing a bug on this and talking with Ken Rockot about it.

[dpapad, 2015-12-08 20:06:12]

Friendly ping.
@nasko: Please review (security) for the mojom file.
@dbeam: Please review ui/webui/*

[Dan Beam, 2015-12-09 01:39:31]

why wouldn't we keep the old separation and just make to mojo interfaces?  1 for
everything and 1 for chromeos?

[Dan Beam, 2015-12-09 01:40:18]

make 2 mojo interfaces**

[dpapad, 2015-12-09 02:10:18]

On 2015/12/09 at 01:40:18, dbeam wrote:
> make 2 mojo interfaces**

Having two interfaces will inflict more boilerplate code both on JS and C++, I
don't see a clear win from doing so. Specifically

C++:
MojoWebUIController class is templatized with a single Mojo interface (see 
chrome/browser/ui/webui/version_ui.h in this CL). This means
VersionUI would have to inherit directly from MojoWebUIControllerBase instead of
MojoWebUIController, and therefore needs to override
RenderViewCreated to register both handlers.

JS:
Two different Mojo objects would be used to communicate, one for everything and
one for ChromeOS only. Getting the reference to the
second object also adds a bit of boilerplate code.

The above would achieve avoiding having the following method, but at the cost of
adding more code.

void VersionHandler::GetOsVersion(const GetOsVersionCallback& callback) {
#if defined(OS_CHROMEOS)
  ...
#endif  // defined(OS_CHROMEOS)
}

Do you think that the extra effort would result in more readable/understandable
code? If so I am willing to try it.

[sky, 2015-12-09 17:06:18]

On 2015/12/09 02:10:18, dpapad wrote:
> On 2015/12/09 at 01:40:18, dbeam wrote:
> > make 2 mojo interfaces**
> 
> Having two interfaces will inflict more boilerplate code both on JS and C++, I
> don't see a clear win from doing so. Specifically
> 
> C++:
> MojoWebUIController class is templatized with a single Mojo interface (see 
> chrome/browser/ui/webui/version_ui.h in this CL). This means
> VersionUI would have to inherit directly from MojoWebUIControllerBase instead
of
> MojoWebUIController, and therefore needs to override
> RenderViewCreated to register both handlers.
> 
> JS:
> Two different Mojo objects would be used to communicate, one for everything
and
> one for ChromeOS only. Getting the reference to the
> second object also adds a bit of boilerplate code.
> 
> The above would achieve avoiding having the following method, but at the cost
of
> adding more code.
> 
> void VersionHandler::GetOsVersion(const GetOsVersionCallback& callback) {
> #if defined(OS_CHROMEOS)
>   ...
> #endif  // defined(OS_CHROMEOS)
> }
> 
> Do you think that the extra effort would result in more
readable/understandable
> code? If so I am willing to try it.

I responded on the other thread about a way to provide two interfaces, if that
is what we want.

For completeness I'll mention other options. AFAICT the model for the version
page is to have the page query at creation for all the strings.

If it's ok to provide all the strings at once you could do:

struct VersionData {
  string exec_path;
  string profile_path;
  string flash_version;
  array<string> variations;
  string os_version;
}

interface VersionConsumer {
  SetVersionData(VersionData data);
}

The WebUI side would get the VersionConsumer from the page and call
SetVersionData. OS specific data could be provided in two ways. Either directly
in the VersionData structure, or as a separate structure with another function
in VersionConsumer.

Another option is to do away with the structure and have specific functions on
VersionConsumer for setting the data, eg:

interface VersionConsumer {
  SetProfilePath(...);
  SetOsVersion(...);
}

Platform specific data is provided either directly in VersionConsumer, or in
another interface that is platform specific.

Lastly, you could do something like:
struct VersionData {
  string exec_path;
  string profile_path;
  string flash_version;
  array<string> variations;
  string os_version;
  platform specific data;
}

VersionHandlerMojo {
  GetVersionData() => (VersionData version_data);
}

A single interface like in this patch with all the OS specific data provided in
VersionData. I think this approach makes for nice structuring of code while
minimizing ifdefs. It has the down side of assuming you want to provide all data
at once.

Personally I think we over analyzing this and the code in the current patch
seems reasonable to me.

[Dan Beam, 2015-12-09 20:19:34]

- Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).

I'm still confused at to why this ^ part of the CL is better?

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:116: #endif  // defined(OS_CHROMEOS)
so we just never resolve the Promise() for the JS when not on ChromeOS?

[nasko, 2015-12-09 20:20:39]

I'll take a pass over this CL likely tomorrow.

[dpapad, 2015-12-09 20:34:00]

>- Removing version_handler_chromeos.cc (merging functionality to
>   version_handler.cc).

>I'm still confused at to why this ^ part of the CL is better?

The previous code has VersionHandlerChromeOS inheriting from VersionHandler and
implementing the extra method. In the new code, a FooHandler implements an
auto-generated interface, so this ties back to the conversation of one versus
two interfaces discussion. If those are separate classes, then what interface
would each be implementing? If we still kept one interface but separated those
classes, than what would be the benefit (the base class would have a no-op
method still)?

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:116: #endif  // defined(OS_CHROMEOS)
On 2015/12/09 at 20:19:34, Dan Beam wrote:
> so we just never resolve the Promise() for the JS when not on ChromeOS?

If somebody were to call this method on non-ChromeOS the Promise would never be
resolved, but we are not calling it see
components/version_ui/resources/about_version.js, where the call to
getOsVersion() is guarded.

[Dan Beam, 2015-12-09 20:38:19]

On 2015/12/09 20:34:00, dpapad wrote:
> >- Removing version_handler_chromeos.cc (merging functionality to
> >   version_handler.cc).
> 
> >I'm still confused at to why this ^ part of the CL is better?
> 
> The previous code has VersionHandlerChromeOS inheriting from VersionHandler
and
> implementing the extra method.

^ that's the bug, IMO, as it should've just been adding a handler on ChromeOS
that only gets the OS version (and doesn't derive from VersionHandler)

> In the new code, a FooHandler implements an
> auto-generated interface, so this ties back to the conversation of one versus
> two interfaces discussion. If those are separate classes, then what interface
> would each be implementing? If we still kept one interface but separated those
> classes, than what would be the benefit (the base class would have a no-op
> method still)?

[dpapad, 2015-12-09 20:45:48]

On 2015/12/09 at 20:38:19, dbeam wrote:
> On 2015/12/09 20:34:00, dpapad wrote:
> > >- Removing version_handler_chromeos.cc (merging functionality to
> > >   version_handler.cc).
> > 
> > >I'm still confused at to why this ^ part of the CL is better?
> > 
> > The previous code has VersionHandlerChromeOS inheriting from VersionHandler
and
> > implementing the extra method.
> 
> ^ that's the bug, IMO, as it should've just been adding a handler on ChromeOS
that only gets the OS version (and doesn't derive from VersionHandler)
If that is considered a bug, then this CL is better because it removes the bug
;>
Anyway regardless the previous handler setup, what is the issue with merging
those two classes, given the constraints 
I mentioned in my previous comment, and given that they only differ so little
(one extra message). 

> 
> > In the new code, a FooHandler implements an
> > auto-generated interface, so this ties back to the conversation of one
versus
> > two interfaces discussion. If those are separate classes, then what
interface
> > would each be implementing? If we still kept one interface but separated
those
> > classes, than what would be the benefit (the base class would have a no-op
> > method still)?

[Dan Beam, 2015-12-09 21:45:08]

how are we going to test this code?

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/active_tab_unittest.cc:364: GURL
internal("chrome://about");
why is this necessary?

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version.mojom (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version.mojom:7: GetFlashVersion() => (string
flashVersion);
^^ desktop-only

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version.mojom:9: GetOsVersion() => (string osVersion);
^^ chromeos-only

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version.mojom:10: };
wait, so it's actually 50% of the interface that runs on only some platforms?

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:35: base::string16* profile_path_out)
{
ident off

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:103:
callback.Run(variations_array.Pass());
can this be

callback.Run(mojo::Array<mojo::String>::From(variations).Pass());

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (left):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:36: // This also initiates the OS
Version load on ChromeOS.
that's weird

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:37: void OnGotPlugins(const
std::vector<content::WebPluginInfo>& plugins);
it's weird that this existed on iOS

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_ui.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_ui.cc:150: IDR_VERSION_MOJO_JS);
html_source->AddResourcePath("chrome/browser/ui/webui/version.mojom",
                             IDR_VERSION_MOJO_JS);

is less lines and matches L144

https://codereview.chromium.org/1486403002/diff/340001/components/resources/v...
File components/resources/version_ui_resources.grdp (right):

https://codereview.chromium.org/1486403002/diff/340001/components/resources/v...
components/resources/version_ui_resources.grdp:4: <include
name="IDR_VERSION_UI_HTML" file="../version_ui/resources/about_version.html"
flattenhtml="true" allowexternalscript="true" type="BINDATA" />
should this if <if expr="is_ios">?

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:4: 
nit: remove extra line

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:7: function getUiHandler() {
same question as below as to why this is a method when only used once?

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:31: }
why is this a method when it's only called once?  why not

var domContentLoaded = new Promise(function(resolve, reject) {
  document.addEventListener('DOMContentLoaded', resolve);
});

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:43:
uiHandler.getFlashVersion().then(function(response) {
you're calling this on Android even though flash isn't there, right?

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:51: if (cr.isChromeOS) {
this is a snapshot of the platforms that use this right now, with no
programmatic enforcement if this changes in the future.

something this would be better:

  if (uiHandler.getOsVersion)
    uiHandler.getOsVersion().then(...)

and we don't provide getOsVersion unless it's useful, because right now this
seems akin to doing:

  document.addEventListener('thingThatWillNeverHappen', function() {
  });

it's just dead code.  it's not a huge deal in this case, but I'm wondering what
precedent we're setting for things like settings where there's *TONS* of
platform-specific stuff (especially ChromeOS-specific)

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:58: main();
what's the point of main()?  are we trying to be pythonic or something (i.e. if
__name__ == '__main__':)?

(function() { ... })() would make more sense... but there are no new variables
introduced (so adding a scope isn't too helpful)

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
File components/version_ui/resources/about_version_ios.js (right):

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version_ios.js:19: function onLoadWork() {
nit: onLoad seems more like everywhere else, or just inlining the method

[Dan Beam, 2015-12-09 21:51:36]

On 2015/12/09 20:45:48, dpapad wrote:
> On 2015/12/09 at 20:38:19, dbeam wrote:
> > On 2015/12/09 20:34:00, dpapad wrote:
> > > >- Removing version_handler_chromeos.cc (merging functionality to
> > > >   version_handler.cc).
> > > 
> > > >I'm still confused at to why this ^ part of the CL is better?
> > > 
> > > The previous code has VersionHandlerChromeOS inheriting from
VersionHandler
> and
> > > implementing the extra method.
> > 
> > ^ that's the bug, IMO, as it should've just been adding a handler on
ChromeOS
> that only gets the OS version (and doesn't derive from VersionHandler)
> If that is considered a bug, then this CL is better because it removes the bug
> ;>
> Anyway regardless the previous handler setup, what is the issue with merging
> those two classes, given the constraints 
> I mentioned in my previous comment, and given that they only differ so little
> (one extra message). 

technically there are 2 platform-specific messages, yes?

typically BaseThing exists.  then BaseThing needs to do something less places
than BaseThing runs.  so you #ifdef that code.  eventually, more #ifdefs
necessitate splitting at a higher level (around the point that you have more
platform-dependent thing than platform-indepenent).

you're taking something that was split (though poorly), and re-combining it,
which seems very odd to me.

I like that we're fixing the (seemingly unnecessary) concrete derivation with
this CL, but I'm arguing that we should be doing it a higher level, and likely
making it functionality based (i.e. not VersionHandlerChromeOS, but
OSVersionHandler and/or Flash(or Plugins?)VersionHandler).

either way: you're fixing the bug so I guess I'll shut up about this if you
disagree, as it seems mainly a matter of opinion.

> 
> > 
> > > In the new code, a FooHandler implements an
> > > auto-generated interface, so this ties back to the conversation of one
> versus
> > > two interfaces discussion. If those are separate classes, then what
> interface
> > > would each be implementing? If we still kept one interface but separated
> those
> > > classes, than what would be the benefit (the base class would have a no-op
> > > method still)?

[nasko, 2015-12-09 23:39:05]

How are we ensuring that the process that is calling the new Version Mojo
interface has the right privileges (aka BINDINGS_POLICY_WEB_UI)?

[Dan Beam, 2015-12-09 23:49:55]

On 2015/12/09 23:39:05, nasko wrote:
> How are we ensuring that the process that is calling the new Version Mojo
> interface has the right privileges (aka BINDINGS_POLICY_WEB_UI)?

MojoWebUIController is a subclass of WebUIController, so presumably via
[Chrome]WebUIControllerFactory just like everyone else/as it was before:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[dpapad, 2015-12-10 00:53:58]

Patchset #16 (id:360001) has been deleted

[dpapad, 2015-12-10 01:08:28]

>how are we going to test this code?

That is the next challenge before converting more WebUI, figuring out how to
test Moj-ified code. I have not done it in this CL, and have not thought about
it yet. The well-defined interfaces that Mojo brings seem that would facilitate
testing, compared to the previously chrome.send/CallJavascript function implicit
pairs.

Planning to try this in follow up CL, mostly to explore Mojo and not so much
because I am passionate about testing chrome://version (I think manual testing
is acceptable for such an infrequently modified page).

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/active_tab_unittest.cc:364: GURL
internal("chrome://about");
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> why is this necessary?

This is already explained previously in this review. Repeating here:

@devlin.cronin already reviewed this part. The context for this change is that
instantiating a MojoWebUIController in the context of a unit_test fails. For
this particular test the exact chrome URL does not matter so changing to
chrome://about is sufficient. Eventually I will file a bug on the Mojo side to
provide a solution on how to handle such cases in the future.

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:35: base::string16* profile_path_out)
{
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> ident off

Done.

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.cc:103:
callback.Run(variations_array.Pass());
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> can this be
> 
> callback.Run(mojo::Array<mojo::String>::From(variations).Pass());

Done.

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (left):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:36: // This also initiates the OS
Version load on ChromeOS.
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> that's weird

Indeed. Removed that comment, it made no sense.

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:37: void OnGotPlugins(const
std::vector<content::WebPluginInfo>& plugins);
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> it's weird that this existed on iOS

What makes you think it existed? The IOS handler implementation is at
https://code.google.com/p/chromium/codesearch#chromium/src/ios/chrome/browser...
and it is not affected by this CL.

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_ui.cc (right):

https://codereview.chromium.org/1486403002/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_ui.cc:150: IDR_VERSION_MOJO_JS);
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> html_source->AddResourcePath("chrome/browser/ui/webui/version.mojom",
>                              IDR_VERSION_MOJO_JS);
> 
> is less lines and matches L144

Done.

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:4: 
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> nit: remove extra line

Done.

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:7: function getUiHandler() {
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> same question as below as to why this is a method when only used once?

See answer at the end of this file.

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:31: }
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> why is this a method when it's only called once?  why not
> 
> var domContentLoaded = new Promise(function(resolve, reject) {
>   document.addEventListener('DOMContentLoaded', resolve);
> });

See answer at the end of this file.

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:43:
uiHandler.getFlashVersion().then(function(response) {
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> you're calling this on Android even though flash isn't there, right?

Fixed.

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:51: if (cr.isChromeOS) {
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> this is a snapshot of the platforms that use this right now, with no
programmatic enforcement if this changes in the future.
> 
> something this would be better:
> 
>   if (uiHandler.getOsVersion)
>     uiHandler.getOsVersion().then(...)
> 
> and we don't provide getOsVersion unless it's useful, because right now this
seems akin to doing:
> 
>   document.addEventListener('thingThatWillNeverHappen', function() {
>   });
> 
> it's just dead code.  it's not a huge deal in this case, but I'm wondering
what precedent we're setting for things like settings where there's *TONS* of
platform-specific stuff (especially ChromeOS-specific)

I don't think that this should be the precedent for cases with TONS of platform
specific stuff, in which case the multiple interfaces should be preferred. Also
regarding this being dead-code, yes it is, as it was before (see all the code
that is removed from about_version_ios.js for example). At least this makes the
fact that it is dead-code for some cases very explicit.

Also, just for completeness, chrome://version does have a lot of platform
specific stuff, but it is mostly handled on the C++ side while populating
loadTimeData. If we decide to convert all loadTimeData for chrome://version to
use Mojo, then I would be in favor of breaking down to multiple interfaces at
that time.

https://codereview.chromium.org/1486403002/diff/340001/components/version_ui/...
components/version_ui/resources/about_version.js:58: main();
On 2015/12/09 at 21:45:07, Dan Beam wrote:
> what's the point of main()?  are we trying to be pythonic or something (i.e.
if __name__ == '__main__':)?
> 
> (function() { ... })() would make more sense... but there are no new variables
introduced (so adding a scope isn't too helpful)

The point of this is just readability. It makes it painfully obvious that main()
is the starting point of this script. This is the same reason why I prefer to
have getUIHandler, whenDomContentLoaded as a method instead of turning them to
Promise variables as suggested. The reader can start reading from the first line
of main() knowing that no code has executed before that, makes it easier to
follow IMO.

Also the question "why is X a function if it is only called once?" is not a
golden criterion for the existence of a function. Sometimes methods/functions
exist because they encapsulate/abstract some operation, which is the case here,
not because they are reused multiple times.

I generally don't find such comments useful. From my point of view these
comments fall into the category "why is this piece of code not exactly as I (the
reviewer) would have written it myself?". Authoring code is like hand-writing
style, no two people have the exact same style.
What matters most is whether you had hard time understanding the code, whether
it violates some styleguide or a profound software engineering good practice and
whether it does not do what it advertises of doing.

Feel free to grab me in person and talk about this further if you like, I don't
think the CL is the best place to have such a discussion.

[sky, 2015-12-10 16:20:34]

https://codereview.chromium.org/1486403002/diff/380001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (right):

https://codereview.chromium.org/1486403002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:49:
mojo::StrongBinding<VersionHandlerMojo> binding_;
Normally MojoWebUIHandler is owned by content. StrongBinding means if the pipe
goes away the implemention of VersionHandlerMojo (in this case it is 'this')
would be deleted. So, use a Binding here.

[dpapad, 2015-12-10 18:40:19]

https://codereview.chromium.org/1486403002/diff/380001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (right):

https://codereview.chromium.org/1486403002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:49:
mojo::StrongBinding<VersionHandlerMojo> binding_;
On 2015/12/10 at 16:20:34, sky wrote:
> Normally MojoWebUIHandler is owned by content. StrongBinding means if the pipe
goes away the implemention of VersionHandlerMojo (in this case it is 'this')
would be deleted. So, use a Binding here.

Done.

[Eugene But (OOO till 7-30), 2016-06-01 23:31:58]

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

[Eugene But (OOO till 7-30), 2016-06-01 23:32:00]

Thanks! This works on iOS after adding a missing include. Mojo on iOS is not
enabled by default yet, and I will let you know when it happen (basically when
this lands: https://codereview.chromium.org/2027733003/)

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
File components/version_ui/resources/about_version_ios.js (right):

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
components/version_ui/resources/about_version_ios.js:20:
chrome.send('requestVersionInfo');
Is there a reason why ios is different from other platforms?

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
File components/version_ui/version_ui_constants.h (right):

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
components/version_ui/version_ui_constants.h:7: 
Needs |#include "build/build_config.h"|

Otherwise |OS_IOS| will not be defined.

[dpapad, 2016-06-01 23:44:11]

Does it work on IOS using about_version.js?

As explained in the comment, about_version_ios.js was introduced because IOS did
not support Mojo at the time of writing this CL, so if you simply patch this CL
and run on IOS it would not actually exercise the Mojo version.

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
File components/version_ui/resources/about_version_ios.js (right):

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
components/version_ui/resources/about_version_ios.js:20:
chrome.send('requestVersionInfo');
On 2016/06/01 at 23:32:00, Eugene But wrote:
> Is there a reason why ios is different from other platforms?

The only reason is that at the time I authored this CL initially, I was planning
to actually land it, without breaking IOS. So introducing different JS files for
IOS and for other platforms was the way to go. If IOS Mojo works, then we can
remove this part of the CL and merge the JS files again.

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
File components/version_ui/version_ui_constants.h (right):

https://codereview.chromium.org/1486403002/diff/440001/components/version_ui/...
components/version_ui/version_ui_constants.h:7: 
On 2016/06/01 at 23:32:00, Eugene But wrote:
> Needs |#include "build/build_config.h"|
> 
> Otherwise |OS_IOS| will not be defined.

Done.

[Eugene But (OOO till 7-30), 2016-06-02 22:23:12]

I'm the process of adding all source code and resources to iOS target when I can
run it. For now just a minor comment regarding |define| usage...

https://codereview.chromium.org/1486403002/diff/460001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version.mojom (right):

https://codereview.chromium.org/1486403002/diff/460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version.mojom:4: 
iOS can not use chrome. So we'll have to put these into components.

https://codereview.chromium.org/1486403002/diff/460001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/460001/components/version_ui/...
components/version_ui/resources/about_version.js:8: define([
define('main', [

Otherwise this does not work on iOS.

[Eugene But (OOO till 7-30), 2016-06-04 01:15:25]

Mojo version works on iOSwith a few tweaks I mentioned in the comments. iOS CL
is here: https://codereview.chromium.org/2041603002/

I will follow up on email regarding possible next steps. Thanks!

https://codereview.chromium.org/1486403002/diff/460001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (right):

https://codereview.chromium.org/1486403002/diff/460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:9: 
This file can be shared with iOS. Not much (only inheritance from
mojom::VersionPageHandler and GetVariations implementation) but still. Should we
componetize this file and mojo_web_ui_handler.h?

https://codereview.chromium.org/1486403002/diff/460001/components/version_ui/...
File components/version_ui/resources/about_version.js (right):

https://codereview.chromium.org/1486403002/diff/460001/components/version_ui/...
components/version_ui/resources/about_version.js:36:
uiHandler.getFilePaths().then(function(response) {
This should not be called for iOS

https://codereview.chromium.org/1486403002/diff/460001/components/version_ui/...
components/version_ui/resources/about_version.js:46: if (!cr.isAndroid) {
ditto

[Eugene But (OOO till 7-30), 2016-06-06 17:20:54]

https://codereview.chromium.org/1486403002/diff/460001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/version_handler.h (right):

https://codereview.chromium.org/1486403002/diff/460001/chrome/browser/ui/webu...
chrome/browser/ui/webui/version_handler.h:9: 
On 2016/06/04 01:15:25, Eugene But wrote:
> This file can be shared with iOS. Not much (only inheritance from
> mojom::VersionPageHandler and GetVariations implementation) but still. Should
we
> componetize this file and mojo_web_ui_handler.h?
Sorry, I was wrong. This file uses content// which can not be used on iOS. Since
sharing the code will not be trivial, please ignore my comment.

[dpapad, 2016-07-20 17:50:09]

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

[commit-bot: I haz the power, 2016-07-20 17:50:59]

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-07-20 18:00:51]

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

[commit-bot: I haz the power, 2016-07-20 18:00:53]

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

[dpapad, 2016-07-20 21:51:46]

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

[commit-bot: I haz the power, 2016-07-20 21:52:26]

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-07-20 22:07:20]

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

[commit-bot: I haz the power, 2016-07-20 22:07:21]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dpapad, 2016-07-21 22:51:36]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=566163
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality (temporarily).
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

IOS code is updated to use Mojo at https://codereview.chromium.org/2041603002.

BUG=566163
==========

[dpapad, 2016-07-21 22:51:49]

Description was changed from

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality (temporarily).
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

IOS code is updated to use Mojo at https://codereview.chromium.org/2041603002.

BUG=566163
==========

to

==========
Mojo-ifying chrome://version.

 - Adding version.mojom interface file.
 - Removing version_handler_chromeos.cc (merging functionality to
   version_handler.cc).
 - Modifying version_handler.cc to use MOJO for communicating with JS.
 - Updating gn,gyp,gypi and grd files as necessary.

Preserving IOS functionality.
 - Adding about_version_ios.js which only includes the parts that are
   actually used by IOS.
 - about_version.js is Mojo-ified and serves all platforms except IOS.

BUG=566163
==========

[dpapad, 2016-07-21 22:51:49]

dpapad@chromium.org changed reviewers:
- estade@chromium.org

[dpapad, 2016-07-21 22:58:33]

This CL is now revived, PTAL.

When this CL was originally authored (quite a few months ago) Mojo was not
supported on IOS, but since then this has been fixed (crbug.com/567809). This CL
had been put on hold because it was forking the code (about_version.js) for IOS
and non-IOS platforms, which was not desirable.

The difference this time is that even though this CL is still forking the code
(see about_version.js and about_version_ios.js), it is only temporary so that
@eugenebut can follow up with https://codereview.chromium.org/2041603002 which
unforks the code and hooks up IOS to use Mojo.

Thank you

[dpapad, 2016-07-21 22:59:45]

On 2016/07/21 at 22:58:33, dpapad wrote:
> This CL is now revived, PTAL.
> 
> When this CL was originally authored (quite a few months ago) Mojo was not
supported on IOS, but since then this has been fixed (crbug.com/567809). This CL
had been put on hold because it was forking the code (about_version.js) for IOS
and non-IOS platforms, which was not desirable.
> 
> The difference this time is that even though this CL is still forking the code
(see about_version.js and about_version_ios.js), it is only temporary so that
@eugenebut can follow up with https://codereview.chromium.org/2041603002 which
unforks the code and hooks up IOS to use Mojo.
> 
> Thank you

Forgot to mention that I am still investigating a compilation error (pasted
below) happening only on a subset of the Windows bots, any help is appreciated.

IOError: [Errno 2] No such file or directory:
u'..\\..\\out\\Debug\\gen\\components\\version_ui\\version.mojom.js'

[sky, 2016-07-21 23:04:14]

That usually indicates deps aren't correct. That is, someone is trying
to use version.mojom.js before it's been generated.

On Thu, Jul 21, 2016 at 3:59 PM, dpapad@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
wrote:
> On 2016/07/21 at 22:58:33, dpapad wrote:
>> This CL is now revived, PTAL.
>>
>> When this CL was originally authored (quite a few months ago) Mojo was not
> supported on IOS, but since then this has been fixed (crbug.com/567809).
> This CL
> had been put on hold because it was forking the code (about_version.js) for
> IOS
> and non-IOS platforms, which was not desirable.
>>
>> The difference this time is that even though this CL is still forking the
>> code
> (see about_version.js and about_version_ios.js), it is only temporary so
> that
> @eugenebut can follow up with https://codereview.chromium.org/2041603002
> which
> unforks the code and hooks up IOS to use Mojo.
>>
>> Thank you
>
> Forgot to mention that I am still investigating a compilation error (pasted
> below) happening only on a subset of the Windows bots, any help is
> appreciated.
>
> IOError: [Errno 2] No such file or directory:
> u'..\\..\\out\\Debug\\gen\\components\\version_ui\\version.mojom.js'
>
>
> https://codereview.chromium.org/1486403002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dpapad, 2016-07-21 23:20:35]

On 2016/07/21 at 23:04:14, sky wrote:
> That usually indicates deps aren't correct. That is, someone is trying
> to use version.mojom.js before it's been generated.

I am suspecting that it could have been a forward VS backward slash issue (see
patch 28).

[dpapad, 2016-07-21 23:20:48]

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

[commit-bot: I haz the power, 2016-07-21 23:21:28]

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-07-21 23:33:02]

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

[commit-bot: I haz the power, 2016-07-21 23:33:03]

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

[Eugene But (OOO till 7-30), 2016-12-27 23:42:07]

On 2016/07/21 23:20:35, dpapad wrote:
> On 2016/07/21 at 23:04:14, sky wrote:
> > That usually indicates deps aren't correct. That is, someone is trying
> > to use version.mojom.js before it's been generated.
> 
> I am suspecting that it could have been a forward VS backward slash issue (see
> patch 28).
Demetrios, do you still plan to Mojo-ify chrome://version?

[dpapad, 2017-01-05 21:58:38]

On 2016/12/27 at 23:42:07, eugenebut wrote:
> On 2016/07/21 23:20:35, dpapad wrote:
> > On 2016/07/21 at 23:04:14, sky wrote:
> > > That usually indicates deps aren't correct. That is, someone is trying
> > > to use version.mojom.js before it's been generated.
> > 
> > I am suspecting that it could have been a forward VS backward slash issue
(see
> > patch 28).
> Demetrios, do you still plan to Mojo-ify chrome://version?

This has been sitting on the backburner for a while, mostly because of me
focusing on the MD Settings effort (higher priority), but also was waiting on
some Mojo JS improvements (after communicating with the Mojo team). I'll look
into how much work it is to rebase that CL and attempt to get it reviewed and
landed before any Mojo JS improvements actually happen, but can't focus on this
until MD Settings is launched.