466626 - nsStyleLinkElement::ParseLinkTypes should return a bitmask

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Fred Jendrzejewski]

Roc in bug #461047 :
nsStyleLinkElement::ParseLinkTypes should return a bitmask of known link types, or something like that, so we don't need to keep wrestling with strings.

Comment 1

[Dave Johnston]

Ok,
So I guess the known list should be based on the version 5 spec:
http://www.w3.org/TR/2008/WD-html5-20080610/single-page/#linkTypes

Any thoughts on how to handle unknown link types? Is it acceptable to just ignore them?
Looking through the code, we only match against a very small subset of the known types anyway.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's enough to just assign bits for the types that the code currently cares about.

Comment 3

[Martin Cerdeira (mRt)]

I am trying to get some mozilla bugs closed (I am a beginer) Could you give some tips on this bug?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is comment #0 unclear?

Comment 5

[Andrew Quartey [:drexler]]

Attachment: implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

Changed the relevant source files and tested on Aurora.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 575065 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

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

(linkTypes & NEXT) == NEXT will be false when linkTypes contains NEXT and something else. So you just want to check (linkTypes & NEXT) != 0, which abbreviates to (linkTypes & NEXT). Similar for the other places where you replaced Contains with the mask check.

"? true : false" is superfluous. A boolean expression already returns the right boolean value.

::: content/base/src/nsStyleLinkElement.h
@@ +87,5 @@
>    virtual void OverrideBaseURI(nsIURI* aNewBaseURI);
>    virtual void SetLineNumber(PRUint32 aLineNumber);
>  
> +  static void ParseLinkTypes(const nsAString& aTypes, PRUint32* aLinkMask);
> +  static void SetLinkMask(const nsAString& aLink, PRUint32* aLinkMask);

ParseLinkTypes can just return a PRUint32 result directly.

SetLinkMask doesn't need to be in this class, it can just be a static function in the .cpp file. And it can be called, say, ToLinkMask and return a PRUint32 directly.

::: content/xml/document/src/nsXMLContentSink.cpp
@@ +650,5 @@
>      if (nodeInfo->Equals(nsGkAtoms::link, kNameSpaceID_XHTML)) {
>        nsAutoString relVal;
>        aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::rel, relVal);
>        if (!relVal.IsEmpty()) {
> +        PRUint32 linkTypes = 0x0; 

Just 0.

Comment 7

[Andrew Quartey [:drexler]]

Attachment: implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

Thanks for the review and feedback. I made the required changes as suggested.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 575273 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

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

::: content/base/src/nsContentSink.cpp
@@ +828,5 @@
>    if (!LinkContextIsOurDocument(aAnchor)) {
>      return NS_OK;
>    }
>    
> +  bool hasPrefetch = linkTypes & PREFETCH ;

unnecessary space before ;

@@ +833,2 @@
>    // prefetch href if relation is "next" or "prefetch"
> +  if (hasPrefetch || (linkTypes & NEXT) ){

drop the space between ) ), add a space between ) {

@@ +835,4 @@
>      PrefetchHref(aHref, aElement, hasPrefetch);
>    }
>  
> +  if ((!aHref.IsEmpty()) &&  (linkTypes & DNS_PREFETCH)) {

only need one space after &&. Unnecessary parens around !aHref.IsEmpty()

@@ +839,5 @@
>      PrefetchDNS(aHref);
>    }
>  
>    // is it a stylesheet link?
> +  if (~(linkTypes & STYLESHEET)) {

Use boolean ! instead of bitwise ~ here (same for other cases below).

~ would work, but this really is a boolean expression and a boolean "not".

::: content/base/src/nsStyleLinkElement.cpp
@@ +59,5 @@
>  #include "nsUnicharInputStream.h"
>  #include "nsContentUtils.h"
>  
> +
> +static PRUint32 ToLinkMask(const nsAString& aLink, PRUint32 aLinkMask);

Just move the definition up to before the first use so you don't need a separate prototype.

@@ +194,5 @@
> +
> +PRUint32 ToLinkMask(const nsAString& aLink, PRUint32 aLinkMask)
> +{ 
> +  if (aLink.Equals(NS_LITERAL_STRING("prefetch")))
> +     return aLinkMask |= PREFETCH;

Don't take aLinkMask as a parameter. Just return PREFETCH and require the caller to | it in if necessary. Same for the other values.

This will make ToLinkMask simpler.

Comment 9

[Andrew Quartey [:drexler]]

> 
> @@ +194,5 @@
> > +
> > +PRUint32 ToLinkMask(const nsAString& aLink, PRUint32 aLinkMask)
> > +{ 
> > +  if (aLink.Equals(NS_LITERAL_STRING("prefetch")))
> > +     return aLinkMask |= PREFETCH;
> 
> Don't take aLinkMask as a parameter. Just return PREFETCH and require the
> caller to | it in if necessary. Same for the other values.
> 
> This will make ToLinkMask simpler.

Since we're returning PRUint32, what will be the default return if the string doesn't match any of the specified links?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

0

Comment 11

[Andrew Quartey [:drexler]]

Attachment: implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

Oops silly me. Thanks. I made the requisite changes and here's hoping that three is actually the charm.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 575325 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

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

The rest is good.

::: content/base/src/nsStyleLinkElement.cpp
@@ +154,5 @@
>  }
>  
> +PRUint32 ToLinkMask(const nsAString& aLink)
> +{ 
> +  if (aLink.Equals(NS_LITERAL_STRING("prefetch")))

These should use EqualsLiteral. Sorry I didn't spot this before.

Comment 13

[Andrew Quartey [:drexler]]

Attachment: implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

Comment 14

[Ed Morley [:emorley]]

In my queue with a few other bits that are being sent to try first and then onto inbound.

https://tbpl.mozilla.org/?tree=Try&rev=eb96679e6888

To save time for future patches (this one is fine for now), could you set your hgrc to include the author in format "Foo Bar <[e-mail]>" automatically & also add a commit message, along the lines of:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)

Comment 15

[Ed Morley [:emorley]]

Bug 687511 included in that try run failed check-sync-dirs, so had to push again:

https://tbpl.mozilla.org/?tree=Try&rev=329bc2d383a3 
(this changeset is there, is just hidden due to the previous push to try)

Comment 16

[Andrew Quartey [:drexler]]

(In reply to Ed Morley [:edmorley] from comment #14)
> In my queue with a few other bits that are being sent to try first and then
> onto inbound.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=eb96679e6888
> 
> To save time for future patches (this one is fine for now), could you set
> your hgrc to include the author in format "Foo Bar <[e-mail]>"
> automatically & also add a commit message, along the lines of:
> http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
> 
> Thanks :-)

Sure. Will do.

Comment 17

[Ed Morley [:emorley]]

Pushed to inbound :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a751a20c3e8c

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a751a20c3e8c