Bug 74278 - Specialize isLoading() method in HTMLLinkelement.h
Summary: Specialize isLoading() method in HTMLLinkelement.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Renata Hodovan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-12 02:39 PST by Renata Hodovan
Modified: 2011-12-21 02:21 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (3.88 KB, patch)
2011-12-12 02:42 PST, Renata Hodovan
no flags Details | Formatted Diff | Diff
Proposed patch (4.03 KB, patch)
2011-12-14 03:47 PST, Renata Hodovan
rhodovan.u-szeged: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (3.93 KB, patch)
2011-12-19 11:13 PST, Renata Hodovan
darin: review+
rhodovan.u-szeged: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>