We'll be sending impression pings according to the privacy policy to figure out which tiles are being seen. Similar to bug 972936 except with more detail. For now the data should probably be similar to the click ping data: links list id, link index, tile index, link score, tile pinned. So no click action, and we probably don't need tile index as we'll include all shown tiles, so the index is implicit. We would probably include the tokens needed for cardinality estimates for bug 972933. clarkbw, what other data should be included? The above data was from directory tile clicks, but the impression data will include non-directory. So we'll probably want to include type (history, enhanced, organic, affiliate, sponsored) as well as the full url but no title?

We'll send the data like it's sent in bug 993906 for fetch POST. The endpoint will probably be controlled by a pref: browser.newtabpage.directory.reportViewEndPoint https://tiles.up.mozillalabs.com/v1/links/view

(In reply to Ed Lee :Mardak from comment #0) > clarkbw, what other data should be included? The above data was from > directory tile clicks, but the impression data will include non-directory. > So we'll probably want to include type (history, enhanced, organic, > affiliate, sponsored) as well as the full url but no title? Yes that's the only difference as far as I understand. I'll get Jason to weigh in on this tomorrow.

Notes: only if enhanced = true change default enhanced = true

The POST data to send from bug 1043669: { locale: _, # locale for user agent urls: [ # metadata for list of urls currently appearing in a newtab page, including history links { tile_id: _, # an id provided in the fetch payload, if a directory tile url: _, # a url if a history/enhanced tile pos: _, # the position of the tile in the grid pinned: _, # boolean, whether or not the tile is pinned action: _ # one of ["click", "block", "pin"] or null }, ... ] } This payload structure will be the same for click and impression where the difference is the click sets an action for the appropriate tile.

Changes ping structure to be the same for click and impression: array of tiles {action,id,pin,pos,score,url} where all keys are optional although there will always be one (either id or url).

The main thing missing from the patch is the logic to upgrade users with DNT on (i.e., privacy.donottrackheader.enabled = true and privacy.donottrackheader.value = 1) to have browser.newtab.enhanced = false; otherwise default true. Where the enhanced pref controls the sending of pings.

The view ping sent by a new profile with this patch: {"locale":"en-US","tiles":[{"id":"f"},{"id":"y"},{"id":"w"},{"id":"l"},{"id":"m"},{"id":"p"},{"id":"t"},{"id":"a"},{"id":"b"}} action: undefined as it's a view ping id: corresponding strings -- I put some ids for the currently packaged json pin: undefined as none are pinned (would be "1" as a number if pinned) score: undefined as none produce a value after round(frecency / 10000) (minimum frecency of 5000 needed to result in a sent score of 1) url: undefined as directory tiles only include ids (would be sent for enhanced or history)

Changes the payload to have the action outside of the tiles array with the key being the action and value as the index of the actioned link in the tiles array. Also makes directoryIds numeric.

Report click and view pings the same way by including sites with their metadata when tiles are enhanced defaulting to true unless DNT is on. Remove telemetry probes and related now unused codepaths.

Comment on attachment 8472112 [details] [diff] [review] v1 Review of attachment 8472112 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/DirectoryLinksProvider.jsm @@ +132,5 @@ > + _setDefaultEnhanced: function DirectoryLinksProvider_setDefaultEnhanced() { > + if (!Services.prefs.prefHasUserValue(PREF_NEWTAB_ENHANCED)) { > + let enhanced = true; > + try { > + // Default to not enhanced if DNT is set to tell websites to not track That's interesting and it does make some sense, since enhanced tiles can be viewed as a form of tracking, but I think tying the two is at least debatable. Could you point to discussions where you decided to do this? Putting the summary of the decision here in the bug as a kind of record would be useful. @@ +319,5 @@ > + if (site) { > + // Remember which tiles data triggered the action > + let {link} = site; > + let tilesIndex = tiles.length; > + if (clickedLink == link) { Comparing the link objects directly instead of their URLs makes me a little nervous, but I can't think of a reason why it would fail. @@ +326,5 @@ > + > + // Make the payload in a way so keys can be excluded when stringified > + let id = link.directoryId; > + tiles.push({ > + id: id || site.enhancedId, Since enhanced tiles show up on top of history tiles (I think?), and this sends the directory ID to a Mozilla sever, this lets Mozilla know the URLs of some of your top visited sites. The TLDs actually, due to the other bugs in progress. Is that right? How does that relate to our privacy policy(ies)? I don't think that was true for the old ping since it only counted directory tiles, not enhanced tiles, but maybe I'm wrong. @@ +328,5 @@ > + let id = link.directoryId; > + tiles.push({ > + id: id || site.enhancedId, > + pin: site.isPinned() ? 1 : undefined, > + pos: pos != tilesIndex ? pos : undefined, Seems like this makes interpreting this part of the payload unnecessarily complicated -- always making this be pos would be easier to understand -- but I'll leave that to you. @@ +330,5 @@ > + id: id || site.enhancedId, > + pin: site.isPinned() ? 1 : undefined, > + pos: pos != tilesIndex ? pos : undefined, > + score: Math.round(link.frecency / PING_SCORE_DIVISOR) || undefined, > + url: id === undefined ? link.url : undefined, Nit: Wouldn't id || link.url be equivalent? @@ +344,5 @@ > } > > // Package the data to be sent with the ping > let ping = new XMLHttpRequest(); > + ping.open("POST", pingEndPoint + (action == "view" ? "view" : "click")); Seems like the URL's query param should be the same in all cases since the action is encoded in the payload, but again I'll leave that to you. :-) ::: toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js @@ +179,5 @@ > +add_task(function test_reportSitesAction() { > + let deferred, expectedPath, expectedPost; > + let done = false; > + server.registerPrefixHandler(kPingPath, (aRequest, aResponse) => { > + if (done) return; Nit: if (done) return; @@ +249,5 @@ > + frecency: 1000, > + url: "http://directory/", > + } > + }; > + // NB: there should be no "pos: 0" nor score Nit: Including `pos: undefined, score: undefined` would codify this comment and make it unnecessary. @@ +582,5 @@ > + Services.prefs.clearUserPref(kNewtabEnhancedPref); > + do_check_eq(Services.prefs.getBoolPref(kNewtabEnhancedPref), expected); > + } > + > + // Use the default donottrack prefs You should probably clearUserPref on the DNT prefs first to really ensure they have their default values. And include a comment that donottrackheader.value's default is 1. (I had to look it up at this point because I was confused why your next call to checkDefault(false) would work, given the conditional in _setDefaultEnhanced. I assumed 0 was the default.)

(In reply to Drew Willcoxon :adw from comment #11) > > + // Default to not enhanced if DNT is set to tell websites to not track > That's interesting and it does make some sense, since enhanced tiles can be > viewed as a form of tracking, but I think tying the two is at least > debatable. Could you point to discussions where you decided to do this? > Putting the summary of the decision here in the bug as a kind of record > would be useful. clarkbw, can you provide the context for this decision?

(In reply to Drew Willcoxon :adw from comment #11) > > + if (clickedLink == link) { > Comparing the link objects directly instead of their URLs makes me a little > nervous, but I can't think of a reason why it would fail. Nod. This requires the link object to be the same one from the sites object passed in. Potentially this could pass in clickedLinkIndex or probably clickedSiteIndex (this.cell.index). I've changed it to that. > > + // Make the payload in a way so keys can be excluded when stringified > > + let id = link.directoryId; > > + tiles.push({ > > + id: id || site.enhancedId, > > + url: id === undefined ? link.url : undefined, > Since enhanced tiles show up on top of history tiles (I think?), and this > sends the directory ID to a Mozilla sever, this lets Mozilla know the URLs > of some of your top visited sites. The TLDs actually, due to the other bugs > in progress. Is that right? How does that relate to our privacy > policy(ies)? To be clear, the combination of id and url will submit the top visited sites to Mozilla servers. There's communications going out on Monday covering this, so this patch should not land before that happens. There will also be in-product messaging similar to this mockup from bug 1053530: https://bug1053530.bugzilla.mozilla.org/attachment.cgi?id=8472712 > I don't think that was true for the old ping since it only counted directory > tiles, not enhanced tiles, but maybe I'm wrong. That's right. The old ping only sent for directory tiles and now it'll send for history, enhanced history, and directory tiles. > > + pos: pos != tilesIndex ? pos : undefined, > Seems like this makes interpreting this part of the payload unnecessarily > complicated -- always making this be pos would be easier to understand -- > but I'll leave that to you. Nod slightly complex but saves a little bit of space in the common case of a full (instead of sparse) array. Most users will submit a whole array of tiles with pos being undefined -- the only users that will submit entries with "pos" are those who have pinned a tile with empty tiles (removed enough history and directory tiles). > > > + url: id === undefined ? link.url : undefined, > Nit: Wouldn't id || link.url be equivalent? Not quite as we only want to provide a url value if there is no id. "id || link.url" would include a non-undefined id, e.g., 123, as the url. > > // Package the data to be sent with the ping > > let ping = new XMLHttpRequest(); > > + ping.open("POST", pingEndPoint + (action == "view" ? "view" : "click")); > Seems like the URL's query param should be the same in all cases since the > action is encoded in the payload, but again I'll leave that to you. :-) This "query" (actually url path) simplifies some routing on the server side as submissions sent to "view" will have no action and processing can be optimized for this common case. There were some discussions that, yes because the payload is actually the same, there could be a single view+click endpoint later. > > + if (done) return; > Nit: > if (done) > return; Done. > > + // NB: there should be no "pos: 0" nor score > Nit: Including `pos: undefined, score: undefined` would codify this comment > and make it unnecessary. Done. > > + // Use the default donottrack prefs > You should probably clearUserPref on the DNT prefs first to really ensure > they have their default values. And include a comment that > donottrackheader.value's default is 1. (I had to look it up at this point > because I was confused why your next call to checkDefault(false) would work, > given the conditional in _setDefaultEnhanced. I assumed 0 was the default.) Done.

clarkbw, in addition to comment 12; the view and click pings don't indicate how many tiles are actually visible. Do we need to include that number or maybe the window content size with both pings? (For the latter, it seems that we don't have any Firefox window size statistics, so we had to guess for bug 1026561 and bug 1053530.)

(In reply to Ed Lee :Mardak from comment #12) > (In reply to Drew Willcoxon :adw from comment #11) > > > + // Default to not enhanced if DNT is set to tell websites to not track > > That's interesting and it does make some sense, since enhanced tiles can be > > viewed as a form of tracking, but I think tying the two is at least > > debatable. Could you point to discussions where you decided to do this? > > Putting the summary of the decision here in the bug as a kind of record > > would be useful. > clarkbw, can you provide the context for this decision? Mozilla is putting out some communications this week around this so I don't want to step on that. But you're spot on that this could be viewed as a form of tracking. And for my part the decision was about removing that ambiguity from our users who are concerned about any form of data collection. I'll reference public blogs and such as they come out so we have the record.

(In reply to Ed Lee :Mardak from comment #14) > clarkbw, in addition to comment 12; the view and click pings don't indicate > how many tiles are actually visible. Do we need to include that number or > maybe the window content size with both pings? (For the latter, it seems > that we don't have any Firefox window size statistics, so we had to guess > for bug 1026561 and bug 1053530.) We need to know which tiles are actually visible in the view. If we need the window size to calculate that out we'll just need to get that reviewed.

Notes: we'll include the number of tiles that are visible instead of the resolution to save on some processing and magic numbers on the server side.

Make "view" into an action, like the other actions, that reports the last viewed site.

Comment on attachment 8475377 [details] [diff] [review] pt2: v1 Review of attachment 8475377 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/page.js @@ +119,5 @@ > > if (document.hidden) { > addEventListener("visibilitychange", this); > } else { > + // Allow the document to reflow so the page has sizing info Do we need to go through this rigmarole in the document.hidden/visibilitychange listener case? i.e., should it be built into onPageFirstVisible (or a function that then calls onPageFirstVisible)? @@ +214,5 @@ > site.captureIfMissing(); > } > } > > + // Work backwards to find the first visible site from the end ... or, this new chunk of code (plus reportSitesAction()) might work better factored out into its own method. Then the checkSizing business could be isolated there along with it. I like that better. What do you think? @@ +225,5 @@ > + let rect = node.getBoundingClientRect(); > + let target = document.elementFromPoint(rect.x + rect.width / 2, > + rect.y + rect.height / 2); > + if (target && target.compareDocumentPosition(node) & > + target.DOCUMENT_POSITION_CONTAINS) { I think you could use node.contains(target) instead, which in addition to being nicer to read returns true if target is an "inclusive descendant" of node (a descendant or node itself). http://dom.spec.whatwg.org/#dom-node-comparedocumentposition Although I wonder if this should use similar logic to gGrid._resizeGrid. Maybe the part that calculates the number of visible rows and cols could even be factored out so you could use it here and in _resizeGrid? @@ +231,5 @@ > + } > + } > + } > + > + DirectoryLinksProvider.reportSitesAction(gGrid.sites, "view", lastIndex); I think you should change the name and description of reportSitesAction's third param since in your other patch it's called clickedSiteIndex, which doesn't make sense with this change.

(In reply to Drew Willcoxon :adw from comment #19) > > + // Allow the document to reflow so the page has sizing info > Do we need to go through this rigmarole in the > document.hidden/visibilitychange listener case? My testing indicated it wasn't necessary as the preloaded document triggering "visibilitychange" will have sizing info. Cleaned up by moving this code to the end of onPageFirstVisible > > + // Work backwards to find the first visible site from the end > ... or, this new chunk of code (plus reportSitesAction()) might work better > factored out into its own method. For above, I've split the logic into waiting for sizing info. (So this code now is in onPageFirstSized.) > > + if (target && target.compareDocumentPosition(node) & > > + target.DOCUMENT_POSITION_CONTAINS) { > I think you could use node.contains(target) instead Neat! Saves a null check too. > Although I wonder if this should use similar logic to gGrid._resizeGrid. The main difference is the number of visible columns isn't calculated in resizeGrid. It sets a maximum number of columns then relies on layout to display/wrap the appropriate number of columns in the visible space. > I think you should change the name and description of reportSitesAction's > third param since in your other patch it's called clickedSiteIndex Indeed.

adw, fyi.. at least from my limited testing, these two sets of setTimeouts are enough to get the _reportSitesAction test to pass.

We can't send the ping until the in-product messaging is in place.

Comment on attachment 8475489 [details] [diff] [review] pt2: v2 Review of attachment 8475489 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Ed Lee :Mardak from comment #20) > The main difference is the number of visible columns isn't calculated in > resizeGrid. It sets a maximum number of columns then relies on layout to > display/wrap the appropriate number of columns in the visible space. Ah right.

Comment on attachment 8475497 [details] [diff] [review] pt2: v2 extra Review of attachment 8475497 [details] [diff] [review]: ----------------------------------------------------------------- Works for me. :-)

Please do not land this patch until I r+ it.

Comment on attachment 8476284 [details] [diff] [review] pt2: only report urls for enhanced (and not history) tiles Spoke to drew about this patch, and he agreed that this has the desired effect - I'm not close enough to the code to confirm, but I'll trust you guys.

Comment on attachment 8476284 [details] [diff] [review] pt2: only report urls for enhanced (and not history) tiles Review of attachment 8476284 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #28) > Spoke to drew about this patch, and he agreed that this has the desired > effect Which is that the only URLs in your newtab page that are sent to Mozilla are those that are listed in Mozilla's directory links JSON as "enhanced."

Tracking because enhanced tile is a new feature.

(In reply to Drew Willcoxon :adw from comment #11) > ... > Since enhanced tiles show up on top of history tiles (I think?), and this > sends the directory ID to a Mozilla sever, this lets Mozilla know the URLs > of some of your top visited sites. ... (In reply to Ed Lee :Mardak from comment #27) > Created attachment 8476284 [details] [diff] [review] > pt2: only report urls for enhanced (and not history) tiles Does this mean the initial intent was to track also the history stuff from the new tab page, and later the decision was changed to only track directory tiles? Were there public discussions on those? I've seen these two possibly related posts: https://blog.mozilla.org/netpolicy/2014/08/21/trust-should-be-the-currency/ https://blog.mozilla.org/advancingcontent/2014/08/21/a-call-for-trust-transparency-and-user-control-in-advertising/ Are these the "communications" you were talking about? If yes, then to me they're quite vague and don't really express or explain how directory tiles work, what kinds of data is sent on each mode, how to avoid sending data, etc.

(In reply to Avi Halachmi (:avih) from comment #32) > what kinds of data is sent on each mode, how to avoid sending data, etc. There was a townhall as well, but it seems that none of them really explained these other than saying DNT is considered in the data sending and users have easy controls to turn things off directly from the page. As those didn't provide information, I'll put that here from what I understand. There are 2 types of requests involving tiles: 1) fetch data 2) send ping The fetch data downloads once a day to get a remote json that contains default directory links that can also be enhanced. The directory tiles aspect kicks in if the user would have seen empty tiles and instead sees the links from that file. If one of those links is "enhanced," it'll display another image before the tile is hovered. The user's history will push out these directory links, e.g., typing in a page will be enough to create a new history entry on the new tab page that sits before these directory tiles. Additionally, if the history entry matches the eTLD+1 of an enhanced link, that tile will get its visual treatment (unhovered image, hovered logo, text) updated but its original link will remain the same to not break user expectations of where clicking the link will take them. To be clear, this functionality existed before bug 1042876 was fixed -- there was a locally packaged json file. The request for fetching data only sends the user's locale so that the server can respond with the appropriate set of links. Where as of now, it only has links for en-US. There's no identifier or other data to personalize the server's response. However the user's newtab page is personalized as only the history tiles that the user would have seen would get enhanced. The data ping is primarily for accounting and setting expectations with partners. There are two types of data pings: views and clicks, and together, they're used to calculate click through rates of tiles. These I hear are incredibly useful when approaching new potential partners (but the patch that landed does not send url data of plain history), so that they have some expectation on how much their tile will be viewed/clicked and whether that is worth it to pay Mozilla. These could also be used to help Mozilla determine how much to charge for the tiles: http://www.cnet.com/news/mozilla-expands-advertising-experiment-to-many-more-firefox-users/ "To help steer things what it sees as the right direction, Mozilla will offer a discounted rate to those whose advertising agenda matches Mozilla's. "Partners who are good actors -- respecting Do Not Track and other areas of the Web we believe fits our mission around innovation, opportunity, and trust -- will get a discount," Herman said. "Not only is this a media content opportunity, it's an opportunity for Mozilla to push our mission forward and reward our partners that are like-minded."

To turn off the remote requests (fetch and ping), just set the new tab preference to "classic" or "blank" from the customize menu in the top corner of the page. Only in "enhanced" will it make those requests. Additionally, if the user already had DNT turned on to tell websites not to track, the default upgrade behavior is to have new tab tiles set to classic. enhanced: browser.newtabpage.enabled = true, browser.newtabpage.enhanced = true classic: browser.newtabpage.enabled = true, browser.newtabpage.enhanced = false blank: browser.newtabpage.enabled = false At a technical level, both the fetch and ping endpoints are controlled by prefs: - fetch = browser.newtabpage.directory.source - ping = browser.newtabpage.directory.ping So setting those will affect the behavior as well.

Thanks for the very clear info. I don't know where do these discussions take place, but I think that if it was presented as clearly as you presented it (even if somewhat more simplified), it would: 1. Let the users know what's the difference between classic and enhanced. Because right now "enhanced" at the menu is just a word which probably wouldn't mean much to users, and it isn't easily, or at all, associated with ads. 2. If worded accurately, e.g. "Display 3 Ads on this page to support Mozilla. No personally identifiable information is ever sent. More help is available <here>", then users might even opt in if they feel like helping a good cause. As it stands, without proper communication or help at the page itself (the latter more important IMO), I'm guessing there would be FUD (like I had TBH) about the newtab page, and FUD on a product never helps anyone - except competitors. Displaying the proper wording on/from the newtab page itself somehow, e.g. via a small help link or at the menu, would really make the move transparent and put the user in control. Also, to me, the connection between the do not track flag and the tiles as ads is not an obvious one. I believe that if we'll put the info clearly at the front it would alienate much less than it could if it's released as it is right now.

(In reply to Avi Halachmi (:avih) from comment #35) > I believe that if we'll put the info clearly at the front it would > alienate much less than it could if it's released as it is right now. I'm not sure how you're testing the changes for this bug or what you mean "as it is right now," but this bug landed along with bug 1053530 which puts messaging in the new tab page plus provides a link to more information (although that link doesn't have the correct information yet). There's also bug 1040369 that provides information about the sponsored tiles.

(In reply to Ed Lee :Mardak from comment #36) > I'm not sure how you're testing the changes for this bug or what you mean > "as it is right now," but this bug landed along with bug 1053530 which puts > messaging in the new tab page plus provides a link to more information > (although that link doesn't have the correct information yet). There's also > bug 1040369 that provides information about the sponsored tiles. By "right now" I meant latest nightly (currently 2014-08-24), where it doesn't have the "What's this page" links/info. I didn't test with a clean profile so I also didn't see the intro if there's one. I wasn't aware of the other bugs. Thanks.