Last Comment Bug 466626 - nsStyleLinkElement::ParseLinkTypes should return a bitmask
: nsStyleLinkElement::ParseLinkTypes should return a bitmask
Status: RESOLVED FIXED
[good first bug]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla11
Assigned To: Andrew Quartey [:drexler]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-25 01:47 PST by Fred Jendrzejewski
Modified: 2012-02-01 13:59 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask (13.54 KB, patch)
2011-11-16 18:26 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask (13.50 KB, patch)
2011-11-17 13:05 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask (12.93 KB, patch)
2011-11-17 16:09 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask (12.87 KB, patch)
2011-11-17 17:15 PST, Andrew Quartey [:drexler]
roc: review+
Details | Diff | Splinter Review

Description User image Fred Jendrzejewski 2008-11-25 01:47:28 PST
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 User image Dave Johnston 2009-02-08 15:52:08 PST
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-08 16:26:22 PST
I think it's enough to just assign bits for the types that the code currently cares about.
Comment 3 User image Martin Cerdeira (mRt) 2010-12-17 13:29:35 PST
I am trying to get some mozilla bugs closed (I am a beginer) Could you give some tips on this bug?
Comment 4 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-12-17 18:57:37 PST
Is comment #0 unclear?
Comment 5 User image Andrew Quartey [:drexler] 2011-11-16 18:26:47 PST
Created attachment 575065 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask

Changed the relevant source files and tested on Aurora.
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-16 20:03:20 PST
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 User image Andrew Quartey [:drexler] 2011-11-17 13:05:26 PST
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.
Comment 8 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 13:17:13 PST
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 User image Andrew Quartey [:drexler] 2011-11-17 13:50:03 PST
> 
> @@ +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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 14:44:08 PST
0
Comment 11 User image Andrew Quartey [:drexler] 2011-11-17 16:09:26 PST
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.
Comment 12 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 16:50:56 PST
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 User image Andrew Quartey [:drexler] 2011-11-17 17:15:40 PST
Created attachment 575342 [details] [diff] [review]
implementation of nsStyleLinkElement::ParseLinkTypes returning a bitmask
Comment 14 User image Ed Morley [:emorley] 2011-11-19 18:10:42 PST
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 :-)
Comment 15 User image Ed Morley [:emorley] 2011-11-19 18:25:34 PST
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 User image Andrew Quartey [:drexler] 2011-11-19 18:57:41 PST
(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.
Comment 17 User image Ed Morley [:emorley] 2011-11-20 03:20:53 PST
Pushed to inbound :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a751a20c3e8c
Comment 18 User image Ed Morley [:emorley] 2011-11-20 14:20:07 PST
https://hg.mozilla.org/mozilla-central/rev/a751a20c3e8c

Note You need to log in before you can comment on or make changes to this bug.