Bug 31186 – Cleanup: Rename fields RenderTextControl::m_edited and RenderTextControl::m_userEdited

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Following up on Darin's comments in bug #20780.:

We should rename the fields RenderTextControl::m_edited and RenderTextControl::m_userEdited to be more descriptive.

[Daniel Bates]

Created attachment 43020 [details]
Patch

No functionality was changed so no tests were included in this patch.

On Darin's suggestion <https://bugs.webkit.org/show_bug.cgi?id=20780#c24>, I chose to rename RenderTextControl::m_edited to wasChangedSinceLastChangeEvent, and RenderTextControl::m_userEdited to m_lastChangeWasUserEdit. I felt these names were consistent (in wording - both use the word "change" to describe the modification to the text control) and best described what these fields are for. It would have been nice if we could have more concise names for these fields, but I could not think of any.

We should probably look into re-factoring the functionality of these methods/cleanup RenderTextControl as outlined in <https://bugs.webkit.org/show_bug.cgi?id=20780#c14>. Then we may have a cleaner API.

[Darin Adler]

Comment on attachment 43020 [details]
Patch

> -        // We set m_userEdited to false since this change was not explicty made by the user (say, via typing on the keyboard), see <rdar://problem/5359921>.
> -        m_userEdited = false;
> +        // We set m_lastChangeWasUserEdit to false since this change was not explicty made by the user (say, via typing on the keyboard), see <rdar://problem/5359921>.
> +        m_lastChangeWasUserEdit = false;

It would be nice to fix the spelling of explicitly.

I don't think the "see rdar" comment is really all that useful, even for people at Apple.

I think the comment is no longer needed; the new name of the data member speaks for itself. Or if we really want one, the comment could say, "Set m_lastChangeWasUserEdit to false since this function is only used for programmatic changes, not for user editing."

As you mentioned, we should make other changes here. We don't want people to directly set these with set functions.

[Daniel Bates]

I'll just remove the comment when I land this, since the name of the field implies that we are using it to differentiate programmatic changes from user editing.

(In reply to comment #2)
> (From update of attachment 43020 [details])
> > -        // We set m_userEdited to false since this change was not explicty made by the user (say, via typing on the keyboard), see <rdar://problem/5359921>.
> > -        m_userEdited = false;
> > +        // We set m_lastChangeWasUserEdit to false since this change was not explicty made by the user (say, via typing on the keyboard), see <rdar://problem/5359921>.
> > +        m_lastChangeWasUserEdit = false;
> 
> It would be nice to fix the spelling of explicitly.
> 
> I don't think the "see rdar" comment is really all that useful, even for people
> at Apple.
> 
> I think the comment is no longer needed; the new name of the data member speaks
> for itself. Or if we really want one, the comment could say, "Set
> m_lastChangeWasUserEdit to false since this function is only used for
> programmatic changes, not for user editing."
> 
> As you mentioned, we should make other changes here. We don't want people to
> directly set these with set functions.

[Adam Barth]

Comment on attachment 43020 [details]
Patch

Dan to edit before landing.

[Daniel Bates]

Committed r50999: <http://trac.webkit.org/changeset/50999>

[Daniel Bates]

Committed r51000: <http://trac.webkit.org/changeset/51000>

[Darin Adler]

Comment on attachment 43020 [details]
Patch

Clearing review flag since Daniel will be doing a new patch that covers the rest of the call sites.

[Daniel Bates]

Commit in r50999 but failed to build on the Win build bots. So, rolled it out in r51000.

Will correct the Windows build issues.

[Daniel Bates]

Re-opening bug since we need to fix this issue.

[Daniel Bates]

Created attachment 43256 [details]
Patch

Makes my Windows build smile :-)

[Darin Adler]

Comment on attachment 43256 [details]
Patch

Changing the name of the function in DOMPrivate.idl will break compilation of Safari for Windows, although it doesn't break the compilation of WebKit itself.

I suggest you take the same approach with Windows that you did with Mac OS X. Leave the interface of WebKit for clients outside WebKit with the old name. Change only the names internal to WebKit. So the DOMPrivate.idl name would need to stay the same, as would DOMHTMLClasses.h, and the function names in DOMHTMLClasses.cpp.

[Daniel Bates]

Created attachment 43259 [details]
Patch

Updated patch based on Darin's comment.

[WebKit Commit Bot]

Comment on attachment 43259 [details]
Patch

Clearing flags on attachment: 43259

Committed r51148: <http://trac.webkit.org/changeset/51148>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.