Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10882024:
Add webui handler for promotions on Android NTP. (Closed)
|
Created:
8 years, 4 months ago by aruslan Modified:
8 years, 3 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv (Not doing code reviews) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd webui handler for promotions on Android NTP.
Add promotions support on Android NTP.
BUG=144565
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154540
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 59
Patch Set 3 : Addressing the review comments. #
Total comments: 13
Patch Set 4 : Review comments addressed; UMA actions file added. #Patch Set 5 : Whitespace merge fail. #
Total comments: 7
Patch Set 6 : Histograms for "shown", UMA for "action"; code style. #Patch Set 7 : Histograms for "shown", UMA for "action"; code style. #Patch Set 8 : Rebase. #
Total comments: 7
Patch Set 9 : Histograms for all events. #
Total comments: 20
Patch Set 10 : Addressed review comments. #Patch Set 11 : Forgotten images added. #Patch Set 12 : -webkit-image-set actually appears to work. Thanks, Dan! #Patch Set 13 : Missed a call during the code rearranging. #Patch Set 14 : Removing images (added separately in https://chromiumcodereview.appspot.com/10905035/) #Messages
Total messages: 27 (0 generated)
estade and dbeam for webui/ntp code -- please help with the review. I will add OWNERS for gypi etc after the initial round of reviews.
https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... File chrome/browser/resources/ntp_android/ntp_android.css (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.css:47: margin: 16px 24px 16px 24px; nit: margin: 16px 24x; (shorter and equivalent) https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.css:61: padding: 10px 10px; nit: padding: 10px; (shorter and equivalent again :D) https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:867: title.id = item.titleId; nit: title.id = item.titleId || ''; (shorter and equivalent, or just item.titleId if it's a blank string) https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1012: (!hasRecentlyClosedTabs); nit: the style guide says don't use extra (), these technically are extra, so use judgement if removing them wouldn't hurt the readability of the code https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1060: mostVisitedEl.style.display = mostVisitedEl.hidden = !shouldPromoBeShownOnMostVisited(); https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1091: promoIsAllowed = promotions['promoIsAllowed'] === true; you should use . notation instead of [''] notation if possible https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1097: mostVisitedEl.innerHTML = promotions['promoMessage'] || ''; this is bad if there's a <script> in the message, can we just use .textContent instead? if not, the NTP has something that parses stuff and looks for only certain nodes and certain attributes which would probably be best to use <http://goo.gl/ju0fH>. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1115: promoButtonEls[i].href = 'javascript:void(0)'; hmm, why is this necessary? why not just '#' and do e.preventDefault(); in the onclick handler? https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1615: var list = findList('promo_vc'); I'm scared of code without newlines :( https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1858: case SectionType.PROMO_VC_SESSION_HEADER: I'm slightly confused by this, why can't this just be a map instead of a switch? also, you can switch on strings in JS... https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:2435: chrome.send('showContextMenu', [ this is super repetitive, why not do something like: var details; // bajillion if statements, hopefully setting details to something useful if (details) chrome.send('showContextMenu', details); https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:32: namespace { nit: \n https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:81: } nit: \n https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:106: if (profile) { if this is the norm, might as well change to DCHECK() https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:107: // Watch for pref changes that cause us to need to re-inject promolines. why do you need to do this in registermessages rather than ctor? https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:134: } } else { NOTREACHED() << "Unknown pref changed."; } also, I <3 switch statements https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:146: LOG(ERROR) << "promoSendEmail: expected three args, got " probably DLOG or DVLOG(##) here, eh? https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:162: chrome::android::SendEmail( cool! https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:209: if (args && !args->empty()) if you expect something be passed to this, perhaps DCHECK(!args->empty()) is better as it shows where something's misbehaving sooner https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:219: NotificationPromo::MOBILE_NTP_SYNC_PROMO)) nit: curlies https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:222: if (!id.compare("most_visited")) nit: curlies on every level https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:244: if (!profile || !CanShowNotificationPromo(profile)) same general gist on the if () -> DCHECK() if applicable https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:260: !UTF8ToUTF16(utf8long.c_str(), utf8long.length(), &promo_line_long)) nit: curlies https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:263: promo_line = ReplaceSimpleMarkupWithHTML(promo_line); WithHtml would probably be the preferred capitalization https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:285: // If the promo doesn't require any sync, we are good to go. nit: some prefer not to use "we", "you", or "us" in comments as they believe it's hard to know exactly what the original author meant by this. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:287: return true; nit: \n after every if that might return from the method, IMO https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:314: return; // already got a desktop session. nit: Already https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/promo_handler.h (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.h:36: // Args: [ subject, body, app-chooser-message ]. nit: can you remove [ ] or just make this less ambiguous between chrome.send('msg', [arg,arg]); and chrome.send('msg', [[arg, arg]]); https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.h:52: // Args: [ ("most_visited" | "open_tabs" | "sync_promo") ]. probably better to just say "a page to record an impression from" to not make this list updated every time and/or make this an enum (then do a lot of if/else checking the string value itself in C++ and mapping to the enum, :/)
Thanks, Dan; please take a look at the changes! https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... File chrome/browser/resources/ntp_android/ntp_android.css (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.css:47: margin: 16px 24px 16px 24px; On 2012/08/25 00:48:20, Dan Beam wrote: > nit: margin: 16px 24x; (shorter and equivalent) Done - thanks! https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.css:61: padding: 10px 10px; On 2012/08/25 00:48:20, Dan Beam wrote: > nit: padding: 10px; (shorter and equivalent again :D) Done - thanks! https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:867: title.id = item.titleId; On 2012/08/25 00:48:20, Dan Beam wrote: > nit: title.id = item.titleId || ''; > (shorter and equivalent, or just item.titleId if it's a blank string) Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1012: (!hasRecentlyClosedTabs); On 2012/08/25 00:48:20, Dan Beam wrote: > nit: the style guide says don't use extra (), these technically are extra, so > use judgement if removing them wouldn't hurt the readability of the code Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1060: mostVisitedEl.style.display = On 2012/08/25 00:48:20, Dan Beam wrote: > mostVisitedEl.hidden = !shouldPromoBeShownOnMostVisited(); The rest of the code uses style.display, and there is no reason for the promo code to be inconsistent; let me find out whether this was an accident or the intent, so that I could clean it up in a subsequent CL. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1091: promoIsAllowed = promotions['promoIsAllowed'] === true; On 2012/08/25 00:48:20, Dan Beam wrote: > you should use . notation instead of [''] notation if possible Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1097: mostVisitedEl.innerHTML = promotions['promoMessage'] || ''; On 2012/08/25 00:48:20, Dan Beam wrote: > this is bad if there's a <script> in the message, can we just use .textContent > instead? if not, the NTP has something that parses stuff and looks for only > certain nodes and certain attributes which would probably be best to use > <http://goo.gl/ju0fH>. Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1115: promoButtonEls[i].href = 'javascript:void(0)'; On 2012/08/25 00:48:20, Dan Beam wrote: > hmm, why is this necessary? why not just '#' and do e.preventDefault(); in the > onclick handler? Done -- thanks! https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1615: var list = findList('promo_vc'); On 2012/08/25 00:48:20, Dan Beam wrote: > I'm scared of code without newlines :( Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1858: case SectionType.PROMO_VC_SESSION_HEADER: On 2012/08/25 00:48:20, Dan Beam wrote: > I'm slightly confused by this, why can't this just be a map instead of a switch? > also, you can switch on strings in JS... Done -- as this is not an enum, changed it to be strings everywhere. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:2435: chrome.send('showContextMenu', [ On 2012/08/25 00:48:20, Dan Beam wrote: > this is super repetitive, why not do something like: > > var details; > > // bajillion if statements, hopefully setting details to something useful > > if (details) > chrome.send('showContextMenu', details); Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:32: namespace { On 2012/08/25 00:48:20, Dan Beam wrote: > nit: \n Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:81: } On 2012/08/25 00:48:20, Dan Beam wrote: > nit: \n Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:106: if (profile) { On 2012/08/25 00:48:20, Dan Beam wrote: > if this is the norm, might as well change to DCHECK() Good point; the profile is not even necessary here. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:107: // Watch for pref changes that cause us to need to re-inject promolines. On 2012/08/25 00:48:20, Dan Beam wrote: > why do you need to do this in registermessages rather than ctor? Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:134: } On 2012/08/25 00:48:20, Dan Beam wrote: > } else { > NOTREACHED() << "Unknown pref changed."; > } > > also, I <3 switch statements Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:146: LOG(ERROR) << "promoSendEmail: expected three args, got " On 2012/08/25 00:48:20, Dan Beam wrote: > probably DLOG or DVLOG(##) here, eh? Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:162: chrome::android::SendEmail( On 2012/08/25 00:48:20, Dan Beam wrote: > cool! :) https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:209: if (args && !args->empty()) On 2012/08/25 00:48:20, Dan Beam wrote: > if you expect something be passed to this, perhaps DCHECK(!args->empty()) is > better as it shows where something's misbehaving sooner Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:219: NotificationPromo::MOBILE_NTP_SYNC_PROMO)) On 2012/08/25 00:48:20, Dan Beam wrote: > nit: curlies Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:222: if (!id.compare("most_visited")) On 2012/08/25 00:48:20, Dan Beam wrote: > nit: curlies on every level Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:244: if (!profile || !CanShowNotificationPromo(profile)) On 2012/08/25 00:48:20, Dan Beam wrote: > same general gist on the if () -> DCHECK() if applicable I'll find out more about circumstances except the tests where we could have a NULL profile. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:260: !UTF8ToUTF16(utf8long.c_str(), utf8long.length(), &promo_line_long)) On 2012/08/25 00:48:20, Dan Beam wrote: > nit: curlies Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:263: promo_line = ReplaceSimpleMarkupWithHTML(promo_line); On 2012/08/25 00:48:20, Dan Beam wrote: > WithHtml would probably be the preferred capitalization Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:285: // If the promo doesn't require any sync, we are good to go. On 2012/08/25 00:48:20, Dan Beam wrote: > nit: some prefer not to use "we", "you", or "us" in comments as they believe > it's hard to know exactly what the original author meant by this. Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:287: return true; On 2012/08/25 00:48:20, Dan Beam wrote: > nit: \n after every if that might return from the method, IMO Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:314: return; // already got a desktop session. On 2012/08/25 00:48:20, Dan Beam wrote: > nit: Already Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/promo_handler.h (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.h:36: // Args: [ subject, body, app-chooser-message ]. On 2012/08/25 00:48:20, Dan Beam wrote: > nit: can you remove [ ] or just make this less ambiguous between > chrome.send('msg', [arg,arg]); and chrome.send('msg', [[arg, arg]]); Done. https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.h:52: // Args: [ ("most_visited" | "open_tabs" | "sync_promo") ]. On 2012/08/25 00:48:20, Dan Beam wrote: > probably better to just say "a page to record an impression from" to not make > this list updated every time and/or make this an enum (then do a lot of if/else > checking the string value itself in C++ and mapping to the enum, :/) Done - changed the comment.
https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/2001/chrome/browser/reso... chrome/browser/resources/ntp_android/ntp_android.js:1060: mostVisitedEl.style.display = On 2012/08/27 22:19:51, aruslan wrote: > On 2012/08/25 00:48:20, Dan Beam wrote: > > mostVisitedEl.hidden = !shouldPromoBeShownOnMostVisited(); > > The rest of the code uses style.display, and there is no reason for the promo > code to be inconsistent; let me find out whether this was an accident or the > intent, so that I could clean it up in a subsequent CL. it's probably not intentional, either way it's fine that we change later, I just worry about how many times something has been added in the same way with intention to possibly change later but have instead helped reinforce the convention, ;)
will re-review in a little while, busy right now
didn't get to this today, will look tomorrow morning
https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:224: if (!id.compare("most_visited")) { use == https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:226: content::UserMetricsAction("MobilePromoShownOnMostVisited")); please factor out the content::RecordAction(content::UserMetricsAction bit https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:260: const std::string utf8 = promo.text(); I don't know why you are bothering with the temporary variables but if you're going to go that route use a const ref. https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:264: !UTF8ToUTF16(utf8long.c_str(), utf8long.length(), &promo_line_long)) { actually, why are you even converting to utf16? when it's serialized to json it will be converted back to utf8 anyway https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:265: return false; probably a suitable place for an error log. https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:273: promo.show_on_most_visited()); indent to align with previous arg.
Thanks for the review; please take a look at the cleaned-up version. I'll look if UMA extraction could be done cleaner, but this is what people use elsewhere. https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:224: if (!id.compare("most_visited")) { On 2012/08/28 17:25:55, Evan Stade wrote: > use == Done. https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:226: content::UserMetricsAction("MobilePromoShownOnMostVisited")); On 2012/08/28 17:25:55, Evan Stade wrote: > please factor out the content::RecordAction(content::UserMetricsAction bit This copy-pasted mess is believed to be required by the UMA extract_actions tool. I've just asked people if that's still the case, and it appears so: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/use... https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:260: const std::string utf8 = promo.text(); On 2012/08/28 17:25:55, Evan Stade wrote: > I don't know why you are bothering with the temporary variables but if you're > going to go that route use a const ref. Done. https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:264: !UTF8ToUTF16(utf8long.c_str(), utf8long.length(), &promo_line_long)) { On 2012/08/28 17:25:55, Evan Stade wrote: > actually, why are you even converting to utf16? when it's serialized to json it > will be converted back to utf8 anyway Done. https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:265: return false; On 2012/08/28 17:25:55, Evan Stade wrote: > probably a suitable place for an error log. Done. https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:273: promo.show_on_most_visited()); On 2012/08/28 17:25:55, Evan Stade wrote: > indent to align with previous arg. Done.
https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/3005/chrome/browser/ui/w... chrome/browser/ui/webui/ntp/android/promo_handler.cc:226: content::UserMetricsAction("MobilePromoShownOnMostVisited")); On 2012/08/28 19:16:49, aruslan wrote: > On 2012/08/28 17:25:55, Evan Stade wrote: > > please factor out the content::RecordAction(content::UserMetricsAction bit > > This copy-pasted mess is believed to be required by the UMA extract_actions > tool. I've just asked people if that's still the case, and it appears so: > http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/use... ok, no problem. Thanks for the explanation. https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/res... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:2385: menuOptions = [ here is how to format multi-line javascript array literals: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.cc:216: content::UserMetricsAction("MobilePromoShownOnMostVisited")); this seems like the kind of thing that should go into a histogram instead. https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.cc:287: return !prefs || this doesn't fit on one line?
https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/res... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:2385: menuOptions = [ my reading is: menuOptions = [ [ ContextMenuItemIds.BOOKMARK_OPEN_IN_NEW_TAB, templateData.elementopeninnewtab ], [ ContextMenuItemIds.BOOKMARK_OPEN_IN_INCOGNITO_TAB, templateData.elementopeninincognitotab ] ];
Thanks for the reviews, Evan! I changed the code to use histograms for "shown" and UMA actions for "user actions". Please take a look at the changed code. https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/res... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:2385: menuOptions = [ On 2012/08/28 22:37:28, Evan Stade wrote: > my reading is: > > menuOptions = [ > [ > ContextMenuItemIds.BOOKMARK_OPEN_IN_NEW_TAB, > templateData.elementopeninnewtab > ], > [ > ContextMenuItemIds.BOOKMARK_OPEN_IN_INCOGNITO_TAB, > templateData.elementopeninincognitotab > ] > ]; > Done. https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.cc:216: content::UserMetricsAction("MobilePromoShownOnMostVisited")); On 2012/08/28 20:29:53, Evan Stade wrote: > this seems like the kind of thing that should go into a histogram instead. Done -- good point, thanks! https://chromiumcodereview.appspot.com/10882024/diff/20001/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.cc:287: return !prefs || On 2012/08/28 20:29:53, Evan Stade wrote: > this doesn't fit on one line? Done.
dbeam@, estade@ -- thank you for the reviews! Please take a look at the changed code bits. sky@ -- please take a look at chrome/chrome_browser.gypi, chrome/common/pref_names.* and chrome/tools/chromeactions.txt. It adds the mobile NTP promotion handler, its user pref and UMA actions. If a pref is used strictly internally in this one feature, should the pref be put it to chrome/common/pref_names at all or I better remove its name from pref_names? battre@ -- please take a look at chrome/browser/prefs/browser_prefs.cc -- it adds the user prefs registration for mobile NTP promo handler.
I did not do a full review, but I was confused that you add a promo related preference if the other promo related preferences are managed by the PromoResourceService. Anyway, I leave this to the main reviewers. The general pattern for adding the preference is correct. So LGTM on that. https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/pre... File chrome/browser/prefs/browser_prefs.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/pre... chrome/browser/prefs/browser_prefs.cc:241: #if defined(OS_ANDROID) any reason for not combining this with the previous #if? https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.h (right): https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:18: public content::NotificationObserver { I wonder whether some of this functionality should go into the PromoResourceService. Have you considered this?
LGTM
Thanks, Scott and Dominic! Dominic -- if you'd prefer to have the #if merged with the previous one, I can do it; just let me know. I want to avoid unnecessary "already known" conflicts while we upstream and downmerge. https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/pre... File chrome/browser/prefs/browser_prefs.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/pre... chrome/browser/prefs/browser_prefs.cc:241: #if defined(OS_ANDROID) On 2012/08/29 21:04:19, battre wrote: > any reason for not combining this with the previous #if? Yes, this is to avoid unnecessary conflicts during the merge -- the previous one is going to be either removed or moved. I can merge them if you'd prefer that. https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.h (right): https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:18: public content::NotificationObserver { On 2012/08/29 21:04:19, battre wrote: > I wonder whether some of this functionality should go into the > PromoResourceService. Have you considered this? Absolutely; majority of the code in this handler is Android- or Android-NTP specific (and handled differently on iOS); handling of the sync sessions is too narrow and too specific to be put into PromoResourceService. In fact, it all started within the PromoResourceService :)
https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/pre... File chrome/browser/prefs/browser_prefs.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/pre... chrome/browser/prefs/browser_prefs.cc:241: #if defined(OS_ANDROID) On 2012/08/29 21:52:53, aruslan wrote: > On 2012/08/29 21:04:19, battre wrote: > > any reason for not combining this with the previous #if? > > Yes, this is to avoid unnecessary conflicts during the merge -- the previous one > is going to be either removed or moved. > I can merge them if you'd prefer that. Up to you. If it gets moved/deleted soon anyway there is no need to merge it.
https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.cc:241: content::UserMetricsAction("MobilePromoClosePressedOnOpenTabs")); we actually still use histograms for actions most of the time. The reason is that a histogram gives you some context (this many clicks on this button vs. this many clicks on this other button), and also because it allows tracking across different versions instead of lumping all stats together. That said, maybe my information/understanding is out of date. You should ask someone like isherman.
Thanks, Evan. I talked with Ilya and removed UMA actions entirely. https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.cc (right): https://chromiumcodereview.appspot.com/10882024/diff/12002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.cc:241: content::UserMetricsAction("MobilePromoClosePressedOnOpenTabs")); On 2012/08/29 22:17:06, Evan Stade wrote: > we actually still use histograms for actions most of the time. The reason is > that a histogram gives you some context (this many clicks on this button vs. > this many clicks on this other button), and also because it allows tracking > across different versions instead of lumping all stats together. That said, > maybe my information/understanding is out of date. You should ask someone like > isherman. Done -- Ilya helped a lot; I should have had talked with him long before. Thanks!
https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:258: RECENTLY_CLOSED: 'recently_closed', nit: alpha sort now https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:1110: promoIsAllowed = promotions.promoIsAllowed === true; !!promotions.promoIsAllowed or Boolean(promotions.promoIsAllowed) unless you really need to check for type (which I don't think you need to/should, really...) https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:1123: if (openTabsVCTitleEl) is it valid to not have these elements? if no, I wouldn't guard them with an if, IMO, as it just hides errors (well, at least while developing I guess...) https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:1131: document.getElementsByClassName('promo-button'); nit: I prefer document.querySelectorAll('.promo-button') as it's not live (i.e. it needs to keep track of whether a new node with this class has been added into the whole document whenever you access this list) and/or do a relative query if all these are going to be inside a node somewhere (i.e. rootNode.querySelectorAll()) https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.h (right): https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:8: #include "base/memory/scoped_ptr.h" nit: probably not needed now, eh? https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:9: #include "base/values.h" nit: change to forwards, i.e. namespace base { class DictionaryValue; class List; } https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:16: // The handler for Javascript messages related to the Android NTP promo. nit: JavaScript https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:23: // WebUIMessageHandler override and implementation. nit: probably don't need "override and" https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:40: // |args| is not used, could be empty or NULL. nit: just "No arguments." https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:49: void InjectPromoDecorations(const base::ListValue* args); why isn't this named HandleGetPromotions?
Thank you, Dan! Hope it looks cleaner now! :) https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... File chrome/browser/resources/ntp_android/ntp_android.js (right): https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:258: RECENTLY_CLOSED: 'recently_closed', On 2012/08/30 18:52:26, Dan Beam wrote: > nit: alpha sort now Done. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:1110: promoIsAllowed = promotions.promoIsAllowed === true; On 2012/08/30 18:52:26, Dan Beam wrote: > !!promotions.promoIsAllowed or Boolean(promotions.promoIsAllowed) unless you > really need to check for type (which I don't think you need to/should, > really...) Done. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:1123: if (openTabsVCTitleEl) On 2012/08/30 18:52:26, Dan Beam wrote: > is it valid to not have these elements? if no, I wouldn't guard them with an if, > IMO, as it just hides errors (well, at least while developing I guess...) Yes, it's valid not to have this element; those that must have been injected or should be present in HTML go without the guard. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/res... chrome/browser/resources/ntp_android/ntp_android.js:1131: document.getElementsByClassName('promo-button'); On 2012/08/30 18:52:26, Dan Beam wrote: > nit: I prefer document.querySelectorAll('.promo-button') as it's not live (i.e. > it needs to keep track of whether a new node with this class has been added into > the whole document whenever you access this list) and/or do a relative query if > all these are going to be inside a node somewhere (i.e. > rootNode.querySelectorAll()) Good point regarding Selector APIs (couldn't say it's non-controversial). I filed a bug (for this and for .hidden vs .style.display) so that we don't forget to reconsider/fix. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... File chrome/browser/ui/webui/ntp/android/promo_handler.h (right): https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:8: #include "base/memory/scoped_ptr.h" On 2012/08/30 18:52:26, Dan Beam wrote: > nit: probably not needed now, eh? Done. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:9: #include "base/values.h" On 2012/08/30 18:52:26, Dan Beam wrote: > nit: change to forwards, i.e. > > namespace base { > class DictionaryValue; > class List; > } Done. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:16: // The handler for Javascript messages related to the Android NTP promo. On 2012/08/30 18:52:26, Dan Beam wrote: > nit: JavaScript Done. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:23: // WebUIMessageHandler override and implementation. On 2012/08/30 18:52:26, Dan Beam wrote: > nit: probably don't need "override and" Done. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:40: // |args| is not used, could be empty or NULL. On 2012/08/30 18:52:26, Dan Beam wrote: > nit: just "No arguments." Done. https://chromiumcodereview.appspot.com/10882024/diff/25002/chrome/browser/ui/... chrome/browser/ui/webui/ntp/android/promo_handler.h:49: void InjectPromoDecorations(const base::ListValue* args); On 2012/08/30 18:52:26, Dan Beam wrote: > why isn't this named HandleGetPromotions? Done.
lgtm
Thank you for the reviews and suggestions!
lgtm
you can remove the images from this CL now
On 2012/08/31 01:47:24, Dan Beam wrote: > you can remove the images from this CL now Thanks a ton!!! Will do.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aruslan@chromium.org/10882024/39002
Change committed as 154540 |
