Bug 63021 – Remove FrameLoader::m_workingURL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove FrameLoader::m_workingURL

[Adam Barth]

Created attachment 97875 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 97875 [details]
Patch

OK.

[Darin Adler]

I am currently reviewing and writing comments. You may want to wait before doing commit-queue+.

[Eric Seidel (no email)]

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.

[Adam Barth]

> I am currently reviewing and writing comments.

Thanks.  Comments always appreciated.

[Darin Adler]

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.

[Adam Barth]

(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.

[Adam Barth]

Created attachment 97881 [details]
Patch

[Darin Adler]

(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!

[Adam Barth]

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.

[Adam Barth]

Committed r89312: <http://trac.webkit.org/changeset/89312>

[Eric Seidel (no email)]

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.

[Adam Barth]

(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.