Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10541054:
Immediate notification if active promotion arrives (Closed)
|
Created:
8 years, 6 months ago by aruslan Modified:
8 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImmediate notification if active promotion arrives
If an active (begin<now<end) promotion arrives after the initial
launch sequence, the promo resource service will notify clients
immediately AND schedule a notification at the end of promotion.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141158
Patch Set 1 #
Total comments: 2
Patch Set 2 : Comment fix #
Total comments: 6
Patch Set 3 : More comments and "else". #Messages
Total messages: 12 (0 generated)
so this change to make sure we don't accidentally run the end notification before the start? having a hard time telling the functional difference here from just moving the same LOC down a couple lines... https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... chrome/browser/web_resource/promo_resource_service.cc:160: // Schedule notification at the end of promotion nit: . at end of comment.
On 2012/06/07 18:14:57, Dan Beam wrote: > so this change to make sure we don't accidentally run the end notification > before the start? having a hard time telling the functional difference here from > just moving the same LOC down a couple lines... > > https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... > File chrome/browser/web_resource/promo_resource_service.cc (right): > > https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... > chrome/browser/web_resource/promo_resource_service.cc:160: // Schedule > notification at the end of promotion > nit: . at end of comment. so this change is to**
The logic before the change was correct (we need to send the notification immediately if it is currently active, and we need to schedule a notification at the promotion end time). However, the order was wrong: once you schedule a notification, all subsequent attempts to schedule a notification or to immediately notify observers are ignored due to "web_resource_update_scheduled_" flag. https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... chrome/browser/web_resource/promo_resource_service.cc:160: // Schedule notification at the end of promotion On 2012/06/07 18:14:57, Dan Beam wrote: > nit: . at end of comment. Done.
I don't really know much about promotion stuff, so I'll keep my mouth shut.
On 2012/06/07 18:21:35, aruslan wrote: > The logic before the change was correct (we need to send the notification > immediately if it is currently active, and we need to schedule a notification at > the promotion end time). > However, the order was wrong: once you schedule a notification, all subsequent > attempts to schedule a notification or to immediately notify observers are > ignored due to "web_resource_update_scheduled_" flag. ah, ok, didn't look outside your one line change, :). lgtm but you might want to wait for achuith as well, I assume this wasn't intentional or anything but he might know better... > https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... > File chrome/browser/web_resource/promo_resource_service.cc (right): > > https://chromiumcodereview.appspot.com/10541054/diff/1/chrome/browser/web_res... > chrome/browser/web_resource/promo_resource_service.cc:160: // Schedule > notification at the end of promotion > On 2012/06/07 18:14:57, Dan Beam wrote: > > nit: . at end of comment. > > Done.
https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:155: if (ms_until_end > 0) { Maybe also add else here? https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:160: // Schedule notification at the end of promotion. 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?
Thanks for the comments! Please let me know if the comments make things clearer. https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:155: if (ms_until_end > 0) { On 2012/06/07 20:34:27, achuith.bhandarkar wrote: > Maybe also add else here? Done. https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:160: // Schedule notification at the end of promotion. 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.
https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:160: // Schedule notification at the end of promotion. On 2012/06/07 20:55:40, aruslan wrote: > 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. 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? https://chromiumcodereview.appspot.com/10541054/diff/9001/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:193: // TODO(achuith): This crashes if we post delay_ms = 0 to the message loop. 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_?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aruslan@chromium.org/10541054/9002
Change committed as 141158 |
