nsStyleLinkElement::ParseLinkTypes should return a bitmask

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Fred Jendrzejewski, Assigned: drexler)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Whiteboard: [good first bug]

Comment 1

9 years ago
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.
I think it's enough to just assign bits for the types that the code currently cares about.

Updated

8 years ago
Component: Content → DOM
QA Contact: content → general
I am trying to get some mozilla bugs closed (I am a beginer) Could you give some tips on this bug?
Is comment #0 unclear?
(Assignee)

Comment 5

6 years ago
Created attachment 575065 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

Changed the relevant source files and tested on Aurora.
Attachment #575065 - Flags: review?(roc)
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.
(Assignee)

Comment 7

6 years ago
Created attachment 575273 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

Thanks for the review and feedback. I made the required changes as suggested.
Attachment #575065 - Attachment is obsolete: true
Attachment #575273 - Flags: review?(roc)
Attachment #575065 - Flags: review?(roc)
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.
(Assignee)

Comment 9

6 years ago
> 
> @@ +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?
0
(Assignee)

Comment 11

6 years ago
Created attachment 575325 [details] [diff] [review]
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.
Attachment #575273 - Attachment is obsolete: true
Attachment #575325 - Flags: review?(roc)
Attachment #575273 - Flags: review?(roc)
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.
(Assignee)

Comment 13

6 years ago
Created attachment 575342 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask
Attachment #575342 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #575325 - Attachment is obsolete: true
Attachment #575325 - Flags: review?(roc)
Attachment #575342 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Attachment #575342 - Flags: checkin?(roc)
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 <bob@example.com>" 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 :-)
Assignee: nobody → andrew.quartey
Status: NEW → ASSIGNED

Updated

6 years ago
Attachment #575342 - Flags: checkin?(roc)
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)
(Assignee)

Comment 16

6 years ago
(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 <bob@example.com>"
> 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.
Pushed to inbound :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a751a20c3e8c
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/a751a20c3e8c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.