Remove FrameLoader::m_workingURL
Created attachment 97875 [details] Patch
Comment on attachment 97875 [details] Patch OK.
I am currently reviewing and writing comments. You may want to wait before doing commit-queue+.
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.
> I am currently reviewing and writing comments. Thanks. Comments always appreciated.
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.
(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.
Created attachment 97881 [details] Patch
(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!
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.
Committed r89312: <http://trac.webkit.org/changeset/89312>
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.
(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.