Bug 74278

Summary: Specialize isLoading() method in HTMLLinkelement.h
Product: WebKit Reporter: Renata Hodovan <rhodovan.u-szeged>
Component: DOMAssignee: Renata Hodovan <rhodovan.u-szeged>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, kling
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
none
Proposed patch
rhodovan.u-szeged: commit-queue-
Proposed patch darin: review+, rhodovan.u-szeged: commit-queue-

Description Renata Hodovan 2011-12-12 02:39:58 PST
Fulfill FIXME with renaming isLoading() method to isStyleSheetLoading().
Comment 1 Renata Hodovan 2011-12-12 02:42:40 PST
Created attachment 118757 [details]
Proposed patch
Comment 2 Andreas Kling 2011-12-12 06:20:44 PST
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.
Comment 3 Renata Hodovan 2011-12-12 06:33:54 PST
(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 :)
Comment 4 Alexey Proskuryakov 2011-12-12 10:23:39 PST
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?
Comment 5 Andreas Kling 2011-12-12 10:35:55 PST
(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.
Comment 6 Renata Hodovan 2011-12-12 10:44:26 PST
(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 :)
Comment 7 Darin Adler 2011-12-12 15:17:20 PST
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.
Comment 8 Renata Hodovan 2011-12-14 03:47:31 PST
Created attachment 119192 [details]
Proposed patch

According to Darin's advice I made the renaming: isStyleSheetLoading() -> linkElementStyleSheetIsLoading().
Any objections?
Comment 9 Darin Adler 2011-12-14 10:53:39 PST
(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.
Comment 10 Eric Seidel (no email) 2011-12-19 10:48:03 PST
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.
Comment 11 Renata Hodovan 2011-12-19 11:13:37 PST
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 :)
Comment 12 Renata Hodovan 2011-12-21 02:21:34 PST
Committed r103401: <http://trac.webkit.org/changeset/103401>