Bug 63021 - Remove FrameLoader::m_workingURL
Summary: Remove FrameLoader::m_workingURL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 29947
  Show dependency treegraph
 
Reported: 2011-06-20 15:43 PDT by Adam Barth
Modified: 2011-06-22 13:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.35 KB, patch)
2011-06-20 15:48 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2011-06-20 16:05 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

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