Consistently use notifications from component updater w/ on-demand PNaCl.

Consistently use notifications from component updater w/ on-demand PNaCl.

Once an "update ready" notification is sent, the
PNaCl installer will get to see if the install is
successful or not.  At that point, we can stop watching
for the "updater is sleeping" notification.

Previously, we would need to be careful about whether
the PNaCl installer gets through to notify its
callbacks first, or if the "updater is sleeping"
happens first.  Now there is more of a defined
sequence of events.

Move to the new style of using the observer, and off
of using the notification registrar.

Also, get rid of the CancelCallback within the
PNaCl installer, since it's not used at all, and simplify
it so that only one callback is active at once, since
only one install request will ever actually go through.

Move RequestFirstInstall out of the global namespace while
we are at it...

BUG=none

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

[jvoung (off chromium), 2013-06-27 16:23:28]

Hi Carlos, in the review https://codereview.chromium.org/17001003/ you brought
up a good point that it was a bit brittle.

Is this approach better?

[cpu_(ooo_6.6-7.5), 2013-07-01 18:30:22]

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:175: typedef
std::string UpdateSource;
not big fan of this typedef

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:31:
registrar_.Remove(this,
registrar_. RemoveAll() ?

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:37:
is_observing_ = false;
registrar_ IsEmpty()?

https://codereview.chromium.org/18006003/diff/39001/content/public/test/test_...
File content/public/test/test_notification_tracker.h (right):

https://codereview.chromium.org/18006003/diff/39001/content/public/test/test_...
content/public/test/test_notification_tracker.h:47: 
if we need virtual here, I expect to see some code overriding it with the
OVERRIDE at the end of the declaration, but such code is not in the CL.

[jvoung (off chromium), 2013-07-02 01:04:26]

Thanks for the review!

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:175: typedef
std::string UpdateSource;
On 2013/07/01 18:30:22, cpu wrote:
> not big fan of this typedef

Ok, removed and changed back to string.

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:31:
registrar_.Remove(this,
On 2013/07/01 18:30:22, cpu wrote:
> registrar_. RemoveAll() ?

Ah yes, Done.

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:37:
is_observing_ = false;
On 2013/07/01 18:30:22, cpu wrote:
> registrar_ IsEmpty()?

Done.

https://codereview.chromium.org/18006003/diff/39001/content/public/test/test_...
File content/public/test/test_notification_tracker.h (right):

https://codereview.chromium.org/18006003/diff/39001/content/public/test/test_...
content/public/test/test_notification_tracker.h:47: 
On 2013/07/01 18:30:22, cpu wrote:
> if we need virtual here, I expect to see some code overriding it with the
> OVERRIDE at the end of the declaration, but such code is not in the CL.

Is line 107 of:

https://codereview.chromium.org/18006003/diff/39001/chrome/browser/component_...

what you are looking for?

[cpu_(ooo_6.6-7.5), 2013-07-03 18:04:53]

lgtm

But give a chance for sorin to look at it. I guess that new function you added
to CUS public interface looks handy but it does not do anything that cannot be
done in a stand alone helper. I can go either way but I want a second opinion.

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:51: // or an
error occurred.
or you are up to date..

[Sorin Jianu, 2013-07-03 21:17:32]

Thank you!

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:167: const
CrxComponent& component);
This static member seems out of place in the definition of
ComponentUpdateService, which is a abstract class. Having a statric member here
could also make mocking of the class difficult in the future.

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

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:352:
CrxComponent pnacl_component = pci->GetCrxComponent();
could declare the variable const here and below.

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

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:50:
CrxComponent GetCrxComponent();
could declare the function const.

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
File chrome/browser/component_updater/test/component_updater_service_unittest.h
(right):

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/test/component_updater_service_unittest.h:99:
// dereferencing an already-free'd pointer.
free'd

perhaps use a different word such as deleted or deallocated.

[jvoung (off chromium), 2013-07-03 22:51:50]

thanks

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:167: const
CrxComponent& component);
On 2013/07/03 21:17:32, Sorin Jianu wrote:
> This static member seems out of place in the definition of
> ComponentUpdateService, which is a abstract class. Having a statric member
here
> could also make mocking of the class difficult in the future.

Ah good point. Changed to non-static.

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

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:352:
CrxComponent pnacl_component = pci->GetCrxComponent();
On 2013/07/03 21:17:32, Sorin Jianu wrote:
> could declare the variable const here and below.

Done.

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

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:50:
CrxComponent GetCrxComponent();
On 2013/07/03 21:17:32, Sorin Jianu wrote:
> could declare the function const.

