Bug 31186

Summary: Cleanup: Rename fields RenderTextControl::m_edited and RenderTextControl::m_userEdited
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, brunner, commit-queue, darin, dbates, eric, jparent, justin.garcia, michelangelo, ojan, thomas.godart_wk
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
abarth: commit-queue-
Patch
darin: review-
Patch none

Daniel Bates
Reported 2009-11-05 13:44:52 PST
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.
Attachments
Patch (9.74 KB, patch)
2009-11-11 16:55 PST, Daniel Bates
abarth: commit-queue-
Patch (13.35 KB, patch)
2009-11-15 16:34 PST, Daniel Bates
darin: review-
Patch (11.29 KB, patch)
2009-11-15 17:13 PST, Daniel Bates
no flags
Daniel Bates
Comment 1 2009-11-11 16:55:40 PST
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 2 2009-11-12 08:36:04 PST
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
Comment 3 2009-11-12 10:43:45 PST
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 4 2009-11-12 18:19:25 PST
Comment on attachment 43020 [details] Patch Dan to edit before landing.
Daniel Bates
Comment 5 2009-11-15 14:56:11 PST
Daniel Bates
Comment 6 2009-11-15 15:16:04 PST
Darin Adler
Comment 7 2009-11-15 15:16:53 PST
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
Comment 8 2009-11-15 15:17:55 PST
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
Comment 9 2009-11-15 16:33:15 PST
Re-opening bug since we need to fix this issue.
Daniel Bates
Comment 10 2009-11-15 16:34:08 PST
Created attachment 43256 [details] Patch Makes my Windows build smile :-)
Darin Adler
Comment 11 2009-11-15 16:55:13 PST
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
Comment 12 2009-11-15 17:13:19 PST
Created attachment 43259 [details] Patch Updated patch based on Darin's comment.
WebKit Commit Bot
Comment 13 2009-11-18 15:46:17 PST
Comment on attachment 43259 [details] Patch Clearing flags on attachment: 43259 Committed r51148: <http://trac.webkit.org/changeset/51148>
WebKit Commit Bot
Comment 14 2009-11-18 15:46:31 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.