[webkit-reviews] review granted: [Bug 41165] FrameLoader cleanup : Remove m_URL member : [Attachment 65432] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 41165: FrameLoader cleanup : Remove m_URL member
https://bugs.webkit.org/show_bug.cgi?id=41165

Attachment 65432: patch
https://bugs.webkit.org/attachment.cgi?id=65432&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Great idea to do this!

Unlike loader(), there is a tiny window where a frame can have 0 for
document(). I’m hoping we won’t introduce any new null-dereference problems
with this change.

It seems to me that could have been done in two steps. One step would be about
removing FrameLoader::m_URL, and a separate step would be removing
FrameLoader::url. This would make one patch have all the substantive changes,
and another just the mechanical step of calling directly through document()
instead of through loader(), making it easier to review.

> +    // We need to be careful about initializing the url in the case where
we're passed an empty url.
> +    // Specifically, we don't want to set about:blank for the initial empty
document unless it's not the main frame.
> +    if (!url.isEmpty() || (frame &&
(!frame->loader()->stateMachine()->creatingInitialEmptyDocument() ||
frame->tree()->parent())))
>	   setURL(url);

This comment should be replaced by one that says why we don't want to set
about:blank for the initial empty document on the main frame.

Comments should focus on why the code does something, not what the code does.
The code itself should be written so that it's clear what it does. he code
already says what the comment says -- don't set the URL to the empty URL if
it's an initial empty document for the main frame -- so we don't need to repeat
that. What we need from the comment is the information *not* contained in the
code.

I do not like the phrase "be careful" here, or frankly ever in discussions of
computer software. We should always be careful! ;-)

The word is "URL" not "url" in plain language, despite the fact that we use
"url" in variable names.

Otherwise, the patch seems good.

r=me, but I’m not going to set commit-queue+ because I’d like to see the
comment replaced by a substantive one.