342228 - microsummary service should support <link> tags with rel=space-separated list of words

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect, P1)

Description

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

The microsummary service ignores link tags whose rel attribute isn't the exact string "microsummary", but the HTML specs says the rel attribute takes a space-separated list of values, so the microsummary service should extract the value of that attribute into an array and process the <link> tag as a microsummary reference so long as any member of that array is "microsummary".

Comment 1

[Phil Ringnalda (:philor)]

I didn't find your detection code within the limits of my attention span, but reminding myself from attachment 191767 [details] for RSS/Atom - both rel and type should be case-insensitive, and whitespace around type should be ignored, if they aren't already.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: simple patch

Simple change to accept LINK tags with space-separated, case-insensitive REL attribute values.

[Verified with spec at http://www.w3.org/TR/REC-html40/struct/links.html#edef-LINK]

Comment 3

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

Comment on attachment 229763 [details] [diff] [review]
simple patch

>-      if (link.getAttribute("rel") != "microsummary")
>+      var relAttr = link.getAttribute("rel");
>+      if (!relAttr)
>         continue;

We use hasAttribute elsewhere to check for the presence of an attribute.  We should use it here, too, i.e.:

if (!link.hasAttribute("rel"))
  continue;


>+      // The attribute's value can be a space-separated list of link types.
>+      var linkTypes = relAttr.split(/\s+/);
>+      var found = false;
>+
>+      for ( var j = 0; j < linkTypes.length; j++ ) {
>+	if (linkTypes[j].toLowerCase() == "microsummary") {
>+            found = true;
>+            break;
>+          }
>+      }
>+
>+      if (!found)
>+        continue;
>+
>+

Nit: this would be simpler, and perhaps faster, as:

if (!linkTypes.some( function(v) { return v.toLowerCase() == "microsummary" } ))
  continue;

Otherwise looks good.  Thanks for the patch!  Fix the first issue, and (optionally) the second, and r=myk.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: updated w/ comment #3

Updated patch.

I fixed the nit -- I had considered linkTypes.some() before, but thought that might be considered obfuscated. :-) 

Do simple fixes like this need sr/1.8.1 approval? Also, I assume someone else can check this in since I don't have cvs access...

Comment 5

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

Comment on attachment 230035 [details] [diff] [review]
updated w/ comment #3

Looks good, thanks!

>I fixed the nit -- I had considered linkTypes.some() before, but thought that
>might be considered obfuscated. :-) 

Yeah, there's a definite tradeoff there, so consider that suggestion strictly optional.  :-)

But FWIW, I think such expressions will become easier to read as developers become more familiar with Array.some() and its JS 1.6 siblings, and there's value in exercising the new features of the language.


> Do simple fixes like this need sr/1.8.1 approval? Also, I assume someone 
> else can check this in since I don't have cvs access...

Per mconnor, I can review fixes to microsummaries code, but I should require sr from a Firefox peer at my discretion, which I generally do when the code affects other modules or raises larger design issues.  This one is just a simple fix, however, and doesn't need sr.

It does, however, need approval1.8.1 to go on the branch.  I'll check it in to the trunk for you and then request approval for the branch.

Comment 6

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

Fix checked in the trunk.

Comment 7

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

Comment on attachment 230035 [details] [diff] [review]
updated w/ comment #3

Notes for drivers considering this approval request:

This makes the microsummary service support the standard specification of the <link> tag.  It is a small fix with a low risk of regressions.  It has been on the trunk since Thursday afternoon.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 230035 [details] [diff] [review]
updated w/ comment #3

a=mconnor on behalf of drivers for checkin to the 1.8 branch