Editor.h includes SelectionController.h only because it needs to access EDirection. We should move this enum into a separate header file so that we don't have to include SelectionController.h in Editor.h
Created attachment 77249 [details] cleanup
(In reply to comment #1) > Created an attachment (id=77249) [details] > cleanup I ended up renaming SelectionController::EDirection to SelectionDirection and putting it in VisibleSelection.h rather than adding a new header. But I did add WritingDirection.h though.
Attachment 77249 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7286113
Attachment 77249 [details] did not build on qt: Build output: http://queues.webkit.org/results/7282090
Attachment 77249 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7202118
Comment on attachment 77249 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=77249&action=review r- for build breaks (and a few minor things that I happened to see). I probably won't give the final r+ on this but thought it valuable to move it past this step. > WebCore/editing/Editor.h:50 > +class CSSMutableStyleDeclaration; Out of order. > WebCore/editing/WritingDirection.h:2 > + * Copyright (C) 2008 Apple Inc. All rights reserved. Multiple things are wrong here....
(In reply to comment #6) > > WebCore/editing/Editor.h:50 > > +class CSSMutableStyleDeclaration; > > Out of order. Oops, will fix. > > WebCore/editing/WritingDirection.h:2 > > + * Copyright (C) 2008 Apple Inc. All rights reserved. > > Multiple things are wrong here.... WritingDirection was added in 2008 by mitz so this copyright information is correct.
Created attachment 77254 [details] Fixed per levin's comment and build breaks
http://trac.webkit.org/changeset/34896 is the change set that added WritingDirection. It was subsequently moved by the following change sets: http://trac.webkit.org/changeset/34935 http://trac.webkit.org/changeset/72573
Comment on attachment 77254 [details] Fixed per levin's comment and build breaks LGTM.
Thanks for the review, Eric! I'll wait until the EWS finishes building because there might be some WebKit code that's using EDirection as there was in mac port. (In reply to comment #10) > (From update of attachment 77254 [details]) > LGTM.
Attachment 77254 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7295116
Created attachment 77258 [details] Replaced all instances of SelectionController::Direction(\w+) by Direction in WebKit
(In reply to comment #11) > Thanks for the review, Eric! I'll wait until the EWS finishes building because there might be some WebKit code that's using EDirection as there was in mac port. (In reply to comment #13) > Created an attachment (id=77258) [details] > Replaced all instances of SelectionController::Direction(\w+) by Direction in WebKit It turned out there were lots of them. I replaced all in this patch. Resubmitting for a review.
Attachment 77254 [details] did not build on qt: Build output: http://queues.webkit.org/results/7336021
Comment on attachment 77258 [details] Replaced all instances of SelectionController::Direction(\w+) by Direction in WebKit View in context: https://bugs.webkit.org/attachment.cgi?id=77258&action=review OK. I figiured there were more includes you would remove. > WebCore/page/Frame.h:37 > +#include "SelectionController.h" Should this be SelectionController.h here?
(In reply to comment #16) > > WebCore/page/Frame.h:37 > > +#include "SelectionController.h" > > Should this be SelectionController.h here? Yes, Frame holds an instance of SelectionController as a member variable.
Attachment 77258 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7258116
(In reply to comment #18) > Attachment 77258 [details] did not build on chromium: > Build output: http://queues.webkit.org/results/7258116 Odd. It seems like my patch was missing the changes to WebFrameImpl.cpp. Will fix before the land.
Attachment 77258 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7270108
Committed r74566: <http://trac.webkit.org/changeset/74566>