Turn on component updater on chromeos, only for the pnacl component.

Turn on component updater on chromeos, only for the pnacl component.

Make the pnacl component place files in a profile-specific directory
for chromeos.  We don't want to share data between users on
chromeos because of questions about integrity of these files.
This adds a notification watcher and will re-register pnacl with
a different version whenever the user logs into
a different account.

Files are installed to a stateful partition, which should
be okay.  The stateful partition is mounted no-exec, but
the pnacl files are not typical executables.

This is a temporary solution to help with testing out PNaCl on chromeos.
We will need to find a different solution later that does
not involve multiple copies, one for each user.

BUG=221381
TEST= manual with:

1) export GYP_DEFINES="chromeos=1"
2) build
3) out/Release/chrome --enable-pnacl --enable-nacl --user-data-dir=/tmp/temp_profile_chromeos --login-profile=user --login-manager
4) wait 20 seconds and see /tmp/temp_profile_chromeos/test-user/pnacl/0.0.0.5 show up

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

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

[Dmitry Polukhin, 2013-03-26 11:59:56]

LGTM with nits

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:490: if
(!command_line.HasSwitch(switches::kDisableCRLSets))
Nit, please add {} around inner if.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:495: if
(g_browser_process->pnacl_component_installer())
Nit, same here. If-body has more than 1 line so it should be enclosed in {}.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:156:
FILE_PATH_LITERAL("pnacl"));
Nit, 2 idents.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:20: class
PnaclComponentInstaller : public ComponentInstaller {
Nit, class comment would be nice.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:34: #if
defined(OS_CHROMEOS)
Optional suggestion it looks like we can avoid all but one "#if
defined(OS_CHROMEOS)" is we make flag that distinguish user level component from
machine level. On Chrome OS we should use user level.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:42: Version
get_current_version() {
Nit, as far as know know getters should be without "get_" prefix.

[jvoung - send to chromium..., 2013-03-26 15:46:53]

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:490: if
(!command_line.HasSwitch(switches::kDisableCRLSets))
On 2013/03/26 11:59:56, Dmitry Polukhin wrote:
> Nit, please add {} around inner if.

Done.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:495: if
(g_browser_process->pnacl_component_installer())
On 2013/03/26 11:59:56, Dmitry Polukhin wrote:
> Nit, same here. If-body has more than 1 line so it should be enclosed in {}.

Done.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:156:
FILE_PATH_LITERAL("pnacl"));
On 2013/03/26 11:59:56, Dmitry Polukhin wrote:
> Nit, 2 idents.

Done.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:20: class
PnaclComponentInstaller : public ComponentInstaller {
On 2013/03/26 11:59:56, Dmitry Polukhin wrote:
> Nit, class comment would be nice.

Done.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:34: #if
defined(OS_CHROMEOS)
On 2013/03/26 11:59:56, Dmitry Polukhin wrote:
> Optional suggestion it looks like we can avoid all but one "#if
> defined(OS_CHROMEOS)" is we make flag that distinguish user level component
from
> machine level. On Chrome OS we should use user level.

That's true, it would be more clear why the code is that way -- Done.

The profile observer is still if-def'ed, though, because the notification types
are only available when compiling for chromeos.

https://codereview.chromium.org/13071002/diff/9014/chrome/browser/component_u...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:42: Version
get_current_version() {
On 2013/03/26 11:59:56, Dmitry Polukhin wrote:
> Nit, as far as know know getters should be without "get_" prefix.

Done.

[jvoung - send to chromium..., 2013-03-26 16:10:54]

sky, can you give an OWNERS review for chrome/test and chrome/browser, or should
I look for someone else?

[sky, 2013-03-26 21:04:43]

The chrome/browser and chrome/test changes LGTM, but get a local reviewer for
the profile change as well as the c/b/component_updater changes.

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:770: if
(!pnacl_component_installer_.get()) {
nit: no {}

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:491: if
(g_browser_process->crl_set_fetcher()) {
combine with previous if

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:12: 
nit: remove one of these newlines.

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:60: };
DISALLOW_...

[jvoung (off chromium), 2013-03-26 21:33:34]

Thanks for the review.

cpu: Is this change okay w.r.t component updater usage?

davemoore: Is the profile_manager change okay?

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:770: if
(!pnacl_component_installer_.get()) {
On 2013/03/26 21:04:44, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:491: if
(g_browser_process->crl_set_fetcher()) {
On 2013/03/26 21:04:44, sky wrote:
> combine with previous if

Done.

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:12: 
On 2013/03/26 21:04:44, sky wrote:
> nit: remove one of these newlines.

Done.

https://codereview.chromium.org/13071002/diff/19001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:60: };
On 2013/03/26 21:04:44, sky wrote:
> DISALLOW_...

Done.

[jvoung (off chromium), 2013-03-28 15:57:54]

On 2013/03/26 21:33:34, jvoung (cr) wrote:
> Thanks for the review.
> 
> cpu: Is this change okay w.r.t component updater usage?
> 
> davemoore: Is the profile_manager change okay?
> 

Ping -- the component updater change is pretty localized to PNaCl.  The thing is
that this may be the first time that kReplaced is exercised?

[cpu_(ooo_6.6-7.5), 2013-03-29 03:09:07]

to your question: yes

I'll finish the review tomorrow. Ping me then. Lots of perf for me to doy.

[jvoung (off chromium), 2013-03-29 21:45:14]

On 2013/03/29 03:09:07, cpu wrote:
> to your question: yes
> 
> I'll finish the review tomorrow. Ping me then. Lots of perf for me to doy.

Ok thanks.

[cpu_(ooo_6.6-7.5), 2013-03-29 22:20:55]

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:375:
pci->set_current_version(version);
see my comment in the header. The passing version in 379 was more
understandable.

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:58:
CrxComponent pnacl_component_;
I don't see the need for the pnacl_component_ member, all the fields can be set
trivially once you know the installed version.

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
File chrome/browser/component_updater/pnacl/pnacl_profile_observer.cc (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
chrome/browser/component_updater/pnacl/pnacl_profile_observer.cc:34: #endif
the #endif should include the notreached, it seems.

[cpu_(ooo_6.6-7.5), 2013-03-29 22:24:38]

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
File chrome/browser/browser_process.h (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
chrome/browser/browser_process.h:222: virtual PnaclComponentInstaller*
pnacl_component_installer() = 0;
why do we need this member now here?

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:760: // component for ChromeOS.
do we need this ifdef block? given that we aleady ifdef the registration of the
crl component?

[jvoung (off chromium), 2013-03-29 22:54:52]

Just answering some questions first, will send out a revised version.

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
File chrome/browser/browser_process.h (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
chrome/browser/browser_process.h:222: virtual PnaclComponentInstaller*
pnacl_component_installer() = 0;
On 2013/03/29 22:24:39, cpu wrote:
> why do we need this member now here?

I wanted to have a single instance of the installer, even though the component
can get re-registered.  The single instance is to ensure that there is just one
PnaclProfileObserver.  Also the PnaclProfileObserver needed to be created
"early" to observe the user logins, since the component isn't registered until
after login.

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:760: // component for ChromeOS.
On 2013/03/29 22:24:39, cpu wrote:
> do we need this ifdef block? given that we aleady ifdef the registration of
the
> crl component?

Yeah, seems like we don't need to ifdef this anymore.  The constructor doesn't
do anything.

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:58:
CrxComponent pnacl_component_;
On 2013/03/29 22:20:55, cpu wrote:
> I don't see the need for the pnacl_component_ member, all the fields can be
set
> trivially once you know the installed version.

That's true, I'll simplify it.

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
File chrome/browser/component_updater/pnacl/pnacl_profile_observer.cc (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
chrome/browser/component_updater/pnacl/pnacl_profile_observer.cc:34: #endif
On 2013/03/29 22:20:55, cpu wrote:
> the #endif should include the notreached, it seems.

Can the Observe() function get called in the non-OS_CHROMEOS case?  In the
non-OS_CHROMEOS case, nothing is .Add'ed to the "registrar_".

Maybe we don't need the NOTREACHED() at all?

[jvoung (off chromium), 2013-03-29 23:22:40]

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:760: // component for ChromeOS.
On 2013/03/29 22:54:52, jvoung (cr) wrote:
> On 2013/03/29 22:24:39, cpu wrote:
> > do we need this ifdef block? given that we aleady ifdef the registration of
> the
> > crl component?
> 
> Yeah, seems like we don't need to ifdef this anymore.  The constructor doesn't
> do anything.

Done -- also removed the null checks that I added since it can't be null
anymore.

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://chromiumcodereview.appspot.com/13071002/diff/27002/chrome/browser/com...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:375:
pci->set_current_version(version);
On 2013/03/29 22:20:55, cpu wrote:
> see my comment in the header. The passing version in 379 was more
> understandable.

Localized the setting of current version like how it was before, on line 317.

[jvoung (off chromium), 2013-04-01 19:38:58]

ptal

[commit-bot: I haz the power, 2013-04-03 20:17:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/13071002/42001

[cpu_(ooo_6.6-7.5), 2013-04-03 21:56:27]

lgtm

One request though, re-registration is something is not tested, can you please
write a unittest for that?

https://codereview.chromium.org/13071002/diff/42001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_profile_observer.h (right):

https://codereview.chromium.org/13071002/diff/42001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_profile_observer.h:21: int type,
indentation here I believe is 4 spaces

https://codereview.chromium.org/13071002/diff/42001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_profile_observer.h:26:
DISALLOW_COPY_AND_ASSIGN(PnaclProfileObserver);
make the macro be at the end

[jvoung (off chromium), 2013-04-04 04:06:24]

Thanks for the review!  Added unit_test for re-registration.

https://codereview.chromium.org/13071002/diff/42001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_profile_observer.h (right):

https://codereview.chromium.org/13071002/diff/42001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_profile_observer.h:21: int type,
On 2013/04/03 21:56:27, cpu wrote:
> indentation here I believe is 4 spaces

Done.

https://codereview.chromium.org/13071002/diff/42001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_profile_observer.h:26:
DISALLOW_COPY_AND_ASSIGN(PnaclProfileObserver);
On 2013/04/03 21:56:27, cpu wrote:
> make the macro be at the end

Done.

[cpu_(ooo_6.6-7.5), 2013-04-04 18:22:26]

lgtm

[commit-bot: I haz the power, 2013-04-04 18:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/13071002/75002

[commit-bot: I haz the power, 2013-04-04 18:50:29]

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

[jvoung (off chromium), 2013-04-04 23:18:31]

Committed patchset #12 manually as r192415 (presubmit successful).

[jvoung (off chromium), 2013-04-05 23:20:48]

Sorry for the churn, but the last CL (patch set 12) had some bugs (see inline
comments).

Dmitry or Carlos, would you be will to do one more pass?  I uploaded the fixes
to this CL as patch set 14.

https://codereview.chromium.org/13071002/diff/60004/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://codereview.chromium.org/13071002/diff/60004/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:414:
profile_observer_.reset(new PnaclProfileObserver(this));
One fix: Only create the profile observer *after* cus_ is set to something
non-null, so that ReRegister...() cannot get called and start using cus_ before
the cus_ is actually set.

https://codereview.chromium.org/13071002/diff/60004/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:419:
BrowserThread::UI, FROM_HERE,
One fix: was only supposed to check profile_manager() from the UI thread.

Now all the profile_manager() bits get queried during GetProfileInformation()
and the bits are stored in the installer (mainly the current_profile_path_).

[Dmitry Polukhin, 2013-04-08 14:54:07]

LGTM

[commit-bot: I haz the power, 2013-04-08 21:24:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/13071002/119001

[commit-bot: I haz the power, 2013-04-08 22:17:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jvoung (off chromium), 2013-04-09 00:40:03]

Committed patchset #15 manually as r192969 (presubmit successful).