Add an outdated upgrade bubble view.

chrome/browser/upgrade_detector_impl.h

 // Copyright (c) 2011 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.
 
 #ifndef CHROME_BROWSER_UPGRADE_DETECTOR_IMPL_H_
 #define CHROME_BROWSER_UPGRADE_DETECTOR_IMPL_H_
 
 #include "base/memory/weak_ptr.h"
 #include "base/timer.h"
 #include "chrome/browser/upgrade_detector.h"
 
 template <typename T> struct DefaultSingletonTraits;
 
 class UpgradeDetectorImpl : public UpgradeDetector {
  public:
   virtual ~UpgradeDetectorImpl();
 
   // Returns the singleton instance.
   static UpgradeDetectorImpl* GetInstance();
 
  private:
   friend struct DefaultSingletonTraits<UpgradeDetectorImpl>;
 
   UpgradeDetectorImpl();
 
+  // Start the timer that will call |CheckForUpgrade()|.
+  void StartCheckForUpgradeTimer();
+
   // Launches a task on the file thread to check if we have the latest version.
   void CheckForUpgrade();
 
   // Sends out a notification and starts a one shot timer to wait until
   // notifying the user.
-  void UpgradeDetected();
+  void UpgradeDetected(UpgradeAvailable upgrade_available,
+                       bool unstable_channel);
+
+  // Returns true after calling UpgradeDetected if current install is outdated.

[Finnur, 2013/02/04 11:12:04]

If you change what I suggested, then you can remove the UpgradeDetected
reference here.

[MAD, 2013/02/04 17:17:02]

I didn't... I hope it's OK... :-)

+  bool CheckForOutdatedInstall();
 
   // The function that sends out a notification (after a certain time has
   // elapsed) that lets the rest of the UI know we should start notifying the
   // user that a new version is available.
   void NotifyOnUpgrade();
 
+  // Called on the FILE thread to detect an upgrade. Calls back UpgradeDetected
+  // on the UI thread if so.
+  static void DetectUpgradeTask(

[Finnur, 2013/02/04 11:12:04]

Does this still need to be static. If so, I'd like to learn more about the data
race you referred to. It looks to me like we are back to the liniar mode of
running this, so there shouldn't be a data race (except on the variable I
mentioned, because it is set so late in the game).

[MAD, 2013/02/04 17:17:02]

For the same reason that we call InvalidateWeakPtrs in case we get to
CheckForUpgrade before DetectUpgradeTask is done, then it could be messing up
with data member in the file thread as we might be interacting with them in the
UI thread.

I prefer passing the value we want for upgrade_available to UpgradeDetected()
instead of passing the address of the data member to the file thread and get it
changed there. Don't you agree that this is cleaner? This way we ensure that
value is always get/set from the UI thread. Or am I missing something that gets
you to prefer doing it the old way?

[Finnur, 2013/02/04 21:30:17]

I need a fresh mind to think about this. I'll try to tackle this tomorrow.

[Finnur, 2013/02/05 10:52:19]

So, to answer your question: No, I don't prefer the old way. I like sending the
value over to the UI thread. However, by doing so, the function no longer
touches data members of the class and therefore does not need to be static.
Correct?

If I am mistaken, can you spell it out in a little more detail which variables
get touched and at what point, so I can understand the concern better?

[MAD, 2013/02/05 16:49:16]

Ha! Now I see where the confusion is... It used to not be a member function
because it was calling an anonymous callback. Now we can don't this if we pass
in arguments to the function. So either we make this a member function or we
make the call back function (UpgradeDetected) public.

Clear?

[Finnur, 2013/02/05 22:14:50]

Member function would be my preference.

[MAD, 2013/02/06 03:14:28]

You mean not static?
(cause... you know... technically... a static method... is... a member
function... :-)...

The reason why I used a static was to prevent using the this pointer in the FILE
thread and force usage of a weak pointer...

So I tried to see if I could build this differently, e.g., bring back the
function out of the class and use a partially bound callback, so that we could
put the weak pointer in it, yet let the caller of the callback specify the other
argument... But as I read the documentation for all this, I realize that weak
pointers are not supposed to be used across threads... So this code was already
broken... The weak pointer invalidation useless, and even dangerous...

So before I start changing this I would like to have your opinion. I'm thinking
of using an unretained this pointer to post the calls across the threads, and
maybe use an atomically thread safe gate to make sure we don't cross call
UpgradeDetected... Do you have other ideas?

[Finnur, 2013/02/06 10:26:49]

I see.
No. If you can boil this down to a comment for the static function, then I'm
fine with this as is.

On 2013/02/06 03:14:28, MAD wrote:

+      base::WeakPtr<UpgradeDetectorImpl> upgrade_detector);
+
   // We periodically check to see if Chrome has been upgraded.
   base::RepeatingTimer<UpgradeDetectorImpl> detect_upgrade_timer_;
 
   // After we detect an upgrade we start a recurring timer to see if enough time
   // has passed and we should start notifying the user.
   base::RepeatingTimer<UpgradeDetectorImpl> upgrade_notification_timer_;
 
   // We use this factory to create callback tasks for UpgradeDetected. We pass
   // the task to the actual upgrade detection code, which is in
   // DetectUpgradeTask.
   base::WeakPtrFactory<UpgradeDetectorImpl> weak_factory_;
 
   // True if this build is a dev or canary channel build.
   bool is_unstable_channel_;
 
+  // The date the binaries were built.
+  base::Time build_date_;
+
   DISALLOW_COPY_AND_ASSIGN(UpgradeDetectorImpl);
 };
 
 
 #endif  // CHROME_BROWSER_UPGRADE_DETECTOR_IMPL_H_