Immediate notification if active promotion arrives

chrome/browser/web_resource/promo_resource_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/web_resource/promo_resource_service.h"
 
 #include "base/command_line.h"
 #include "base/rand_util.h"
 #include "base/string_number_conversions.h"
 #include "base/threading/thread_restrictions.h"
@@ skipping 134 matching lines @@
                                                 double promo_end) {
   if (promo_start > 0 && promo_end > 0) {
     int64 ms_until_start =
         static_cast<int64>((base::Time::FromDoubleT(
             promo_start) - base::Time::Now()).InMilliseconds());
     int64 ms_until_end =
         static_cast<int64>((base::Time::FromDoubleT(
             promo_end) - base::Time::Now()).InMilliseconds());
     if (ms_until_start > 0)
       PostNotification(ms_until_start);
     if (ms_until_end > 0) {

[achuithb, 2012/06/07 20:34:27]

Maybe also add else here?

[aruslan, 2012/06/07 20:55:40]

Done.

-      PostNotification(ms_until_end);
       if (ms_until_start <= 0) {
         // Notify immediately if time is between start and end.
         PostNotification(0);
       }
+      // Schedule notification at the end of promotion.

[achuithb, 2012/06/07 20:34:27]

Why not use an else block here? I think we are protected by
web_resource_update_scheduled_, but there is no reason to call PostNotification
twice, right?

[aruslan, 2012/06/07 20:55:40]

We should call PostNotification twice -- once with 0 to get an immediate
notification, and the second time with ms_until_end to schedule a future
notification once the promotion is ended, unless I'm missing something.

[achuithb, 2012/06/07 21:31:28]

I see now, that PostNotification sets web_resource_update_scheduled_ to true,
but PromoResourceStateChanged, which gets called from PostNotification, sets it
to false, so the second notification gets scheduled after all. These side
effects are a bit nasty - could you please add some comments about the
orderings, how many notifications are scheduled, etc?

+      PostNotification(ms_until_end);
     }
   }
 }
 
 void PromoResourceService::ScheduleNotificationOnInit() {
   std::string locale = g_browser_process->GetApplicationLocale();
   if (GetPromoServiceVersion() != kPromoServiceVersion ||
       GetPromoLocale() != locale) {
     // If the promo service has been upgraded or Chrome switched locales,
     // refresh the promos.
@@ skipping 11 matching lines @@
     // invalidate the NTP cache at the time of the promo start.
     double promo_start = prefs_->GetDouble(prefs::kNtpPromoStart);
     double promo_end = prefs_->GetDouble(prefs::kNtpPromoEnd);
     ScheduleNotification(promo_start, promo_end);
   }
 }
 
 void PromoResourceService::PostNotification(int64 delay_ms) {
   if (web_resource_update_scheduled_)
     return;
   // TODO(achuith): This crashes if we post delay_ms = 0 to the message loop.

[achuithb, 2012/06/07 21:31:28]

Since we want 2 notifications to be scheduled, posting to the message loop when
delay_ms is 0 is not the right thing to do. Maybe remove this comment, and
replace with another comment that explains that PostNotification with delay_ms
needs to immediately clear web_resource_update_scheduled_?

   // during startup.
   if (delay_ms > 0) {
     web_resource_update_scheduled_ = true;
     MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&PromoResourceService::PromoResourceStateChange,
                    weak_ptr_factory_.GetWeakPtr()),
         base::TimeDelta::FromMilliseconds(delay_ms));
   } else if (delay_ms == 0) {
     PromoResourceStateChange();
@@ skipping 179 matching lines @@
       !(old_logo_end == logo_end)) {
     prefs_->SetDouble(prefs::kNtpCustomLogoStart, logo_start);
     prefs_->SetDouble(prefs::kNtpCustomLogoEnd, logo_end);
     content::NotificationService* service =
         content::NotificationService::current();
     service->Notify(chrome::NOTIFICATION_PROMO_RESOURCE_STATE_CHANGED,
                     content::Source<WebResourceService>(this),
                     content::NotificationService::NoDetails());
   }
 }