If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 2 beta2

Status

Firefox Graveyard
Microsummaries
P1
minor
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: myk, Assigned: Dolske)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 beta2
fixed1.8.1
Bug Flags:
blocking-firefox2 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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".
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.
(Reporter)

Updated

11 years ago
Priority: -- → P1
Whiteboard: [myk-mss]
Target Milestone: --- → Firefox 2 beta2
(Reporter)

Updated

11 years ago
Component: Bookmarks → Microsummaries
Flags: blocking-firefox2?
Whiteboard: [myk-mss]

Updated

11 years ago
Flags: blocking-firefox2? → blocking-firefox2+
(Assignee)

Comment 2

11 years ago
Created attachment 229763 [details] [diff] [review]
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]
Attachment #229763 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #229763 - Flags: review? → review?(myk)

Updated

11 years ago
Whiteboard: [has patch][needs review myk]

Updated

11 years ago
Assignee: nobody → justin-bugzilla
(Reporter)

Comment 3

11 years ago
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.
Attachment #229763 - Flags: review?(myk) → review-
(Assignee)

Comment 4

11 years ago
Created attachment 230035 [details] [diff] [review]
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...
Attachment #229763 - Attachment is obsolete: true
Attachment #230035 - Flags: review?(myk)
(Reporter)

Comment 5

11 years ago
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.
Attachment #230035 - Flags: review?(myk) → review+
(Reporter)

Comment 6

11 years ago
Fix checked in the trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review myk]
(Reporter)

Comment 7

11 years ago
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.
Attachment #230035 - Flags: approval1.8.1?
Comment on attachment 230035 [details] [diff] [review]
updated w/ comment #3

a=mconnor on behalf of drivers for checkin to the 1.8 branch
Attachment #230035 - Flags: approval1.8.1? → approval1.8.1+
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.1
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.