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]
:
:
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 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 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 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 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 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-12-17 18:57:37 PST
Is comment #0 unclear?
Comment 5 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 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 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 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 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 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 14:44:08 PST
0
Comment 11 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 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 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 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 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 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 Ed Morley [:emorley] 2011-11-20 03:20:53 PST
Pushed to inbound :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a751a20c3e8c
Comment 18 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.