Issue 11627039: Added ad counting functionality (Closed)
|
Created:
Sept. 11, 2013, 3:10 p.m. by Thomas Greiner Modified:
Sept. 27, 2013, 8:39 a.m. Visibility:
Public. |
DescriptionI created a stats module which can be used for more than just counting how many requests were blocked because we intend to add more data in the future.
Due to sendRequest and onRequest being deprecated a future commit should update existing parts in the code which use those to use ChromeCompat which is introduced in this commmit.
Patch Set 1 #
Total comments: 28
Patch Set 2 : Changed strings & using appLocale #
Total comments: 14
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
MessagesTotal messages: 14
I created a stats module which can be used for more than just counting how many requests were blocked because we intend to add more data in the future. Due to sendRequest and onRequest being deprecated a future commit should update existing parts in the code which use those to use ChromeCompat which is introduced in this commmit. http://codereview.adblockplus.org/11627039/diff/1/stats.js File stats.js (right): http://codereview.adblockplus.org/11627039/diff/1/stats.js#newcode127 stats.js:127: // Easter Egg Let me know what you think of it (context: https://www.google.com/search?tbm=isch&q=over+9000).
Only reviewed the strings since we want to have a string freeze today. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.json File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:116: "message": "over $number$!", I love this :D Wondering whether we should put the exclamation mark in parentheses: "I blocked over 9000(!) ads and trackers thanks to Adblock Plus" http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:138: "message": "I blocked _TOTAL_ ads and trackers thanks to Adblock Plus" I think this needs a bit of work, probably better discussed on the forum or in a quick meeting than in a review IMO. Anyway, my angle: We have the chance to make these numbers less bogus here by saying something like: "Adblock Plus blocked _TOTAL_ ad and tracking elements". But is that too technical? http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:146: "message": "Blocked ads:" As above, might want to be a bit more precise here. "Blocked ad and tracking elements"? Not sure.
http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.json File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:116: "message": "over $number$!", On 2013/09/17 09:41:04, Felix H. Dahlke wrote: > I love this :D > > Wondering whether we should put the exclamation mark in parentheses: > "I blocked over 9000(!) ads and trackers thanks to Adblock Plus" Done. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:138: "message": "I blocked _TOTAL_ ads and trackers thanks to Adblock Plus" We decided on keeping it simple. Even saying "tracking elements" isn't really accurate due to it only counting blocked requests. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:146: "message": "Blocked ads:" As above we want to keep it simple even though I agree that this is not accurately explaining what the numbers mean, adding more text would clutter the UI.
Publishing my comments on the strings first since that's the urgent part. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.json File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:113: }, No suffixes please - languages other than English can have an entirely different word order, what you consider a suffix won't be a suffix there (try translating into German). Note that even in English these suffixes aren't placed correctly. In general, you should never build together sentences in the application. The translators should always deal with full sentences. In other words, we want to have two sentences translated: I blocked $number$ ads on $page$ thanks to Adblock Plus. I blocked $number$ ads in total thanks to Adblock Plus. A few things to note: * "on this page" lacks context information, the page needs to be mentioned explicitly. * Sentences have to end with a period. * A proper placeholder is used for the number here. I don't know why you decided to introduce your own placeholder format while we are discussing merging the two that currently exist. * Inserting a number into a sentence is generally problematic and should be avoided if somehow possible. In English we can assume that we always need to use plural - the rules are way more complicated in other languages however and are impossible to capture in a single translation. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:116: "message": "over $number$!", Note that the joke will be lost in translation. Also, simply replacing "$number$" with "over $number$(!)" might not be possible in every language due to inflections of other words. The description should explain what this is about and suggest translating this with "$number$" if in doubt. In general, the description shouldn't be vague - the translators need to understand what they are translating: "For some numbers the stats share message will say 'I blocked over 123 ads' instead of 'I blocked 123 ads'. This is a reference to the 'over 9000' meme. This string can be translated simply with '$number$' if that meme isn't known to the speakers of the language or if the resulting sentence would sound weird otherwise." http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:124: "description": "Label for link to adblockplus.org", "Label for the link to adblockplus.org in the share message"? http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:129: "message": "Share on $network$", We've inherited some overly wordy and useless message descriptions from AdThwart - we don't really need to do it like that however. IMHO it's pretty obvious what's being translated here. The only piece of information required concerns the $network$ placeholder - what does it represent? Yet I don't think that we should have that placeholder at all. Depending on language the network name can be inflected. In other words, we should have multiple messages: "Share on Facebook", "Share on Twitter" etc. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:137: "description": "This is the message that can be shared through the popup.", Again, that description doesn't contain any useful information. How about: "This message will be posted to social networks if the user decides to share his stats." http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:141: "description": "This is the label for the share action in the popup.", I think this description needs to explain the difference between stats_share_title and stats_share_label. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:145: "description": "This is the title for the stats section in the popup.", "Title of the stats section in the popup" should be enough as a description (the shorter it is, the more likely that people will actually read it).
My comments on the background page code below. UI code still needs to be reviewed. http://codereview.adblockplus.org/11627039/diff/1/include.preload.js File include.preload.js (right): http://codereview.adblockplus.org/11627039/diff/1/include.preload.js#newcode124 include.preload.js:124: }; Seeing that the content script is merely used to store data here - this isn't really necessary. The blocking happens in the background page, it's logical to store the data there as well. webrequest.js already builds up a data structure for tabs/frames. You can call getFrameData(tabId, 0) to get the data for the top-level frame and add/update the "blockedCount" property there. http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js#newcode37 lib/prefs.js:37: stats_total: {} Why store an object when we really need only a single number? http://codereview.adblockplus.org/11627039/diff/1/lib/stats.js File lib/stats.js (right): http://codereview.adblockplus.org/11627039/diff/1/lib/stats.js#newcode55 lib/stats.js:55: } The exported API here seems overly complicated for the job. If anything, that module only needs to export a single getStats() function: let getStats = exports.getStats = function getStats(tabId) { if (tabId) { let frameData = getFrameData(tabId, 0); return (frameData && "blocked" in frameData ? frameData.blocked : 0); } else return Prefs.stats_total; }; StatsObject and Stats objects aren't really necessary, and neither is caching the prefs value in a local variable. http://codereview.adblockplus.org/11627039/diff/1/lib/stats.js#newcode66 lib/stats.js:66: total.blocked++; Prefs.stats_total++; will do here, no point caching that value. http://codereview.adblockplus.org/11627039/diff/1/lib/stats.js#newcode71 lib/stats.js:71: }, null); Simpler: let frameData = getFrameData(tabId, 0); if (frameData) { if ("blocked" in frameData) frameData.blocked++; else frameData.blocked = 1; } http://codereview.adblockplus.org/11627039/diff/1/webrequest.js File webrequest.js (right): http://codereview.adblockplus.org/11627039/diff/1/webrequest.js#newcode75 webrequest.js:75: FilterNotifier.triggerListeners("document.stats", filter, details.tabId); The usual notification is "filter.hitCount". This one isn't being sent out automatically in Chrome because we don't actually count hits, no reason to add a new notification however.
http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.json File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:113: }, This is only used in the following context and not in a sentence: Blocked ads: 12 on this page 21 in total I changed it to "<strong>$number$</strong> in total" and "<strong>$number$</strong> on this page" because the number needs to be highlighted. http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:124: "description": "Label for link to adblockplus.org", For Facebook we have the ability to add an extra link as an action to the share message (will be displayed similar as "Comment", "Like" and such actions).
http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.json File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/11627039/diff/1/_locales/en_US/messages.jso... _locales/en_US/messages.json:124: "description": "Label for link to adblockplus.org", On 2013/09/18 09:48:25, Thomas Greiner wrote: > For Facebook we have the ability to add an extra link as an action to the share > message (will be displayed similar as "Comment", "Like" and such actions). Sure. The description should indicate the context however - and the context is the share message here. The important information is that this isn't something we display in our UI. http://codereview.adblockplus.org/11627039/diff/11002/_locales/en_US/messages... File _locales/en_US/messages.json (right): http://codereview.adblockplus.org/11627039/diff/11002/_locales/en_US/messages... _locales/en_US/messages.json:148: "message": "I blocked $number$ ads and trackers thanks to Adblock Plus", This sentence still needs a period at the end.
LGTM on the strings. Below is the rest of my comments, reviewed UI code as well now. http://codereview.adblockplus.org/11627039/diff/11002/popup.html File popup.html (right): http://codereview.adblockplus.org/11627039/diff/11002/popup.html#newcode80 popup.html:80: #statsStuff #statsContainer? :) http://codereview.adblockplus.org/11627039/diff/11002/popup.html#newcode88 popup.html:88: background: linear-gradient(top, rgb(250, 250, 255), rgb(200, 200, 255) 50px); This isn't valid linear-gradient syntax, see https://developer.mozilla.org/en-US/docs/Web/CSS/linear-gradient. The WebKit-specific syntax is non-standard, changing "top" into "to bottom" should work however. http://codereview.adblockplus.org/11627039/diff/11002/popup.html#newcode95 popup.html:95: min-width: 30px; Hm, we'll easily see six-digit numbers here... Maybe 50px? http://codereview.adblockplus.org/11627039/diff/11002/popup.html#newcode144 popup.html:144: <div id="statsTotal" class="i18n_stats_label_total label"></div> Please remove the i18n classes here, just fill the fields "manually". This will make sure no bogus content shows up if your code doesn't run for some reason. http://codereview.adblockplus.org/11627039/diff/11002/stats.js File stats.js (right): http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode32 stats.js:32: var shareURL = "https://adblockplus.org"; Add a slash at the end of the URL? http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode33 stats.js:33: var redirectURL = "https://www.facebook.com"; Given that this is Facebook-specific, I don't think that a global variable is necessary - just add it to the shareLinks variable. http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode39 stats.js:39: querystring.push(key + "=" + encodeURIComponent(params[key])); encodeURIComponent(key) please, don't assume that it doesn't need to be encoded. http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode78 stats.js:78: shareActions[i].addEventListener("click", share, false); shareBox.addEventListener("click", share, false) will have exactly the same effect without hardcoding the page structure - events bubble up. http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode82 stats.js:82: chrome.tabs.query({active: true}, function(tabs) You need windowId: chrome.windows.WINDOW_ID_CURRENT as well, otherwise you might get a tab from a different window. Note that currentWindow property isn't supported before Chrome 19. http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode113 stats.js:113: statsTotal.innerText = formatNumber(Stats.total.blocked); So that's why we have placeholders, so that we can later go into the text and assume that the placeholder was inside the <strong> block :) I guess that the best approach would be extending public functionality in i18n.js, e.g. but adding a public setElementText() function: function setElementText(element, stringName, arguments) { function processString(...) { ... } while (element.lastChild) element.removeChild(element.lastChild); processString(i18n.getMessage(stringName, arguments), element); } loadI18nStrings() could then call this function - and you could as well, whenever you need it. http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode122 stats.js:122: shareBox.setAttribute("hidden", true); How about: var shareBox = document.getElementById("shareBox"); shareBox.hidden = !shareBox.hidden; The name should probably be toggleShareBox() to reflect the functionality. Note that I'm opposed to caching all elements in global variables, it's typically unnecessary (doesn't save any code and is irrelevant performance-wise) and makes the code harder to read. http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode135 stats.js:135: .replace("_MESSAGE_", chrome.i18n.getMessage("stats_share_message", blocked)); This is a rather ugly approach, and you forgot to URL-encode the message here. How about generating the links here rather then up front?. Then createShareLink() could insert the message during link generation: var messageMark = {}; var shareLinks = { facebook: ["http://...", { ... name: messageMark, ... }] }; function createShareLink(network, blockedCount) { var url = shareLinks[network][0]; var params = shareLinks[network][1]; ... var value = params[key]; if (value == messageMark) value = i18n.getMessage("stats_share_message", blockedCount); ... } http://codereview.adblockplus.org/11627039/diff/11002/stats.js#newcode144 stats.js:144: } Utils.appLocale returns the browser's locale. So this code would only make sense if toLocaleString() uses the system locale by default and not the browser locale. In my tests this doesn't seem to be the case. In other words, formatNumber(number) does exactly the same as number.toLocaleString().
Just for clarification purposes: We will extend the stats module in the near future to improve the ad counter functionality by showing more details than just the total number of blocked requests. Unless you have anything against it, I will address your comments whilst taking this into account. http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js#newcode37 lib/prefs.js:37: stats_total: {} We agreed on extending it in the near future to provide more value to the user than just showing the total number of blocked requests. Therefore I decided on using an object which can be extended to resemble the window statistics data that we're storing in Firefox: { items: 0, hidden: 0, blocked: 0, whitelisted: 0, filters: {} } http://codereview.adblockplus.org/11627039/diff/1/webrequest.js File webrequest.js (right): http://codereview.adblockplus.org/11627039/diff/1/webrequest.js#newcode75 webrequest.js:75: FilterNotifier.triggerListeners("document.stats", filter, details.tabId); I thought about it before but the arguments differ between this and the Firefox version. Firefox: name, filter, new value, old value Chrome: name, filter, tab ID
http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js#newcode37 lib/prefs.js:37: stats_total: {} On 2013/09/19 09:42:29, Thomas Greiner wrote: > We agreed on extending it in the near future to provide more value to the user > than just showing the total number of blocked requests. I wonder what you have in mind here. Counting element hiding hits isn't possible in Chrome (at least it isn't without impacting the performance) and storing the number of total/whitelisted elements seems fairly useless. http://codereview.adblockplus.org/11627039/diff/1/webrequest.js File webrequest.js (right): http://codereview.adblockplus.org/11627039/diff/1/webrequest.js#newcode75 webrequest.js:75: FilterNotifier.triggerListeners("document.stats", filter, details.tabId); On 2013/09/19 09:42:29, Thomas Greiner wrote: > I thought about it before but the arguments differ between this and the Firefox > version. > > Firefox: name, filter, new value, old value > Chrome: name, filter, tab ID I guess we can introduce an additional application-specific parameter. So you would call here: FilterNotifier.triggerListeners("filter.hitCount", filter, 0, 0, details.tabId); And once we actually start counting hits in Chrome we'll call FilterStorage.increaseHitCount() here and add "context" as an additional parameter for that call.
I stuck with using an object as preference value for now due to the given reasons. You can find the changes to i18n.js that I made in a separate review. http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/11627039/diff/1/lib/prefs.js#newcode37 lib/prefs.js:37: stats_total: {} There are two categories of additional data that we could add: - time-specific: today, this week, etc. - type-specific: tracking, social, etc. (based on the filterlist the filter is in)
LGTM with the two nits fixed http://codereview.adblockplus.org/11627039/diff/23011/stats.js File stats.js (right): http://codereview.adblockplus.org/11627039/diff/23011/stats.js#newcode31 stats.js:31: app_id: 475542399197328, Just realized that this by far exceeds the 32 bit integer range meaning that JavaScript will store this as a floating point number internally. How about using a string to ensure that there cannot be any precision loss? Right now this identifier is only one digit too short to suffer from precision loss in Firefox. http://codereview.adblockplus.org/11627039/diff/23011/stats.js#newcode88 stats.js:88: } How about |else window.close()| here just in case? It's not like we should ever run into that case but if we do we won't display a broken UI.
http://codereview.adblockplus.org/11627039/diff/23011/stats.js File stats.js (right): http://codereview.adblockplus.org/11627039/diff/23011/stats.js#newcode88 stats.js:88: } Closing the popup right after a user clicked on the icon to open it is not a good idea. The user should still be able to use the other features that are available in the popup. Therefore I'd rather leave it like that and not show the stats container in that case.
http://codereview.adblockplus.org/11627039/diff/23011/stats.js File stats.js (right): http://codereview.adblockplus.org/11627039/diff/23011/stats.js#newcode88 stats.js:88: } On 2013/09/24 14:40:58, Thomas Greiner wrote: > Closing the popup right after a user clicked on the icon to open it is not a > good idea. The user should still be able to use the other features that are > available in the popup. > Therefore I'd rather leave it like that and not show the stats container in that > case. You are right of course. I forgot that the stats are hidden by default. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
