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

chrome/browser/component_updater/component_updater_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/component_updater/component_updater_service.h"
 
 #include <algorithm>
 #include <set>
 #include <vector>
 
@@ skipping 293 matching lines @@
  public:
   explicit CrxUpdateService(ComponentUpdateService::Configurator* config);
 
   virtual ~CrxUpdateService();
 
   // Overrides for ComponentUpdateService.
   virtual Status Start() OVERRIDE;
   virtual Status Stop() OVERRIDE;
   virtual Status RegisterComponent(const CrxComponent& component) OVERRIDE;
   virtual Status CheckForUpdateSoon(const CrxComponent& component) OVERRIDE;
+  virtual bool IsUpdateNotificationForComponent(
+      const content::NotificationSource& source,
+      const CrxComponent& component) OVERRIDE;
 
   // The only purpose of this class is to forward the
   // UtilityProcessHostClient callbacks so CrxUpdateService does
   // not have to derive from it because that is refcounted.
   class ManifestParserBridge : public UtilityProcessHostClient {
    public:
     explicit ManifestParserBridge(CrxUpdateService* service)
         : service_(service) {}
 
     virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE {
@@ skipping 653 matching lines @@
       break;
     default:
       event = Configurator::kUnpackError;
       break;
   }
 
   config_->OnEvent(event, CrxIdtoUMAId(component_id));
   ScheduleNextRun(false);
 }
 
+bool CrxUpdateService::IsUpdateNotificationForComponent(

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

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.

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

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?

+    const content::NotificationSource& src,
+    const CrxComponent& component) {
+  std::string id =
+      HexStringToID(StringToLowerASCII(base::HexEncode(&component.pk_hash[0],
+                                       component.pk_hash.size()/2)));
+  return content::Source<std::string>(src)->compare(id) == 0;
+}
+
 // The component update factory. Using the component updater as a singleton
 // is the job of the browser process.
 ComponentUpdateService* ComponentUpdateServiceFactory(
     ComponentUpdateService::Configurator* config) {
   DCHECK(config);
   return new CrxUpdateService(config);
 }