Bug 74278 – Specialize isLoading() method in HTMLLinkelement.h

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Renata Hodovan]

Fulfill FIXME with renaming isLoading() method to isStyleSheetLoading().

[Renata Hodovan]

Created attachment 118757 [details]
Proposed patch

[Andreas Kling]

Comment on attachment 118757 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118757&action=review

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=74278
> +
> +        Specialize isLoading() method for stylesheets.

None of this tells us what's being changed.

"Rename HTMLLinkElement::isLoading() to isStyleSheetLoading()." would be better.

[Renata Hodovan]

(In reply to comment #2)
> (From update of attachment 118757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118757&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        https://bugs.webkit.org/show_bug.cgi?id=74278
> > +
> > +        Specialize isLoading() method for stylesheets.
> 
> None of this tells us what's being changed.
> 
> "Rename HTMLLinkElement::isLoading() to isStyleSheetLoading()." would be better.

All right, thanks :)

[Alexey Proskuryakov]

Comment on attachment 118757 [details]
Proposed patch

We usually try for code to have correct grammar when possible. Compare "if (isLoading())" to "if (isStyleSheetLoading())". This patch makes this aspect worse.

Maybe "styleSheetIsLoading" would be better?

[Andreas Kling]

(In reply to comment #4)
> (From update of attachment 118757 [details])
> We usually try for code to have correct grammar when possible. Compare "if (isLoading())" to "if (isStyleSheetLoading())". This patch makes this aspect worse.
> 
> Maybe "styleSheetIsLoading" would be better?

Yes, you're right. I thought of suggesting isLoadingStyleSheet(), but decided against it since it sort-of implies that the link element is responsible the actual loading. styleSheetIsLoading() is a better name indeed.

[Renata Hodovan]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 118757 [details] [details])
> > We usually try for code to have correct grammar when possible. Compare "if (isLoading())" to "if (isStyleSheetLoading())". This patch makes this aspect worse.
> > 
> > Maybe "styleSheetIsLoading" would be better?
> 
> Yes, you're right. I thought of suggesting isLoadingStyleSheet(), but decided against it since it sort-of implies that the link element is responsible the actual loading. styleSheetIsLoading() is a better name indeed.

Okay guys. I guess you are right indeed. I just followed the idea in the FIXME. But i'll rename it in a sec if styleSheetIsLoading() fits for everyone :)

[Darin Adler]

Names for members with boolean type or boolean return type are supposed to finish a phrase “link element <xxx>” for clarity. The phrase “link element is style sheet loading” is nonsense. I think that “link element style sheet is loading” is OK; perhaps we could do even better.

[Renata Hodovan]

Created attachment 119192 [details]
Proposed patch

According to Darin's advice I made the renaming: isStyleSheetLoading() -> linkElementStyleSheetIsLoading().
Any objections?

[Darin Adler]

(In reply to comment #8)
> According to Darin's advice I made the renaming: isStyleSheetLoading() -> linkElementStyleSheetIsLoading().

Oops. That’s a misunderstanding. The words “link element” are the class name, and implicit because this is a member function. Like this:

    linkElement->styleSheetIsLoading().

Sorry to steer you wrong there.

[Eric Seidel (no email)]

Comment on attachment 118757 [details]
Proposed patch

Cleared Andreas Kling's review+ from obsolete attachment 118757 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Renata Hodovan]

Created attachment 119890 [details]
Proposed patch

(In reply to comment #9)
> (In reply to comment #8)
> > According to Darin's advice I made the renaming: isStyleSheetLoading() -> linkElementStyleSheetIsLoading().
> 
> Oops. That’s a misunderstanding. The words “link element” are the class name, and implicit because this is a member function. Like this:
> 
>     linkElement->styleSheetIsLoading().
> 
> Sorry to steer you wrong there.

Ohh, sorry for being late with this update. I'm on holiday to get ready for my exams and I'm a bit lost :)

[Renata Hodovan]

Committed r103401: <http://trac.webkit.org/changeset/103401>