Move glue/EditorClientImpl.h/cpp to webkit/api/src

Move glue/EditorClientImpl.h/cpp to webkit/api/src

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30270

[yaar, 2009-10-26 22:01:21]

Quite boring - mostly reformating.
(I created a tool to automatically do that, so it wasn't so boring ;))

[darin (slow to review), 2009-10-26 23:32:26]

ooh, i like tools!  but it has some bugs...

http://codereview.chromium.org/330021/diff/1/2
File webkit/api/src/EditorClientImpl.cpp (right):

http://codereview.chromium.org/330021/diff/1/2#newcode47
Line 47: #undef LOG
can this LOG thing be deleted now?

http://codereview.chromium.org/330021/diff/1/2#newcode59
Line 59: using WebKit::PasswordAutocompleteListener;
none of these using WebKit declarations are needed now that all of the code
lives in the WebKit namespace.

it'd be very nice to write "using namespace WebCore" here and then eliminate all
of the WebCore:: prefixes down below.

http://codereview.chromium.org/330021/diff/1/2#newcode91
Line 91: // Our lifetime is bound to the WebViewImpl.
nit: indent by 4

http://codereview.chromium.org/330021/diff/1/2#newcode311
Line 311: void EditorClientImpl::registerCommandForRedo(
nit: no newline here

http://codereview.chromium.org/330021/diff/1/2#newcode359
Line 359: // The below code was adapted from the WebKit file webview.cpp
licenses are supposed to be per-file, so we're going to have to either break
this code out into a separate file or make the apple license apply to the entire
file.  i prefer breaking it out.  this should probably happen as a separate CL.

http://codereview.chromium.org/330021/diff/1/2#newcode537
Line 537: const char* EditorClientImpl::interpretKeyEvent(
nit: no new line here

http://codereview.chromium.org/330021/diff/1/2#newcode553
Line 553: | keyDownEntries[i].virtualKey,
strange indenting here

http://codereview.chromium.org/330021/diff/1/2#newcode559
Line 559: | keyPressEntries[i].charCode,
ditto

http://codereview.chromium.org/330021/diff/1/2#newcode594
Line 594: WebCore::String command_name = interpretKeyEvent(evt);
commandName

http://codereview.chromium.org/330021/diff/1/2#newcode636
Line 636: // TODO(ukai): investigate more detail for various keyboard layout.
TODO -> FIXME

http://codereview.chromium.org/330021/diff/1/2#newcode683
Line 683: void EditorClientImpl::handleInputMethodKeydown(
nit: no newline here

http://codereview.chromium.org/330021/diff/1/2#newcode738
Line 738: bool EditorClientImpl::Autofill(WebCore::HTMLInputElement*
input_element,
variable names should be camelCase

http://codereview.chromium.org/330021/diff/1/2#newcode787
Line 787: bool
strange indenting here

http://codereview.chromium.org/330021/diff/1/2#newcode803
Line 803: WebFrameImpl::FromFrame(input_element->document()->frame());
ditto

http://codereview.chromium.org/330021/diff/1/2#newcode817
Line 817: ASSERT(static_cast<int> (name.length()) > 0);
nit: no space between "static_cast<int>" and "(name."

http://codereview.chromium.org/330021/diff/1/2#newcode830
Line 830: void EditorClientImpl::OnAutofillSuggestionAccepted(
nit: no newline here

http://codereview.chromium.org/330021/diff/1/2#newcode849
Line 849: || event->keyCode() == WebCore::VKEY_DELETE;
strange indenting here

http://codereview.chromium.org/330021/diff/1/2#newcode881
Line 881: int spell_location = -1;
spellLocation

http://codereview.chromium.org/330021/diff/1/2#newcode882
Line 882: int spell_length = 0;
spellLength

http://codereview.chromium.org/330021/diff/1/2#newcode886
Line 886: m_webView->client()->spellCheck(WebString(text, length),
nit: no line wrap here and then drop the brackets

http://codereview.chromium.org/330021/diff/1/3
File webkit/api/src/EditorClientImpl.h (right):

http://codereview.chromium.org/330021/diff/1/3#newcode134
Line 134: virtual bool ShowFormAutofillForNode(WebCore::Node* node);
nit: showFormAutofillForNode

nit: please remove the parameter name here since it doesn't add information

http://codereview.chromium.org/330021/diff/1/3#newcode140
Line 140: virtual void OnAutofillSuggestionAccepted(WebCore::HTMLInputElement*
text_field);
nit: didAcceptAutofillSuggestion

text_field -> textField (but i think you can just leave off this parameter)

http://codereview.chromium.org/330021/diff/1/3#newcode155
Line 155: bool Autofill(WebCore::HTMLInputElement* input_element,
nit: autofill

nit: please use webkit style for parameter names and leave off parameter names
that do not add information

http://codereview.chromium.org/330021/diff/1/3#newcode163
Line 163: void DoAutofill(WebCore::Timer<EditorClientImpl>*);
nit: doAutofill

http://codereview.chromium.org/330021/diff/1/3#newcode165
Line 165: void CancelPendingAutofill();
nit: cancelPendingAutofill

http://codereview.chromium.org/330021/diff/1/3#newcode174
Line 174: bool ShouldSpellcheckByDefault();
nit: shouldSpellcheckByDefault

nit: down below "Spellcheck" is "SpellCheck"... we should be consistent.

http://codereview.chromium.org/330021/diff/1/3#newcode187
Line 187: // turned off. The default setting is SPELLCHECK_AUTOMATIC.
nit: SpellCheckAutomatic

http://codereview.chromium.org/330021/diff/1/3#newcode188
Line 188: enum {
can this be a named enum?

[yaar, 2009-10-27 00:30:19]

All fixed.

http://codereview.chromium.org/330021/diff/1/2
File webkit/api/src/EditorClientImpl.cpp (right):

http://codereview.chromium.org/330021/diff/1/2#newcode359
Line 359: // The below code was adapted from the WebKit file webview.cpp
Shouldn't we use the same disclaimer as the one below also at the top of the
file, and have both Apple and Google on it?

[darin (slow to review), 2009-10-27 04:32:47]

> Shouldn't we use the same disclaimer as the one below also at the top > of the
file, and have both Apple and Google on it?

I think that's a fine solution.  If the snippet of borrowed code is easy to
factor out into a helper, then I'd go with a separate file.  Otherwise, just
using the older license at the top with an extra Copyright Google line should be
good.  That's done elsewhere at least.

[darin (slow to review), 2009-10-27 04:33:41]

http://codereview.chromium.org/330021/diff/4001/5002
File webkit/api/src/EditorClientImpl.h (right):

http://codereview.chromium.org/330021/diff/4001/5002#newcode87
Line 87: virtual void registerCommandForUndo(PassRefPtr<WebCore::EditCommand> );
nit: extra space before ")" ... ditto for line below here

[darin (slow to review), 2009-10-27 04:33:59]

it looks like a revised version hasn't been uploaded yet.

[yaar, 2009-10-27 16:41:38]

.h file fixed.
The code was uploaded, but because its new file, there is no side-by-side view.
But you can see the changes in the "diffs from patch set".

[yaar, 2009-10-27 17:15:24]

ready for (re)review

[darin (slow to review), 2009-10-27 17:28:31]

http://codereview.chromium.org/330021/diff/5011/5012
File webkit/api/src/EditorClientImpl.cpp (right):

http://codereview.chromium.org/330021/diff/5011/5012#newcode44
Line 44: #include "webkit/api/public/WebEditingAction.h"
please drop the google-style include paths for everything except
webkit/glue/webview_impl.h (since we still need that one)

http://codereview.chromium.org/330021/diff/5011/5012#newcode62
Line 62: static const size_t kMaximumUndoStackDepth = 1000;
nit: webkit style just uses variable style naming for constants.  so these
should be maximumUndoStackDepth, etc.

http://codereview.chromium.org/330021/diff/5011/5012#newcode69
Line 69: : m_webView(webview),
nit: webkit style puts the commas in a column underneath the colon, like this:

    Foo::Foo()
        : ...
        , ...
        , ...
    {
        ...
    }

http://codereview.chromium.org/330021/diff/5011/5012#newcode246
Line 246: // FIXME(darin): Pass a reference to the CSSStyleDeclaration somehow.
nit: webkit style is to just do FIXME instead of FIXME(user)

http://codereview.chromium.org/330021/diff/5011/5012#newcode575
Line 575: if (m_webView->client()) {
nit: no {}'s around single line statements

http://codereview.chromium.org/330021/diff/5011/5013
File webkit/api/src/EditorClientImpl.h (right):

http://codereview.chromium.org/330021/diff/5011/5013#newcode48
Line 48: EditorClientImpl(WebViewImpl* webview);
nit: webView

http://codereview.chromium.org/330021/diff/5011/5013#newcode64
Line 64: virtual bool shouldBeginEditing(WebCore::Range* range);
nit: it is webkit style to not mention parameter names when they don't add
information.  so range, node, action, affinity and style can all be deleted.

http://codereview.chromium.org/330021/diff/5011/5013#newcode134
Line 134: virtual bool showFormAutofillForNode(WebCore::Node* node);
nit: drop the node parameter name

[yaar, 2009-10-27 18:10:48]

One more time.
(Soundtrack included, so you don't get bored:
http://www.youtube.com/watch?v=lH-0s0pRleg)

[darin (slow to review), 2009-10-27 18:20:02]

LGTM w/ one nit that you can fix before committing:

http://codereview.chromium.org/330021/diff/9002/10001
File webkit/api/src/EditorClientImpl.cpp (right):

http://codereview.chromium.org/330021/diff/9002/10001#newcode687
Line 687: HTMLInputElement* input_element = WebKit::toHTMLInputElement(node);
inputElement