Closed Bug 993906 Opened 7 years ago Closed 7 years ago

Package and send appropriate data with remotely hosted links request

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: Mardak, Assigned: mzhilyaev)

References

Details

(Whiteboard: p=0 s=it-32c-31a-30b.3 [qa-])

Attachments

(1 file, 4 obsolete files)

Bug 986521 creates the request to the remotely hosted links, and we'll want to send additional data in that request to provide metrics on impressions, clicks, etc with this bug.
Flags: firefox-backlog?
Blocks: 993909
Flags: firefox-backlog? → firefox-backlog+
A data packet is sent on two events:

1. when the newtab page is shown
2. upon directory link click

The data packet is only sent in 1. when there are any directory links shown.

Bug 993901 is also concerned about if directory links are shown and could perhaps re-use some code.
Blocks: 974474
Assignee: nobody → mzhilyaev
Whiteboard: p=13
This bug description has changed. This bug is about sending a data packet on an event unrelated to those described above. Here is the task it describes:

Send a data payload when a fresh batch of links is requested from the server.
It will contain:

1. an identifier for the user, set as a cookie
2. the number of directory link slots available on the newtab page

clarkbw: is the data point described in #2 conformant to our terms of service?
Flags: needinfo?(clarkbw)
clarkbw, to be clear, this bug is related to the data we sent with the request for directory links. I imagine this to include locale and potentially interests down the line.

