Bug 63021

Summary: Remove FrameLoader::m_workingURL
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, darin, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 29947    
Attachments:
Description Flags
Patch
none
Patch none

Description Adam Barth 2011-06-20 15:43:47 PDT
Remove FrameLoader::m_workingURL
Comment 1 Adam Barth 2011-06-20 15:48:48 PDT
Created attachment 97875 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-20 15:52:59 PDT
Comment on attachment 97875 [details]
Patch

OK.
Comment 3 Darin Adler 2011-06-20 15:53:31 PDT
I am currently reviewing and writing comments. You may want to wait before doing commit-queue+.
Comment 4 Eric Seidel (no email) 2011-06-20 15:55:08 PDT
I left the cq? to let the bots do their munching, as well as let others like yourself get a whack it it. :)  I'll CC ap and bradee just in case too.
Comment 5 Adam Barth 2011-06-20 15:56:20 PDT
> I am currently reviewing and writing comments.

Thanks.  Comments always appreciated.
Comment 6 Darin Adler 2011-06-20 15:56:34 PDT
Comment on attachment 97875 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:422
> +    m_haveReceivedFirstData = true;

Since we normally name boolean members so they finish a sentence "frame loader <xxx>" I think this should be m_hasReceivedFirstData.

> Source/WebCore/loader/FrameLoader.cpp:464
> -bool FrameLoader::didOpenURL(const KURL& url)
> +bool FrameLoader::didOpenURL()

The names for didOpenURL, closeURL and other such functions are seeming more and more strange. Removing this URL argument is part of making them even stranger. New names could really help.

> Source/WebCore/loader/FrameLoader.cpp:578
> +    KURL workingURL =
> +#if ENABLE(WEB_ARCHIVE) || ENABLE(MHTML)
> +        m_archive ? m_archive->mainResource()->url() :
> +#endif
> +        activeDocumentLoader()->documentURL();

Is there a way to write this without the crazy formatting? There must be!

> Source/WebCore/loader/FrameLoader.h:439
> +    bool m_haveReceivedFirstData;

It would be best to initialize this in the FrameLoader constructor anther than leaving it uninitialized until the call to open.
Comment 7 Adam Barth 2011-06-20 16:02:43 PDT
(In reply to comment #6)
> (From update of attachment 97875 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97875&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:422
> > +    m_haveReceivedFirstData = true;
> 
> Since we normally name boolean members so they finish a sentence "frame loader <xxx>" I think this should be m_hasReceivedFirstData.

Done.

> > Source/WebCore/loader/FrameLoader.cpp:464
> > -bool FrameLoader::didOpenURL(const KURL& url)
> > +bool FrameLoader::didOpenURL()
> 
> The names for didOpenURL, closeURL and other such functions are seeming more and more strange. Removing this URL argument is part of making them even stranger. New names could really help.

Agreed.  Thoughts on a new name?

> > Source/WebCore/loader/FrameLoader.cpp:578
> > +    KURL workingURL =
> > +#if ENABLE(WEB_ARCHIVE) || ENABLE(MHTML)
> > +        m_archive ? m_archive->mainResource()->url() :
> > +#endif
> > +        activeDocumentLoader()->documentURL();
> 
> Is there a way to write this without the crazy formatting? There must be!

I've changed this to another one.

> > Source/WebCore/loader/FrameLoader.h:439
> > +    bool m_haveReceivedFirstData;
> 
> It would be best to initialize this in the FrameLoader constructor anther than leaving it uninitialized until the call to open.

Done.
Comment 8 Adam Barth 2011-06-20 16:05:30 PDT
Created attachment 97881 [details]
Patch
Comment 9 Darin Adler 2011-06-20 16:11:58 PDT
(In reply to comment #7)
> > The names for didOpenURL, closeURL and other such functions are seeming more and more strange. Removing this URL argument is part of making them even stranger. New names could really help.
> 
> Agreed.  Thoughts on a new name?

I don’t have the slightest idea what closeURL should be named. There’s already a function named stopLoading, one named finishedLoading, one named finishedLoadingDocument, and one named loadDone. I have no clear idea how these functions relate to each other. Aggh. Must keep at it!

For didOpenURL there’s already a function named started, already a function named open, and already a function named transitionToCommitted. I have no clear idea how these functions relate to each other. Aggh. Must keep at it!
Comment 10 Adam Barth 2011-06-20 16:14:18 PDT
Eric is looking at all the functions that start loads.  Maybe the next thing for me to do is to look at all the functions that stop the loader.
Comment 11 Adam Barth 2011-06-20 16:32:59 PDT
Committed r89312: <http://trac.webkit.org/changeset/89312>
Comment 12 Eric Seidel (no email) 2011-06-22 13:44:15 PDT
This caused https://bugs.webkit.org/show_bug.cgi?id=63169.  And could have been avoided by us running the layout tests on our mac-ews bots.  We should look at doing that again.
Comment 13 Adam Barth 2011-06-22 13:54:56 PDT
(In reply to comment #12)
> This caused https://bugs.webkit.org/show_bug.cgi?id=63169.  And could have been avoided by us running the layout tests on our mac-ews bots.  We should look at doing that again.

Interesting.  I ran the tests locally specifically because I was worried about webarchive issues.