Bug 51441 - Editor.h doesn't need to include SelectionController.h
: Editor.h doesn't need to include SelectionController.h
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-12-21 19:33 PST by
Modified: 2010-12-23 11:00 PST (History)


Attachments
cleanup (51.24 KB, patch)
2010-12-22 12:39 PST, Ryosuke Niwa
no flags Review Patch | 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 Review Patch | 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+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-12-22 12:39:48 PST -------
Created an attachment (id=77249) [details]
cleanup
------- Comment #2 From 2010-12-22 12:42:17 PST -------
(In reply to comment #1)
> Created an attachment (id=77249) [details] [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 From 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 From 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 From 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 From 2010-12-22 13:09:48 PST -------
(From update of attachment 77249 [details])
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 From 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 From 2010-12-22 13:28:43 PST -------
Created an attachment (id=77254) [details]
Fixed per levin's comment and build breaks
------- Comment #9 From 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 From 2010-12-22 13:35:20 PST -------
(From update of attachment 77254 [details])
LGTM.
------- Comment #11 From 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] [details])
> LGTM.
------- Comment #12 From 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 From 2010-12-22 13:58:28 PST -------
Created an attachment (id=77258) [details]
Replaced all instances of SelectionController::Direction(\w+) by Direction in WebKit
------- Comment #14 From 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] [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 From 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 From 2010-12-22 14:16:48 PST -------
(From update of attachment 77258 [details])
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 From 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 From 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 From 2010-12-22 14:35:06 PST -------
(In reply to comment #18)
> Attachment 77258 [details] [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 From 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 From 2010-12-23 11:00:07 PST -------
Committed r74566: <http://trac.webkit.org/changeset/74566>