Done.  It does introduce a const-cast when taking a pointer to "this" though
(see the .cc file).

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:51: // or an
error occurred.
On 2013/07/03 18:04:53, cpu wrote:
> or you are up to date..

Done.

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
File chrome/browser/component_updater/test/component_updater_service_unittest.h
(right):

https://codereview.chromium.org/18006003/diff/56001/chrome/browser/component_...
chrome/browser/component_updater/test/component_updater_service_unittest.h:99:
// dereferencing an already-free'd pointer.
On 2013/07/03 21:17:32, Sorin Jianu wrote:
> free'd
> 
> perhaps use a different word such as deleted or deallocated.

Done.

[Sorin Jianu, 2013-07-03 23:10:04]

Thank you Jan!

https://codereview.chromium.org/18006003/diff/72001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18006003/diff/72001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:990: bool
CrxUpdateService::IsUpdateNotificationForComponent(
What is strange about this function is that is seems to be a utility function
that queries some other module. I would just make this a non-member function and
make it available in component_updater_service.h.

I believe cpu@ made a comment that he'd be open to make this a non-member.

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

https://codereview.chromium.org/18006003/diff/72001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:336:
CrxComponent PnaclComponentInstaller::GetCrxComponent() const {
In this case, I suggest dropping the const declaration.

[jvoung (off chromium), 2013-07-04 01:11:04]

https://codereview.chromium.org/18006003/diff/72001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/18006003/diff/72001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:990: bool
CrxUpdateService::IsUpdateNotificationForComponent(
On 2013/07/03 23:10:04, Sorin Jianu wrote:
> What is strange about this function is that is seems to be a utility function
> that queries some other module. I would just make this a non-member function
and
> make it available in component_updater_service.h.
> 
> I believe cpu@ made a comment that he'd be open to make this a non-member.


One argument for having it be a member function, is that the function has
knowledge about the type of the notifications that this "CrxUpdateService"
implementation sends.  In this case, it's a std::string, which happens to be the
CRX's app ID.

If there was a different implementation, it could send a different kind of
notification (something unrelated to CRXes).  However, there's currently no way
for the observer to know that "chrome::NOTIFICATION_COMPONENT_UPDATE_FOUND" came
from this implementation, and therefore should check against this
implementation's IsUpdateNotificationForComponent() method.  Perhaps the source
should be a pointer to the component update service (which is what is done for
chrome::NOTIFICATION_COMPONENT_UPDATER_STARTED), and the details should the ID
string.  Then you could do source->IsUpdateNotificationForComponent(details).


On the other hand, a simple utility function also works, because this is the
only implementation for now...

Thoughts?

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

https://codereview.chromium.org/18006003/diff/72001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:336:
CrxComponent PnaclComponentInstaller::GetCrxComponent() const {
On 2013/07/03 23:10:04, Sorin Jianu wrote:
> In this case, I suggest dropping the const declaration.

Done.

[cpu_(ooo_6.6-7.5), 2013-07-08 21:45:36]

We are thinking (well Soring is doing the code) of adding observers to the
component updater so maybe the way to go is to give a chance during registration
to have the installer add itself as an observer.

The in progress CL is here
https://codereview.chromium.org/18516010/

[jvoung (off chromium), 2013-07-08 22:46:25]

On 2013/07/08 21:45:36, cpu wrote:
> We are thinking (well Soring is doing the code) of adding observers to the
> component updater so maybe the way to go is to give a chance during
registration
> to have the installer add itself as an observer.
> 
> The in progress CL is here
> https://codereview.chromium.org/18516010/


Ah cool.  Yes, it would be nice if there was just one system for this sort of
thing.  Will the events cover the cases where this currently  watches for the
component update service going to sleep also (already up to date, an update
error, or never got around to the update)?  Or can it be extended that way? 
Looks like it currently covers update errors.

[sorin, 2013-07-09 05:32:30]

I am changing the code to use a generic observer, which can be extended to
notify for other worthy events in the component updater service. I will
send the code for review this Tuesday.


On Mon, Jul 8, 2013 at 3:46 PM, <jvoung@chromium.org> wrote:

> On 2013/07/08 21:45:36, cpu wrote:
>
>> We are thinking (well Soring is doing the code) of adding observers to the
>> component updater so maybe the way to go is to give a chance during
>>
> registration
>
>> to have the installer add itself as an observer.
>>
>
>  The in progress CL is here
>>
https://codereview.chromium.**org/18516010/<https://codereview.chromium.org/1...
>>
>
>
> Ah cool.  Yes, it would be nice if there was just one system for this sort
> of
> thing.  Will the events cover the cases where this currently  watches for
> the
> component update service going to sleep also (already up to date, an update
> error, or never got around to the update)?  Or can it be extended that way?
> Looks like it currently covers update errors.
>
>
>
https://codereview.chromium.**org/18006003/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-08-01 20:27:29]

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

[commit-bot: I haz the power, 2013-08-01 20:27:39]

Failed to apply patch for
chrome/browser/component_updater/test/component_updater_service_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/component_updater/test/component_updater_service_unittest.cc
  Hunk #1 FAILED at 128.
  Hunk #2 succeeded at 170 (offset 1 line).
  Hunk #3 succeeded at 388 (offset 34 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/component_updater/test/component_updater_service_unittest.cc.rej

Patch:      
chrome/browser/component_updater/test/component_updater_service_unittest.cc
Index:
chrome/browser/component_updater/test/component_updater_service_unittest.cc
diff --git
a/chrome/browser/component_updater/test/component_updater_service_unittest.cc
b/chrome/browser/component_updater/test/component_updater_service_unittest.cc
index
37de9217f8db89d87743254655078cf0345aeb7f..2f6514cdf566a3174f31edbabb86c8daa0b15198
100644
---
a/chrome/browser/component_updater/test/component_updater_service_unittest.cc
+++
b/chrome/browser/component_updater/test/component_updater_service_unittest.cc
@@ -128,6 +128,32 @@ void
TestConfigurator::SetComponentUpdateService(ComponentUpdateService* cus) {
   cus_ = cus;
 }
 
+TestUpdateNotificationTracker::TestUpdateNotificationTracker() {}
+
+TestUpdateNotificationTracker::~TestUpdateNotificationTracker() {}
+
+std::string TestUpdateNotificationTracker::SourceAt(size_t num) {
+  return sources_[num];
+}
+
+void TestUpdateNotificationTracker::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  if (type == chrome::NOTIFICATION_COMPONENT_UPDATE_FOUND
+      || type == chrome::NOTIFICATION_COMPONENT_UPDATE_READY) {
+    sources_.push_back(std::string(
+        *content::Source<std::string>(source).ptr()));
+  } else
+    sources_.push_back(std::string());
+  TestNotificationTracker::Observe(type, source, details);
+}
+
+void TestUpdateNotificationTracker::Reset() {
+  sources_.clear();
+  TestNotificationTracker::Reset();
+}
+
 ComponentUpdaterTest::ComponentUpdaterTest() : test_config_(NULL) {
   // The component updater instance under test.
   test_config_ = new TestConfigurator;
@@ -169,7 +195,7 @@ const base::FilePath ComponentUpdaterTest::test_file(const
char* file) {
   return test_data_dir_.AppendASCII(file);
 }
 
-TestNotificationTracker& ComponentUpdaterTest::notification_tracker() {
+TestUpdateNotificationTracker& ComponentUpdaterTest::notification_tracker() {
   return notification_tracker_;
 }
 
@@ -354,10 +380,22 @@ TEST_F(ComponentUpdaterTest, InstallCrx) {
   ASSERT_EQ(5ul, notification_tracker().size());
 
   TestNotificationTracker::Event ev1 = notification_tracker().at(1);
+  std::string source1 = notification_tracker().SourceAt(1);
   EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATE_FOUND, ev1.type);
+  // Check that the UPDATE_FOUND is for com1.
+  EXPECT_FALSE(component_updater()->IsUpdateNotificationForComponent(
+      content::Source<std::string>(&source1), com2));
+  EXPECT_TRUE(component_updater()->IsUpdateNotificationForComponent(
+      content::Source<std::string>(&source1), com1));
 
   TestNotificationTracker::Event ev2 = notification_tracker().at(2);
+  std::string source2 = notification_tracker().SourceAt(2);
   EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATE_READY, ev2.type);
+  // Check that the UPDATE_READY is for com1.
+  EXPECT_FALSE(component_updater()->IsUpdateNotificationForComponent(
+      content::Source<std::string>(&source2), com2));
+  EXPECT_TRUE(component_updater()->IsUpdateNotificationForComponent(
+      content::Source<std::string>(&source2), com1));
 
   TestNotificationTracker::Event ev3 = notification_tracker().at(3);
   EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING, ev3.type);

[jvoung (off chromium), 2013-08-02 18:21:00]

Cool, looks like the observer framework has landed now, so I will try to rebase
this CL.  Thanks!

[jvoung (off chromium), 2013-08-02 21:17:59]

Ok, I've updated the CL to use the new observers.

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (left):

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:99: bool
CheckPnaclComponentManifest(const base::DictionaryValue& manifest,
Noticed that this shouldn't have been in the global namespace, so cleaned it up
too.

[Sorin Jianu, 2013-08-05 19:24:58]

lgtm

There are some issues with this code and I suggest the following, with the idea
that we unblock upcoming work and we iterate further to improve the code.

1. We land this code as is, eventually after addressing some of the fluff
feedback below.
2. waffles@ has a code that refactors the component installers. By landing this,
we unblock him.
3. we unblock the other change you have, to make async calls when installing
4. Waffles and I will fix the notifications so that we are being called
hopefully once and only once for success or errors.

How does this look?

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

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:474: void
PnaclComponentInstaller::RequestFirstInstall(
does the argument fit on the line above?

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:44:
must_observe_ = false;
These could be good events to fire: skipping an install, an error, or no update.

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:313:
updater_observer_->StopObserving();
Would it make sense to stop observing right away to avoid rentrancy issues in
case of spurious events if the call to Run() starts dispatchig messages?

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:78: friend
class PnaclUpdaterObserver;
Since I am a n00b in Chrome dev I am not sure what the coding style is in this
case. In general, I am trying to avoid friend declarations. In this case, this
can be avoided either by having this class be the observer by using public
inheritance or you could make the Notify* members below public. Regarding the
latter piece of advice, I am aware it is a matter of taste, I am not asking you
to do it, I am just mentioning that class friendship deserves closer attention.

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:37: // to run
the callback w/ success or failure.
There is case where this would break but it is going to be hard to fix with the
current design and implementation of the code in the component updater.

This function will be called when the CRX (or a differential update of it) has
been downloaded. So in the case of a diff update, we call it, we unsubscribe for
notifications here, but if the patch fails, CUS will try the full, and if the
full update fails too, the NotifyInstallError will not get called.

[jvoung (off chromium), 2013-08-05 20:36:02]

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

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:474: void
PnaclComponentInstaller::RequestFirstInstall(
On 2013/08/05 19:24:58, Sorin Jianu wrote:
> does the argument fit on the line above?

Done.

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/97001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:44:
must_observe_ = false;
On 2013/08/05 19:24:58, Sorin Jianu wrote:
> These could be good events to fire: skipping an install, an error, or no
update.

That would be more direct of a signal, but we should watch out about making too
many events for the installer to consider (would also be good to not have to
worry about the events firing twice for the diff vs full issue you mention).

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:313:
updater_observer_->StopObserving();
On 2013/08/05 19:24:58, Sorin Jianu wrote:
> Would it make sense to stop observing right away to avoid rentrancy issues in
> case of spurious events if the call to Run() starts dispatchig messages?

Good idea, I'll move the StopObserving to an earlier line just in case.  The
Reset() comes too late for that to prevent the install_callback_ from running
more than once.  Perhaps it would have been better if there was a "RunOnce()"
that takes the data, does the reset, then actually runs.

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.h (right):

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
chrome/browser/component_updater/pnacl/pnacl_component_installer.h:78: friend
class PnaclUpdaterObserver;
On 2013/08/05 19:24:58, Sorin Jianu wrote:
> Since I am a n00b in Chrome dev I am not sure what the coding style is in this
> case. In general, I am trying to avoid friend declarations. In this case, this
> can be avoided either by having this class be the observer by using public
> inheritance or you could make the Notify* members below public. Regarding the
> latter piece of advice, I am aware it is a matter of taste, I am not asking
you
> to do it, I am just mentioning that class friendship deserves closer
attention.

In general chrome code style is like google code style.  I usually only see
friend used for base::RefCountedThreadSafe and things like that, but felt it a
bit better in this case to do this instead of making them public.  Perhaps
making this class inherit and implement the observer would be better.

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc (right):

https://codereview.chromium.org/18006003/diff/101001/chrome/browser/component...
chrome/browser/component_updater/pnacl/pnacl_updater_observer.cc:37: // to run
the callback w/ success or failure.
On 2013/08/05 19:24:58, Sorin Jianu wrote:
> There is case where this would break but it is going to be hard to fix with
the
> current design and implementation of the code in the component updater.
> 
> This function will be called when the CRX (or a differential update of it) has
> been downloaded. So in the case of a diff update, we call it, we unsubscribe
for
> notifications here, but if the patch fails, CUS will try the full, and if the
> full update fails too, the NotifyInstallError will not get called. 


Good point about how the install could take more than one try (diff and full). 
Right now we are only using it to install from version 0.  Does that also
attempt a diff first?

[commit-bot: I haz the power, 2013-08-05 21:23:01]

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

[commit-bot: I haz the power, 2013-08-05 23:58:50]

Change committed as 215732