341837 - microsummary service should impose limit on length of microsummary

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

The microsummary service should impose a limit on the length of microsummaries to prevent overlong microsummaries from causing problems.  The limit should be large enough that it doesn't impact microsummary usability (i.e. it doesn't visibly truncate microsummaries in the UI) while being small enough to ensure we can easily handle the amount of data.

4096 characters seems reasonable for text microsummaries.

Comment 1

[Joey Minta]

Talked to myk about taking this one.
-> me

Comment 2

[Joey Minta]

Attachment: limit to 4096

Patch limits the size of the returned summary to 4096 characters.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 230443 [details] [diff] [review]
limit to 4096

>-    if (this.content)
>-      return this.content.replace(/^\s+|\s+$/g, "");
>-    else if (this.template)
>+    if (this.content) {
>+      var content = this.content.replace(/^\s+|\s+$/g, "");
>+      if (content.length > MAX_SUMMARY_LENGTH) 
>+        content = content.substring(0, MAX_SUMMARY_LENGTH);
>+      return content;
>+    } else if (this.template)
>       return this._processTemplate(pageContent);

Nit: it'd be useful to mention why we don't truncate template-generated content here (i.e. because we truncate it inside processTemplate instead).  Or perhaps it'd make sense to factor out that code, truncating both kinds in the same place.


>@@ -1746,16 +1755,17 @@ function getLoadedMicrosummaryResource(u
>  * Removes all characters not in the "chars" string from aName.
>  *
>  * @returns a sanitized name to be used as a filename, or a random name
>  *          if a sanitized name cannot be obtained (if aName contains
>  *          no valid characters).
>  */
> function sanitizeName(aName) {
>   const chars = "-abcdefghijklmnopqrstuvwxyz0123456789";
>+  const maxLength = 60;
> 
>   var name = aName.toLowerCase();
>   name = name.replace(/ /g, "-");
>   //name = name.split("").filter(function (el) {
>   //                               return chars.indexOf(el) != -1;
>   //                             }).join("");
>   var filteredName = "";
>   for ( var i = 0 ; i < name.length ; i++ )
>@@ -1764,16 +1774,19 @@ function sanitizeName(aName) {
>   name = filteredName;
> 
>   if (!name) {
>     // Our input had no valid characters - use a random name
>     for (var i = 0; i < 8; ++i)
>       name += chars.charAt(Math.round(Math.random() * (chars.length - 1)));
>   }
> 
>+  if (name.length > maxLength)
>+    name = name.substring(0, maxLength);
>+

This looks like that other patch that was recently checked in.  r=myk with this part removed.

Comment 4

[Joey Minta]

Landed on trunk.
Checking in browser/components/microsummaries/src/nsMicrosummaryService.js.in;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js.in,v  <--  nsMicrosummaryService.js.in
new revision: 1.31; previous revision: 1.30
done

Comment 5

[Joey Minta]

Comment on attachment 230443 [details] [diff] [review]
limit to 4096

Requesting approval.  Fairly trivial change here, which ispiked wrote a testcase for, using tinderbox build logs. http://people.mozilla.org/~aguthrie/testcases/microsummaries/test-9.html

Comment 6

[Joey Minta]

Landed on branch
Checking in browser/components/microsummaries/src/nsMicrosummaryService.js.in;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js.in,v  <--  nsMicrosummaryService.js.in
new revision: 1.1.2.33; previous revision: 1.1.2.32
done