Bug 51441 - Editor.h doesn't need to include SelectionController.h
Summary: Editor.h doesn't need to include SelectionController.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-21 19:33 PST by Ryosuke Niwa
Modified: 2010-12-23 11:00 PST (History)
10 users (show)

See Also:


Attachments
cleanup (51.24 KB, patch)
2010-12-22 12:39 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per levin's comment and build breaks (51.77 KB, patch)
2010-12-22 13:28 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Replaced all instances of SelectionController::Direction(\w+) by Direction in WebKit (62.12 KB, patch)
2010-12-22 13:58 PST, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-12-21 19:33:01 PST
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
Comment 1 Ryosuke Niwa 2010-12-22 12:39:48 PST
Created attachment 77249 [details]
cleanup
Comment 2 Ryosuke Niwa 2010-12-22 12:42:17 PST
(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.
Comment 3 WebKit Review Bot 2010-12-22 12:55:16 PST
Attachment 77249 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7286113
Comment 4 Early Warning System Bot 2010-12-22 13:02:21 PST
Attachment 77249 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7282090
Comment 5 WebKit Review Bot 2010-12-22 13:08:54 PST
Attachment 77249 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7202118
Comment 6 David Levin 2010-12-22 13:09:48 PST
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....
Comment 7 Ryosuke Niwa 2010-12-22 13:23:43 PST
(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.
Comment 8 Ryosuke Niwa 2010-12-22 13:28:43 PST
Created attachment 77254 [details]
Fixed per levin's comment and build breaks
Comment 9 Ryosuke Niwa 2010-12-22 13:33:17 PST
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 10 Eric Seidel (no email) 2010-12-22 13:35:20 PST
Comment on attachment 77254 [details]
Fixed per levin's comment and build breaks

LGTM.
Comment 11 Ryosuke Niwa 2010-12-22 13:44:27 PST
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.
Comment 12 WebKit Review Bot 2010-12-22 13:50:06 PST
Attachment 77254 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7295116
Comment 13 Ryosuke Niwa 2010-12-22 13:58:28 PST
Created attachment 77258 [details]
Replaced all instances of SelectionController::Direction(\w+) by Direction in WebKit
Comment 14 Ryosuke Niwa 2010-12-22 13:59:48 PST
(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.
Comment 15 Early Warning System Bot 2010-12-22 14:01:04 PST
Attachment 77254 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7336021
Comment 16 Eric Seidel (no email) 2010-12-22 14:16:48 PST
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?
Comment 17 Ryosuke Niwa 2010-12-22 14:18:45 PST
(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.
Comment 18 WebKit Review Bot 2010-12-22 14:21:31 PST
Attachment 77258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7258116
Comment 19 Ryosuke Niwa 2010-12-22 14:35:06 PST
(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.
Comment 20 WebKit Review Bot 2010-12-22 15:47:41 PST
Attachment 77258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7270108
Comment 21 Ryosuke Niwa 2010-12-23 11:00:07 PST
Committed r74566: <http://trac.webkit.org/changeset/74566>