Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10860025:
Remove promotion-type-specific JSON handling from NotificationPromo (Closed)
|
Created:
8 years, 4 months ago by aruslan Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSplit JSON processing of parameters that needs to be handled by the
promotion system (e.g. rollout plan parameters) from processing of
promo-type-specific parameters.
BUG=143692
TESTS=unit_tests, manual tests as per test plan
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153180
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressing the review comments. #Patch Set 3 : Comments. #
Total comments: 29
Patch Set 4 : Addressing the review comments (thanks for the review!). #Patch Set 5 : Rebase. #
Total comments: 28
Patch Set 6 : Addressing review comments. #Patch Set 7 : Per Mihai's suggestion -- NotificationPromoMobileNtp now encapsulates NotificationPromo. #Patch Set 8 : Comments. #Patch Set 9 : Added a header file to .gypi #Patch Set 10 : Rebase. #
Total comments: 38
Patch Set 11 : Addressing review comments. #
Total comments: 2
Patch Set 12 : Removed 2 unused pref names; rebase #Patch Set 13 : Indent in contructor. #Messages
Total messages: 34 (0 generated)
Let's also create a bug for this work item (since it's not minor), and upate the CL description. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo.cc (left): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:136: #if defined(OS_ANDROID) Why remove this? We can probably keep it for another cycle before blowing it away? https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:192: base::Value* deepCopyAndResolveStrings( DeepCopyAndResolveStrings. Also, can we return scoped_ptr<base::Value> to make it clear that a new object is being returned? https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:195: DCHECK(node); this DCHECK is pointless. Let's get rid of it. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:233: base::StringValue* copy = new base::StringValue(actual_value); This temporary seems unnecessary. Why not just return new base::StringValue(actual_value); https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:332: DCHECK(ppcopy != NULL); I think DCHECK(ppcopy) is sufficient. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:333: DCHECK(ppcopy->IsType(base::Value::TYPE_DICTIONARY)); You might want to combine the DCHECKs like DCHECK(ppcopy && ppcopy->IsType...) otherwise this will crash anyway. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:429: const base::DictionaryValue* dv(NULL); let's do const base::DictionaryValue* dv = NULL; as this seems to be the preferred construction syntax in chromium. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:431: DCHECK(dv != NULL); unnecessary https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo.h (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.h:101: scoped_ptr<const base::DictionaryValue> promo_payload_; Let's add a comment here saying that this is currently not used on desktop. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:25: return; Is this right? Are all the fields required? What happens if we bail early like this with a partially constructed object? https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/chrome_browser.... chrome/chrome_browser.gypi:4972: 'browser/web_resource/notification_promo_mobile_ntp.cc', Is mobile the right name? Not android? Or is this also applicable for IOS?
Overall, I quite like the changes!
Dan for additional feedback if you have the time.
On vacation today, can look Monday.
Thank you for the comments, Achuith! I added http://crbug.com/143692 and changed description; please take a look at the changed code. Eric, Mihai -- this is an RFC for cleaning up before upstreaming Android parts; let me know if it would work for iOS and if I'm missing anything or there is a way to do this simpler. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo.cc (left): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:136: #if defined(OS_ANDROID) I do not think removing mobile-specific prefs would cause issues in the upgrade/downgrade scenarios -- am I missing something? Otherwise, both iOS and Android are branched out already, so we should be good to go. On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > Why remove this? We can probably keep it for another cycle before blowing it > away? https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:192: base::Value* deepCopyAndResolveStrings( Renamed to DeepCopyAndResolveStrings -- done. I haven't changed the return type and added a comment instead. (It's recursive, and handling scoped_ptr inside seems to be unnecessary complicates the code.) Let me know if you think otherwise. On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > DeepCopyAndResolveStrings. > > Also, can we return scoped_ptr<base::Value> to make it clear that a new object > is being returned? https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:195: DCHECK(node); On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > this DCHECK is pointless. Let's get rid of it. Done. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:233: base::StringValue* copy = new base::StringValue(actual_value); On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > This temporary seems unnecessary. Why not just > return new base::StringValue(actual_value); Done. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:332: DCHECK(ppcopy != NULL); On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > I think DCHECK(ppcopy) is sufficient. Done. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:333: DCHECK(ppcopy->IsType(base::Value::TYPE_DICTIONARY)); On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > You might want to combine the DCHECKs like > DCHECK(ppcopy && ppcopy->IsType...) > otherwise this will crash anyway. Done. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:429: const base::DictionaryValue* dv(NULL); On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > let's do > const base::DictionaryValue* dv = NULL; > as this seems to be the preferred construction syntax in chromium. Done. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.cc:431: DCHECK(dv != NULL); On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > unnecessary Done. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo.h (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo.h:101: scoped_ptr<const base::DictionaryValue> promo_payload_; On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > Let's add a comment here saying that this is currently not used on desktop. Done. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:25: return; Currently all fields are required: if we bail out early, the valid_ field wouldn't be set to true. I can move valid_ = true to just after action_args_ (everything before is clearly necessary) and change the defaults to match some common use case. Would you mind having it in a separate CL -- I"m not sure what the defaults should be :) On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > Is this right? Are all the fields required? What happens if we bail early like > this with a partially constructed object? https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/chrome_browser.... chrome/chrome_browser.gypi:4972: 'browser/web_resource/notification_promo_mobile_ntp.cc', Very good question! I will add Mihai and Eric to this. But yes, it should be applicable for both Android and iOS. On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > Is mobile the right name? Not android? Or is this also applicable for IOS?
https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:125: // TODO(achuith): remove this in m23. It is probably time to remove this now. M22 has been split.
https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo_mobile_ntp.h:20: class NotificationPromoMobileNtp { I would have expected this to extend the NotificationPromo and inherit all the HandleViewed / CanShow methods. Or we are supposed to use this when we need to parse the NotificationPromo object? https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:183: #if !defined(OS_ANDROID) && !defined(OS_IOS) Change the order here: #if defined(OS_ANDROID) && defined(OS_IOS) NotificationPromo::MOBILE_NTP_SYNC_PROMO; #else NotificationPromo::NTP_NOTIFICATION_PROMO; @endif
https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:25: return; On 2012/08/20 15:36:53, aruslan wrote: > Currently all fields are required: if we bail out early, the valid_ field > wouldn't be set to true. > I can move valid_ = true to just after action_args_ (everything before is > clearly necessary) and change the defaults to match some common use case. > Would you mind having it in a separate CL -- I"m not sure what the defaults > should be :) > On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > > Is this right? Are all the fields required? What happens if we bail early like > > this with a partially constructed object? > Please file a bug, add a TODO with comments and a reference to the bug. https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:125: // TODO(achuith): remove this in m23. On 2012/08/20 15:53:16, noyau wrote: > It is probably time to remove this now. M22 has been split. My only concern is that people who skip m22 and go from m21 -> m23 will be left with a whole bunch of garbage prefs on their disk. This code is doing no harm, let's update the comment to m24 and remove it after one more cycle?
https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:38: // TODO(achuith): Shouldn't this be in pref_names? Let's not have these be in pref_names. These should only be accessed through NotificationPromo. Please remove this comment.
made it to promo_resource_service_mobile_ntp_unittest.cc https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:41: // Keys into the kPrefPromoObject dictionary; used only here. nit: Keys in the https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:202: for (base::ListValue::const_iterator it = list->begin(); * wishes we could say for (auto it = list->begin(); it != list->end(); ++it) { :( https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:203: it != list->end(); nit: move ++it; up a line to match style below https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:218: bool rv = dict->GetWithoutPathExpansion(*it, &child); DCHECK(dict->GetWithoutPathExpansion(*it, &child)); https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:230: bool rv = str->GetAsString(&value); DCHECK(str->GetAsString(&value)); https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:333: DCHECK(ppcopy && ppcopy->IsType(base::Value::TYPE_DICTIONARY)); you might want to split these into 2 statements so you see which part of this fails, up to you https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:27: // Delay between calls to update the cache (12 hours), and 3 min in debug mode. why'd you change this? https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:183: #if !defined(OS_ANDROID) && !defined(OS_IOS) On 2012/08/20 16:09:47, msarda wrote: > Change the order here: > #if defined(OS_ANDROID) && defined(OS_IOS) > NotificationPromo::MOBILE_NTP_SYNC_PROMO; > #else > NotificationPromo::NTP_NOTIFICATION_PROMO; > @endif er, I assume you mean #if defined(OS_ANDROID) || defined(OS_IOS) and #endif ?
http://codereview.chromium.org/10860025/diff/8003/chrome/browser/web_resource... File chrome/browser/web_resource/notification_promo.cc (right): http://codereview.chromium.org/10860025/diff/8003/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:218: bool rv = dict->GetWithoutPathExpansion(*it, &child); On 2012/08/20 20:58:12, Dan Beam wrote: > DCHECK(dict->GetWithoutPathExpansion(*it, &child)); Don't think you can do that - it would get compiled out in release. http://codereview.chromium.org/10860025/diff/8003/chrome/browser/web_resource... chrome/browser/web_resource/notification_promo.cc:230: bool rv = str->GetAsString(&value); On 2012/08/20 20:58:12, Dan Beam wrote: > DCHECK(str->GetAsString(&value)); same.
Thanks for the review! PTAL at the changes. https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/1/chrome/browser/web_res... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:25: return; I moved valid_ = true earlier and changed the defaults to match. On 2012/08/20 19:17:48, achuith.bhandarkar wrote: > On 2012/08/20 15:36:53, aruslan wrote: > > Currently all fields are required: if we bail out early, the valid_ field > > wouldn't be set to true. > > I can move valid_ = true to just after action_args_ (everything before is > > clearly necessary) and change the defaults to match some common use case. > > Would you mind having it in a separate CL -- I"m not sure what the defaults > > should be :) > > On 2012/08/17 22:55:12, achuith.bhandarkar wrote: > > > Is this right? Are all the fields required? What happens if we bail early > like > > > this with a partially constructed object? > > > > Please file a bug, add a TODO with comments and a reference to the bug. https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:38: // TODO(achuith): Shouldn't this be in pref_names? On 2012/08/20 19:28:31, achuith.bhandarkar wrote: > Let's not have these be in pref_names. These should only be accessed through > NotificationPromo. Please remove this comment. Done. https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:41: // Keys into the kPrefPromoObject dictionary; used only here. On 2012/08/20 20:58:12, Dan Beam wrote: > nit: Keys in the Done. https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:125: // TODO(achuith): remove this in m23. Done; bug http://crbug.com/143773 added. On 2012/08/20 19:17:48, achuith.bhandarkar wrote: > On 2012/08/20 15:53:16, noyau wrote: > > It is probably time to remove this now. M22 has been split. > > My only concern is that people who skip m22 and go from m21 -> m23 will be left > with a whole bunch of garbage prefs on their disk. This code is doing no harm, > let's update the comment to m24 and remove it after one more cycle? https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:202: for (base::ListValue::const_iterator it = list->begin(); I hope we could "for (auto it : list) {}" soon! :) On 2012/08/20 20:58:12, Dan Beam wrote: > * wishes we could say > > for (auto it = list->begin(); it != list->end(); ++it) { > > :( https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:203: it != list->end(); On 2012/08/20 20:58:12, Dan Beam wrote: > nit: move ++it; up a line to match style below Done. https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:218: bool rv = dict->GetWithoutPathExpansion(*it, &child); Yes, base/logging.h explicitly says that the |condition| will be "referenced" (as in "false && condition;") to avoid warnings, but will NOT be executed (as in "condition;"). On 2012/08/20 21:52:31, achuith.bhandarkar wrote: > On 2012/08/20 20:58:12, Dan Beam wrote: > > DCHECK(dict->GetWithoutPathExpansion(*it, &child)); > > Don't think you can do that - it would get compiled out in release. https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:230: bool rv = str->GetAsString(&value); Agreed. On 2012/08/20 21:52:31, achuith.bhandarkar wrote: > On 2012/08/20 20:58:12, Dan Beam wrote: > > DCHECK(str->GetAsString(&value)); > > same. https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:333: DCHECK(ppcopy && ppcopy->IsType(base::Value::TYPE_DICTIONARY)); It was 2 statements originally, but but either way we would know that DeepCopyAndResolveStrings is wrong as the input isn't NULL and it's the dictionary. On 2012/08/20 20:58:12, Dan Beam wrote: > you might want to split these into 2 statements so you see which part of this > fails, up to you https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo_mobile_ntp.h:20: class NotificationPromoMobileNtp { The intended purpose of this class is to define a "contract" between the backend server and the client in the interpretation of a payload. I would prefer to keep this class behavior- and state- less. Please think of this class as just an accessor to fetch the data from the NotificationPromo in a structured way. I agree that having NotificationPromoMobileNtp being inherited from NotificationPromo might feel more natural. However, there is no behavior that NotificationPromoMobileNtp could sensibly substitute, and given that NotificationPromo has substantial implementation details, I believe UI handler is a better place to implement a promotion-type-specific behavior that appropriately handles CanShow (depending on the context, previously closed pages on Most Visited etc) and represents the "text" in the manner suitable for the platform. Please chime in if I'm missing something. On 2012/08/20 16:09:47, msarda wrote: > I would have expected this to extend the NotificationPromo and inherit all the > HandleViewed / CanShow methods. Or we are supposed to use this when we need to > parse the NotificationPromo object? https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/promo_resource_service.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:27: // Delay between calls to update the cache (12 hours), and 3 min in debug mode. Originally it was 3 minutes, and I changed it to 10 seconds to facilitate debugging (and I shouldn't have submitted it, my fault). 3 minutes is much better because any mistakes with handling the notification are not get masked out by the quick refreshes. On 2012/08/20 20:58:12, Dan Beam wrote: > why'd you change this? https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/promo_resource_service.cc:183: #if !defined(OS_ANDROID) && !defined(OS_IOS) On 2012/08/20 20:58:12, Dan Beam wrote: > On 2012/08/20 16:09:47, msarda wrote: > > Change the order here: > > #if defined(OS_ANDROID) && defined(OS_IOS) > > NotificationPromo::MOBILE_NTP_SYNC_PROMO; > > #else > > NotificationPromo::NTP_NOTIFICATION_PROMO; > > @endif > > er, I assume you mean > > #if defined(OS_ANDROID) || defined(OS_IOS) > > and > > #endif > > ? Done.
https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo_mobile_ntp.h:20: class NotificationPromoMobileNtp { Agree with Ruslan's sentiments. My first instinct was also to suggest inheritance, but after some thought this approach seemed better for now. We can probably consider some composition approach that would work better, but I think inheriting from NotificationPromo is probably not the right thing to do. On 2012/08/20 22:38:17, aruslan wrote: > The intended purpose of this class is to define a "contract" between the backend > server and the client in the interpretation of a payload. > I would prefer to keep this class behavior- and state- less. > Please think of this class as just an accessor to fetch the data from the > NotificationPromo in a structured way. > > I agree that having NotificationPromoMobileNtp being inherited from > NotificationPromo might feel more natural. > However, there is no behavior that NotificationPromoMobileNtp could sensibly > substitute, and given that NotificationPromo has substantial implementation > details, I believe UI handler is a better place to implement a > promotion-type-specific behavior that appropriately handles CanShow (depending > on the context, previously closed pages on Most Visited etc) and represents the > "text" in the manner suitable for the platform. > > Please chime in if I'm missing something. > > On 2012/08/20 16:09:47, msarda wrote: > > I would have expected this to extend the NotificationPromo and inherit all the > > HandleViewed / CanShow methods. Or we are supposed to use this when we need to > > parse the NotificationPromo object? >
https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo.cc:218: bool rv = dict->GetWithoutPathExpansion(*it, &child); On 2012/08/20 22:38:17, aruslan wrote: > Yes, base/logging.h explicitly says that the |condition| will be "referenced" > (as in "false && condition;") to avoid warnings, but will NOT be executed (as in > "condition;"). > > On 2012/08/20 21:52:31, achuith.bhandarkar wrote: > > On 2012/08/20 20:58:12, Dan Beam wrote: > > > DCHECK(dict->GetWithoutPathExpansion(*it, &child)); > > > > Don't think you can do that - it would get compiled out in release. > I guess we just use CHECK() everywhere in the NTP, which doesn't get compiled out. Ignore me I'm full of it.
https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:436: const base::ListValue* promo_list = NULL; what's the difference here? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:24: if (!payload_->GetString("promo_message_short", &text_)) you could probably combine these together, i.e. if (!payload_ || !payload_->GetString("promo_message_short", &text_) ...) { return; } https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:36: valid_ = true; nit: you're allowed to use \n's you know, ;) https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:24: ~NotificationPromoMobileNtp(); shouldn't this destructor be virtual? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:63: ASSERT_TRUE(value); can you use EXPECT_NE(value, NULL); for these cases (and throughout) or does something complain about some ostream operator not existing? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:105: EXPECT_EQ(std::size_t(2), promo_action_args_.size()); hmmm, I guess I'm used to seeing 2U here instead. is there any reason to do one over the other? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:125: EXPECT_EQ(notification_promo_.prefs_, is there a way you could use the public interface instead of the private one here? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:159: mobile_promo.action_args()->GetString( why does this need to be wrapped so much? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:176: new_promo.closed_ = false; I'm confused as to what this last part is testing? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:181: void TestPromoText() { so this tests that we're not going to show an empty promo? if so, can you update the name of this test to reflect that? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:252: "It\'s simple. Go http://www.google.com/chrome/", nit: why is there a \' ? also, shouldn't it be go to <link>? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:355: // Additionally, it tests that the first string in |strings| is used if nit: I think "it" is slightly ambiguous in this comment; I think it'd probably be safe/better to just remove this word in favor of "Additionally, test that the first string..." https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:356: // no payload.promo_short_message is specified in JSON. nit: "in the JSON" or "in the JSON response" https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:401: // on Android devices with incorrect or unset date/time. ^ I'm confused by this - what does this mean? This is the default date for a newly activated Android device with no timezone settings / never having run `ntpdate` or whatever?
https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/8003/chrome/browser/web_... chrome/browser/web_resource/notification_promo_mobile_ntp.h:20: class NotificationPromoMobileNtp { Let me explain how I am using this. I have a bridge that listens for promo notifications and creates a new NotificationPromo each time there is a change. I use then this object to check out the state of the promo, get the promo payload info and notify it when the promo is viewed / closed etc. After this change I would have 2 options: 1. Create a new NotificationPromoMobileNtp each time I need some info from the promo. This is not good as it will lead to reading the entire payload even though a single piece is needed. 2. Each time I create a new NotificationPromo, I would need to recreate a new object called NotificationPromoMobileNtp that stores the entire payload. I would have preferred not having two objects that represent the same promo, but this solution looks ok. As this really represents only the payload, how about adding a method to the NotificationPromo like "const NotificationPromoPayload* getPayload()" that returns a generic payload object? And the caller would cast it to whatever type it expects (NotificationPromoPayloadMobileNtp and whatever else needed for desktop)? On 2012/08/20 22:45:34, achuith.bhandarkar wrote: > Agree with Ruslan's sentiments. > > My first instinct was also to suggest inheritance, but after some thought this > approach seemed better for now. We can probably consider some composition > approach that would work better, but I think inheriting from NotificationPromo > is probably not the right thing to do. > > On 2012/08/20 22:38:17, aruslan wrote: > > The intended purpose of this class is to define a "contract" between the > backend > > server and the client in the interpretation of a payload. > > I would prefer to keep this class behavior- and state- less. > > Please think of this class as just an accessor to fetch the data from the > > NotificationPromo in a structured way. > > > > I agree that having NotificationPromoMobileNtp being inherited from > > NotificationPromo might feel more natural. > > However, there is no behavior that NotificationPromoMobileNtp could sensibly > > substitute, and given that NotificationPromo has substantial implementation > > details, I believe UI handler is a better place to implement a > > promotion-type-specific behavior that appropriately handles CanShow (depending > > on the context, previously closed pages on Most Visited etc) and represents > the > > "text" in the manner suitable for the platform. > > > > Please chime in if I'm missing something. > > > > On 2012/08/20 16:09:47, msarda wrote: > > > I would have expected this to extend the NotificationPromo and inherit all > the > > > HandleViewed / CanShow methods. Or we are supposed to use this when we need > to > > > parse the NotificationPromo object? > > >
Woohoo; thanks for reviews! Now I'm going to look into Mihai's use-case. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:436: const base::ListValue* promo_list = NULL; Chromium style appears to prefer T* p = NULL vs T* p(NULL). Both are allowed, but given that I change this file anyway, why not to be more consistent. On 2012/08/21 03:53:49, Dan Beam wrote: > what's the difference here? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:24: if (!payload_->GetString("promo_message_short", &text_)) On 2012/08/21 03:53:49, Dan Beam wrote: > you could probably combine these together, i.e. > > if (!payload_ || > !payload_->GetString("promo_message_short", &text_) > ...) { > return; > } Done. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:36: valid_ = true; On 2012/08/21 03:53:49, Dan Beam wrote: > nit: you're allowed to use \n's you know, ;) Done. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:24: ~NotificationPromoMobileNtp(); This class is not intended to be a base class for inheritance. On 2012/08/21 03:53:49, Dan Beam wrote: > shouldn't this destructor be virtual? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:63: ASSERT_TRUE(value); On 2012/08/21 03:53:49, Dan Beam wrote: > can you use > > EXPECT_NE(value, NULL); > > for these cases (and throughout) or does something complain about some ostream > operator not existing? Waiting till NULL is nullptr. Otherwise ./testing/gtest/include/gtest/gtest.h: In function 'testing::AssertionResult testing::internal::CmpHelperNE(char const*, char const*, const T1&, const T2&) [wit h T1 = base::Value*, T2 = int]': ./testing/gtest/include/gtest/gtest.h:1532:1: error: ISO C++ forbids comparison between pointer and integer [-fpermissive] https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:105: EXPECT_EQ(std::size_t(2), promo_action_args_.size()); Just a habit (2u is not size_t). You are right -- grep says that everyone is using 2u. On 2012/08/21 03:53:49, Dan Beam wrote: > hmmm, I guess I'm used to seeing 2U here instead. is there any reason to do one > over the other? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:125: EXPECT_EQ(notification_promo_.prefs_, On 2012/08/21 03:53:49, Dan Beam wrote: > is there a way you could use the public interface instead of the private one > here? Done. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:159: mobile_promo.action_args()->GetString( On 2012/08/21 03:53:49, Dan Beam wrote: > why does this need to be wrapped so much? Done. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:176: new_promo.closed_ = false; My fault -- these two tests are non-mobile-specific and are already in the NotificationPromo test. On 2012/08/21 03:53:49, Dan Beam wrote: > I'm confused as to what this last part is testing? https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:181: void TestPromoText() { On 2012/08/21 03:53:49, Dan Beam wrote: > so this tests that we're not going to show an empty promo? if so, can you update > the name of this test to reflect that? Removed (tested in NotificationPromo). https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:252: "It\'s simple. Go http://www.google.com/chrome/", On 2012/08/21 03:53:49, Dan Beam wrote: > nit: why is there a \' ? also, shouldn't it be go to <link>? Removed unnecessary escaping. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:355: // Additionally, it tests that the first string in |strings| is used if On 2012/08/21 03:53:49, Dan Beam wrote: > nit: I think "it" is slightly ambiguous in this comment; I think it'd probably > be safe/better to just remove this word in favor of "Additionally, test that the > first string..." Thanks! Done. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:356: // no payload.promo_short_message is specified in JSON. On 2012/08/21 03:53:49, Dan Beam wrote: > nit: "in the JSON" or "in the JSON response" Thanks! - Done. https://chromiumcodereview.appspot.com/10860025/diff/14004/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:401: // on Android devices with incorrect or unset date/time. Yes, some of the devices on bots have time in 2001, so the tests were failing there. On 2012/08/21 03:53:49, Dan Beam wrote: > ^ I'm confused by this - what does this mean? This is the default date for a > newly activated Android device with no timezone settings / never having run > `ntpdate` or whatever?
I think this is good for my use case. Thanks for this refactoring.
Excellent, thank you very much for comments and suggestions! sky@ -- please take a look at chrome*.gypi, it adds .h/.cc/unittest to Android. achuith@ -- please review the code bits.
.gypi LGTM
the code lg to me, I don't spot anything particularly large at a glance but I will say that it seems a little hard to read the unittests. any thing to help increase the readability would be appreciated as I think many's eyes will just glaze over when trying to understand what's happening without more comments or a re-organization of the code to make it simpler / more direct. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:214: base::Value* DeepCopyAndResolveStrings( it might be useful to put a TODO() to create a DictionaryValue::LeftJoin() (or some better name) utility method in values.cc/h, IMO if it's generic enough (given dict1 and dict2, create dict3 - a replacement of keys at the same level from dict1 with a copy of dict2's values) https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:7: #include "base/logging.h" probably don't need this any more https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:53: !action_args_) nit: curlies (when conditional or body is over 1 line) https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:58: valid_ = true; what happens if any of the next payload->Get*() calls fail? is this still valid? https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:24: ~NotificationPromoMobileNtp(); nit: if this isn't virtual and just calling the default dtor, you can remove https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:57: would it be worthwhile to make this a struct (so you could pass it around and/or do promo->data()->field rather than having a bazillion getters)? perhaps I just hate how mixins work in C++ / how limited the language is wrt to literals. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:58: bool valid_; it'd seem to be useful to document any of these (that are unique to this class) https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:169: " \"Like Chrome? Go http://www.google.com/chrome/\"," can you indent these so it's apparent they correspond to the key above? https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:508: " \"GOOD_STRING\":" same nit re: ident
Very close. Should be the last round https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:125: // TODO(achuith): http://crbug.com/143773 remove this by m24 thank you! https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:127: prefs->RegisterStringPref(prefs::kNtpPromoLine, Do you think you could do the same thing you're doing for kNtpPromoLineLong for the rest of these prefs? That is, move kNtpPromoLine, etc from pref_names.* to within this function? Sorry to be a bother, but I quite like it because it makes the cleanup much easier (just need to delete this function and not muck around with pref_names). https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:447: const base::DictionaryValue* dv = NULL; do you mind calling this var payload instead? https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo.h (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.h:100: // Note that |payload| isn't currently used for desktop promos. promo_payload_. You should probably drop promo_. I don't think we use |foo| notation for members, just for function args. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:54: return valid_; Let's return false here. No need to be indirect. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:57: // All the fields below are optional and used to override the defaults. Which comment is true? https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:61: &requires_mobile_only_sync_); let's line this up with " https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:66: &show_as_virtual_computer_); let's line this up with the " https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:69: &virtual_computer_lastsync_); let's line this up with "
https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:57: On 2012/08/22 19:43:27, Dan Beam wrote: > would it be worthwhile to make this a struct (so you could pass it around and/or > do promo->data()->field rather than having a bazillion getters)? perhaps I just > hate how mixins work in C++ / how limited the language is wrt to literals. We could have a nested struct to hold all these variables - that's a good idea, but I think we do need the outer class.
Scott - thanks! Dan and Achuith -- thanks for the comments! PTAL at the changes. I agree that unit tests are not easily comprehensible; I'll file a bug to find a way of rewriting unit tests for NotificationPromo/PromoResourceService and for NotificationPromoMobileNtp. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:125: // TODO(achuith): http://crbug.com/143773 remove this by m24 On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > thank you! You are welcome! https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:127: prefs->RegisterStringPref(prefs::kNtpPromoLine, On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > Do you think you could do the same thing you're doing for kNtpPromoLineLong for > the rest of these prefs? That is, move kNtpPromoLine, etc from pref_names.* to > within this function? Sorry to be a bother, but I quite like it because it makes > the cleanup much easier (just need to delete this function and not muck around > with pref_names). Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:214: base::Value* DeepCopyAndResolveStrings( On 2012/08/22 19:43:27, Dan Beam wrote: > it might be useful to put a TODO() to create a DictionaryValue::LeftJoin() (or > some better name) utility method in values.cc/h, IMO if it's generic enough > (given dict1 and dict2, create dict3 - a replacement of keys at the same level > from dict1 with a copy of dict2's values) TODO+http://crbug.com/144320 added. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.cc:447: const base::DictionaryValue* dv = NULL; On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > do you mind calling this var payload instead? Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo.h (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo.h:100: // Note that |payload| isn't currently used for desktop promos. On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > promo_payload_. You should probably drop promo_. I don't think we use |foo| > notation for members, just for function args. Done -- but I kept it promo_payload as it's one of three visible externally -- promo_text(), promo_type() and promo_payload(). https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:7: #include "base/logging.h" On 2012/08/22 19:43:27, Dan Beam wrote: > probably don't need this any more Thanks! Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:53: !action_args_) On 2012/08/22 19:43:27, Dan Beam wrote: > nit: curlies (when conditional or body is over 1 line) Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:54: return valid_; On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > Let's return false here. No need to be indirect. Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:57: // All the fields below are optional and used to override the defaults. On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > Which comment is true? Both; I changed the comments to be more comprehensible. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:58: valid_ = true; On 2012/08/22 19:43:27, Dan Beam wrote: > what happens if any of the next payload->Get*() calls fail? is this still valid? Yes; I changed the comments to make it more apparent. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:61: &requires_mobile_only_sync_); On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > let's line this up with " Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:66: &show_as_virtual_computer_); On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > let's line this up with the " Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.cc:69: &virtual_computer_lastsync_); On 2012/08/22 19:49:20, achuith.bhandarkar wrote: > let's line this up with " Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:24: ~NotificationPromoMobileNtp(); On 2012/08/22 19:43:27, Dan Beam wrote: > nit: if this isn't virtual and just calling the default dtor, you can remove Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:57: On 2012/08/22 19:43:27, Dan Beam wrote: > would it be worthwhile to make this a struct (so you could pass it around and/or > do promo->data()->field rather than having a bazillion getters)? perhaps I just > hate how mixins work in C++ / how limited the language is wrt to literals. I like the idea; my attempt to implement it resulted in not-so-pretty cruft here, and the repetitive promo->data()->field instead of promo->field() on the client side; neither looked pretty. I'm not opposing the idea, though. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:58: bool valid_; On 2012/08/22 19:43:27, Dan Beam wrote: > it'd seem to be useful to document any of these (that are unique to this class) Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:169: " \"Like Chrome? Go http://www.google.com/chrome/\"," On 2012/08/22 19:43:27, Dan Beam wrote: > can you indent these so it's apparent they correspond to the key above? Done. https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/promo_resource_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/promo_resource_service_unittest.cc:508: " \"GOOD_STRING\":" On 2012/08/22 19:43:27, Dan Beam wrote: > same nit re: ident Done.
https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... File chrome/browser/web_resource/notification_promo_mobile_ntp.h (right): https://chromiumcodereview.appspot.com/10860025/diff/22002/chrome/browser/web... chrome/browser/web_resource/notification_promo_mobile_ntp.h:57: On 2012/08/23 00:49:59, aruslan wrote: > On 2012/08/22 19:43:27, Dan Beam wrote: > > would it be worthwhile to make this a struct (so you could pass it around > and/or > > do promo->data()->field rather than having a bazillion getters)? perhaps I > just > > hate how mixins work in C++ / how limited the language is wrt to literals. > > I like the idea; my attempt to implement it resulted in not-so-pretty cruft > here, and the repetitive promo->data()->field instead of promo->field() on the > client side; neither looked pretty. I'm not opposing the idea, though. OK, that's fine. C++ isn't a pretty language.
Please remove promo_resource_service.cc from this CL. LGTM. Thanks for this! http://codereview.chromium.org/10860025/diff/18004/chrome/common/pref_names.h File chrome/common/pref_names.h (left): http://codereview.chromium.org/10860025/diff/18004/chrome/common/pref_names.h... chrome/common/pref_names.h:548: extern const char kNtpPromoBuild[]; Please remove these two as well. This is cruft that got left behind.
Thank you!! http://codereview.chromium.org/10860025/diff/18004/chrome/common/pref_names.h File chrome/common/pref_names.h (left): http://codereview.chromium.org/10860025/diff/18004/chrome/common/pref_names.h... chrome/common/pref_names.h:548: extern const char kNtpPromoBuild[]; On 2012/08/23 06:01:24, achuith.bhandarkar wrote: > Please remove these two as well. This is cruft that got left behind. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aruslan@chromium.org/10860025/32001
Try job failure for 10860025-32001 (retry) on mac_rel for step "interactive_ui_tests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aruslan@chromium.org/10860025/32001
Try job failure for 10860025-32001 (retry) on win_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aruslan@chromium.org/10860025/32001
Change committed as 153180 |