This is separate from bug 975235 that will send data to count impressions and clicks.
(In reply to Olivier Yiptong [:oyiptong] from comment #2)
> This bug description has changed. This bug is about sending a data packet on
> an event unrelated to those described above. Here is the task it describes:
> 
> Send a data payload when a fresh batch of links is requested from the server.
> It will contain:
> 
> 1. an identifier for the user, set as a cookie
> 2. the number of directory link slots available on the newtab page
  3. locale
 
> clarkbw: is the data point described in #2 conformant to our terms of
> service?

I'll have to check with Geof on #2 as I'm not sure.  We should be including the locale (#3) so we can match it to the language we offer.
Flags: needinfo?(clarkbw)
Thanks for your comments, Bryan.

That means that the client implementation needs to accept an HTTP response with status code 204, in the case that there are no links for the requested locale.

204 implies that the request was successful, but that there is no content. The response will not have a body.
Whiteboard: p=13 → p=5
Status: NEW → ASSIGNED
Whiteboard: p=5 → p=5 s=it-31c-30a-29b.3 [qa?]
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa?] → p=5 s=it-31c-30a-29b.3 [qa-]
I propose the locale be sent via the url.

e.g. https://tiles.data.mozilla.com/v1/links/newtab/en-US

This makes the client code much simpler.

The number of open directory link slots seem to make sense as a header.
As a query parameter it does not.

It looks like this can be a GET, unless i'm missing something.
After a conversation with emtwo and maksik, we have agreed that this is a POST, and the locale will be sent in a payload.

The decision to put the locale in a payload is because

1. a user may request multiple locales
2. we may want to return additional localized links based on geo-ip
3. we may send additional data in the payload for reporting

The request should have a header of Content-Type: application/json, and the body would look like:

{"locale": "en-US"}

This affects bug 986521 as well
(In reply to Olivier Yiptong [:oyiptong] from comment #7)
> 1. a user may request multiple locales
> {"locale": "en-US"}
How would we include multiple locales and when would we do that? Perhaps by detecting the languages from the pages loaded?
getLocale() currently fetches from firefox prefs.

To include multiple locales, instead of a string value to the locale key, an array of locales could be provided.

we could obtain page languages as well, and together with geoip decide to return a certain set of links.
Right, I was just thinking if the POST data contained "locale": <string>, it would be trickier to change the format to have multiple locales. Perhaps there's a single string for "browserLocale" and an optional array of "extraLocales" that might be detected from page languages or other sources.
Depends on: 1001619
Attachment #8414000 - Flags: review?(adw)
Attachment #8414000 - Attachment is obsolete: true
Attachment #8414000 - Flags: review?(adw)
Whiteboard: p=5 s=it-31c-30a-29b.3 [qa-] → p=5 s=it-32c-31a-30b.1 [qa-]
Attached patch v2 (obsolete) — Splinter Review
Attachment #8421051 - Flags: review?(adw)
Whiteboard: p=5 s=it-32c-31a-30b.1 [qa-] → p=5 s=it-32c-31a-30b.2 [qa-]
Comment on attachment 8421051 [details] [diff] [review]
v2

Review of attachment 8421051 [details] [diff] [review]:
-----------------------------------------------------------------

Since links aren't always fetched when reportShownCount is called, and the sites on newtab can change over time, there will be gaps of time where the directory counts are different from those in the directoryCount objects sent with requests.  Is that OK?  Should counts be accumulated between requests?

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +196,5 @@
>      try {
>        xmlHttp.open('POST', uri);
> +      xmlHttp.send(JSON.stringify({
> +        locale: this.locale,
> +        directoryCount: this._directoryCount,

I don't know your server, but you might want:

this._directoryCount || {}

since fetch is called by init and may happen before reportShownCount is ever called.  Or just initialize _directoryCount somewhere.
Attachment #8421051 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #13)
> Comment on attachment 8421051 [details] [diff] [review]
> v2
> 
> Review of attachment 8421051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since links aren't always fetched when reportShownCount is called, and the
> sites on newtab can change over time, there will be gaps of time where the
> directory counts are different from those in the directoryCount objects sent
> with requests.  Is that OK?  Should counts be accumulated between requests?
> 
> ::: toolkit/modules/DirectoryLinksProvider.jsm
> @@ +196,5 @@
> >      try {
> >        xmlHttp.open('POST', uri);
> > +      xmlHttp.send(JSON.stringify({
> > +        locale: this.locale,
> > +        directoryCount: this._directoryCount,
> 
> I don't know your server, but you might want:
> 
> this._directoryCount || {}
> 
> since fetch is called by init and may happen before reportShownCount is ever
> called.  Or just initialize _directoryCount somewhere.

This was actually done on purpose, so that if this._directoryCount is null, directoryCount is property is stripped from JSON payload to the backend server.
Removed from iteration.  Will be tracked the same way we track "non-regular" contributions from the community.
Whiteboard: p=5 s=it-32c-31a-30b.2 [qa-] → p=0 [qa-]
Component: Tabbed Browser → New Tab Page
Attached patch v.2.1 rebased onto bug993901 (obsolete) — Splinter Review
Attachment #8421051 - Attachment is obsolete: true
Depends on: 993901
Comment on attachment 8426548 [details] [diff] [review]
v.2.1 rebased onto bug993901

>diff --git a/toolkit/modules/DirectoryLinksProvider.jsm b/toolkit/modules/DirectoryLinksProvider.jsm
>   reportShownCount: function DirectoryLinksProvider_reportShownCount(directoryCount) {
>+    this._directoryCount = directoryCount;
This is causing a bc1 debug leak failure..
https://tbpl.mozilla.org/?tree=Try&rev=fc574de062dc

It seems to get fixed with this change instead:
+    this._directoryCount = JSON.parse(JSON.stringify(directoryCount));
https://tbpl.mozilla.org/?tree=Try&rev=51e03099d523

The directoryCount object is created with {} with keys/values of just the types and numbers. Perhaps somewhere the Object prototype is changed from newtab.js somewhere?
Attachment #8426548 - Attachment is obsolete: true
Comment on attachment 8428411 [details] [diff] [review]
v3.  Make a deep copy of directoryCount

>+++ b/toolkit/modules/DirectoryLinksProvider.jsm
>   reportShownCount: function DirectoryLinksProvider_reportShownCount(directoryCount) {
>+    // make a deep copy of directoryCount to avoid a leak
>+    this._directoryCount = JSON.parse(JSON.stringify(directoryCount));
maksik, did you look into why this was necessary?
Attachment #8428411 - Flags: review?(adw)
Comment on attachment 8428411 [details] [diff] [review]
v3.  Make a deep copy of directoryCount

Clearing review request since I understand another patch is coming.
Attachment #8428411 - Flags: review?(adw)
Maxim explained that he was thinking of adding a DirectoryLinksProvider method that returned the directoryLinks object instead of having the consumer pass in the object, but then there'd need to be another public method that triggered the fetch, since reportShownCount does that now.  I didn't really like that added complexity, so I think it's fine to clone the passed-in object if it fixes the leak.  JSON.parse(JSON.stringify()) kind of sucks, but whatever.

But I think the leak might have something to do with xpconnect or compartment boundaries, so I pushed a few things to try, and I'd like to see if they work before r+'ing the JSON patch here.  If they do work, I'm not sure it proves my guess was right, though.

https://tbpl.mozilla.org/?tree=Try&rev=024ba31e5197
this._directoryCount = Cu.cloneInto(directoryCount, this);

https://tbpl.mozilla.org/?tree=Try&rev=2a803867988f
this._directoryCount = Cu.cloneInto(directoryCount, this,
                                    { cloneFunctions: true });

https://tbpl.mozilla.org/?tree=Try&rev=86d197ebe144
this._directoryCount = directoryCount;
Cu.makeObjectPropsNormal(this._directoryCount);
I haven't seen cloneInto before, but searching in mxr, the pattern seems to be cloneInto(source, {}) as opposed to "this" ? The tests seem to be passing with cloneInto, so perhaps we'll just use that instead of parse/stringify.
Comment on attachment 8428411 [details] [diff] [review]
v3.  Make a deep copy of directoryCount

Review of attachment 8428411 [details] [diff] [review]:
-----------------------------------------------------------------

The second arg is used to root the new object as far as I can tell, so it seems like `this` and {} ought to be equivalent in this case.  But let's go with cloneInto, using either {} or this, instead of JSON.
Attachment #8428411 - Flags: review+
Attachment #8428411 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/84fbe9c9231c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
Whiteboard: p=0 [qa-] → p=0 s=it-32c-31a-30b.3 [qa-]
Blocks: 1042214
Blocks: 1043669
You need to log in before you can comment on or make changes to this bug.