Bug 51441 – Editor.h doesn't need to include SelectionController.h

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 77249 [details]
cleanup

[Ryosuke Niwa]

(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.

[WebKit Review Bot]

Attachment 77249 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7286113

[Early Warning System Bot]

Attachment 77249 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7282090

[WebKit Review Bot]

Attachment 77249 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7202118

[David Levin]

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....

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 77254 [details]
Fixed per levin's comment and build breaks

[Ryosuke Niwa]

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

[Eric Seidel (no email)]

Comment on attachment 77254 [details]
Fixed per levin's comment and build breaks

LGTM.

[Ryosuke Niwa]

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.

[WebKit Review Bot]

Attachment 77254 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7295116

[Ryosuke Niwa]

Created attachment 77258 [details]
Replaced all instances of SelectionController::Direction(\w+) by Direction in WebKit

[Ryosuke Niwa]

(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.

[Early Warning System Bot]

Attachment 77254 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7336021

[Eric Seidel (no email)]

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?

[Ryosuke Niwa]

(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.

[WebKit Review Bot]

Attachment 77258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7258116

[Ryosuke Niwa]

(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.

[WebKit Review Bot]

Attachment 77258 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7270108

[Ryosuke Niwa]

Committed r74566: <http://trac.webkit.org/changeset/74566>