Bug 51306 – [Qt] Refactor EditorClientQt::handleKeyboardEvent

• Component: HTML Editing
• Status: RESOLVED FIXED

[Antonio Gomes]

The method now is a complete mess, although it could be much simpler.

I plan to refactor it in 2 or maybe 3 steps, so it will be a much patch bug.

First one coming ....

[Antonio Gomes]

Created attachment 76959 [details]
patch 1 - v1

[WebKit Review Bot]

Attachment 76959 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/EditorClientQt.cpp']" exit_code: 1
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:498:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Created attachment 76960 [details]
patch 1 - v1.1

Fixed the style issue.

[Antonio Gomes]

Created attachment 76966 [details]
patch 2 - v1

Second part of the refactory. Methods is *much* cleaner now.

Not up to review yet, because it depends on the first one to be in.

[Kenneth Rohde Christiansen]

Comment on attachment 76960 [details]
patch 1 - v1.1

View in context: https://bugs.webkit.org/attachment.cgi?id=76960&action=review

Did you think about this in a WebKit2 sense?

> WebKit/qt/ChangeLog:11
> +        in qwebpage.cpp). However, there are some keyCodes associated to any no QAction,

any no? I dont understand you.

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
> +    // Ctrl+DELETE and Ctrl+BACKSPACE are covered by QWebPagePrivate::editorCommandForWebActions().

why write DELETE with upper case letters?

[Antonio Gomes]

(In reply to comment #5)
> (From update of attachment 76960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76960&action=review
> 
> Did you think about this in a WebKit2 sense?

No actually. As I said, it is just a refactor for the current code. All functionality should behavior exactly the same, kenne. Do you have specifc webkit2 issue in your mind?

> > WebKit/qt/ChangeLog:11
> > +        in qwebpage.cpp). However, there are some keyCodes associated to any no QAction,
> 
> any no? I dont understand you.

"However, there are some keyCodes associated to any QAction," ... consider it fixed locally.

> > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
> > +    // Ctrl+DELETE and Ctrl+BACKSPACE are covered by QWebPagePrivate::editorCommandForWebActions().
> 
> why write DELETE with upper case letters?

I can remove the uppercase, but as I said, this comment was already existent, and I kept it as it. Prefer to change it?

[Antonio Gomes]

Created attachment 77112 [details]
patch 3 - comprises patch 1 and 2

I thought doing it in smaller chunks were make reviewing easier, however it failed. I am upload a new patch with both patch 1 and 2 together.

[WebKit Review Bot]

Attachment 77112 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/EditorClientQt.cpp']" exit_code: 1
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:512:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Created attachment 77151 [details]
patch 3 - v2

Fixed the style issue.

[Kenneth Rohde Christiansen]

Comment on attachment 77151 [details]
patch 3 - v2

View in context: https://bugs.webkit.org/attachment.cgi?id=77151&action=review

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:359
> +// We are not mapping all editor commands here since most of them are being handled in QWebPage.cpp.

There is no file called QWebPage.cpp it is called qwebpage.cpp

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:388
> +const char* interpretKeyEvent(const KeyboardEvent* event)

from the method name it is not obvious what it returns

[Antonio Gomes]

Created attachment 77224 [details]
patch 3 - v3

- Fixed QWebPage.cpp -> qwebpage.cpp
- renamed interpretKeyEvent to editorCommandForKeyCodeAndModifiers

[Antonio Gomes]

Created attachment 77225 [details]
patch 3 - v4 - right file.

see changes in comment #11.

[Kenneth Rohde Christiansen]

Comment on attachment 77225 [details]
patch 3 - v4 - right file.

View in context: https://bugs.webkit.org/attachment.cgi?id=77225&action=review

How is this tested? Maybe the ChangeLog could make that clear

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:353
> +struct KeyDownEntry {

What about KeyUp?

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:361
> +// We are not mapping all editor commands here since most of them are being handled in qwebpage.cpp.
> +// The ones we are mapping here are either not being handled in QWebPage, or need special
> +// handling for Spatial Navigation or Caret Browsing features.

This could be simplified.

// Handle key presses here that are needed for spatial navigation and caret browsing. All other key presses are handled directly in QWebPage.

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:386
> +#define ARRAYSIZE(array) (sizeof(array) / sizeof((array)[0]))

I dont really see the point in this one :-) you use it in one place only. just do something like

int numEntries = sizeof(keyDownEntries) / sizeof ...

That would make the code cleaner

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:392
> +    static HashMap<int, const char*>* keyDownCommandsMap = 0;

Why is this a pointer?

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:399
> +    if (!keyDownCommandsMap) {
> +        keyDownCommandsMap = new HashMap<int, const char*>;
> +
> +        for (unsigned i = 0; i < ARRAYSIZE(keyDownEntries); i++)
> +            keyDownCommandsMap->set(keyDownEntries[i].modifiers << 16 | keyDownEntries[i].virtualKey, keyDownEntries[i].editorCommand);
> +    }

So you create a static array outside the method, and then here you create a hashmap for it. Can't that be simplified?

> WebKit/qt/WebCoreSupport/EditorClientQt.cpp:459
> +            if (!commandName.isEmpty()) {

Shouldn't we assert if something is not handled instead? as that means that we need to add it to EditorClient or QWebPage, right?

[Antonio Gomes]

(In reply to comment #13)
> (From update of attachment 77225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77225&action=review
> 
> How is this tested? Maybe the ChangeLog could make that clear

I am testing it by running the layout tests in LayoutTests/editing and seeing no regressions. As it is a refactor only patch, it should just work as it is now.

I can working on improving test coverage (via Qt autotests) for editing, but it can come later maybe (?)


> > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:353
> > +struct KeyDownEntry {
> 
> What about KeyUp?

what matters for editing is keydown and keypress. We bail out earlier in the case of keyup:

402 void EditorClientQt::handleKeyboardEvent(KeyboardEvent* event)
403 {
      ...
407 
408   const PlatformKeyboardEvent* kevent = event->keyEvent();
409   if (!kevent || kevent->type() == PlatformKeyboardEvent::KeyUp)
410     return;

> > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:392
> > +    static HashMap<int, const char*>* keyDownCommandsMap = 0;
> 
> Why is this a pointer?

Fixed. Not a pointer anymore.

> > WebKit/qt/WebCoreSupport/EditorClientQt.cpp:459
> > +            if (!commandName.isEmpty()) {
> 
> Shouldn't we assert if something is not handled instead? as that means that we need to add it to EditorClient or QWebPage, right?

I can add asserts yes, but I would like to do this in follow ups if you agree too. Again, it is mean to just be a refactor.

[Antonio Gomes]

Created attachment 77787 [details]
patch 3 - v5 (committed r74891- r=kenneth)

Addressed Kenneth's requests.

[WebKit Review Bot]

Attachment 77787 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/qt/ChangeLog', u'WebKit/qt/WebCoreSupport/EditorClientQt.cpp']" exit_code: 1
WebKit/qt/WebCoreSupport/EditorClientQt.cpp:504:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Comment on attachment 77787 [details]
patch 3 - v5 (committed r74891- r=kenneth)

Clearing flags on attachment: 77787

Committed r74891: <http://trac.webkit.org/changeset/74891>

[Yi Shen]

Antonio, I saw some crash on my linux. Do you have any idea about it? Thx

Program received signal SIGSEGV, Segmentation fault.
0x01e66fb7 in WebCore::editorCommandForKeyDownEvent (event=0x83f9f18)
    at ../../../WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
380	    ASSERT(event->type() == eventNames().keydownEvent);
(gdb) i s
#0  0x01e66fb7 in WebCore::editorCommandForKeyDownEvent (event=0x83f9f18)
    at ../../../WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
#1  0x01e6745c in WebCore::EditorClientQt::handleKeyboardEvent (
    this=0x823e780, event=0x83f9f18)

(gdb) p event->type()
$1 = "keypress"

[Antonio Gomes]

(In reply to comment #18)
> Antonio, I saw some crash on my linux. Do you have any idea about it? Thx
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x01e66fb7 in WebCore::editorCommandForKeyDownEvent (event=0x83f9f18)
>     at ../../../WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
> 380        ASSERT(event->type() == eventNames().keydownEvent);
> (gdb) i s
> #0  0x01e66fb7 in WebCore::editorCommandForKeyDownEvent (event=0x83f9f18)
>     at ../../../WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
> #1  0x01e6745c in WebCore::EditorClientQt::handleKeyboardEvent (
>     this=0x823e780, event=0x83f9f18)
> 
> (gdb) p event->type()
> $1 = "keypress"

I can have a look, yi. Steps to reproduce?

[Yi Shen]

(In reply to comment #19)
> (In reply to comment #18)
> > Antonio, I saw some crash on my linux. Do you have any idea about it? Thx
> > 
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x01e66fb7 in WebCore::editorCommandForKeyDownEvent (event=0x83f9f18)
> >     at ../../../WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
> > 380        ASSERT(event->type() == eventNames().keydownEvent);
> > (gdb) i s
> > #0  0x01e66fb7 in WebCore::editorCommandForKeyDownEvent (event=0x83f9f18)
> >     at ../../../WebKit/qt/WebCoreSupport/EditorClientQt.cpp:380
> > #1  0x01e6745c in WebCore::EditorClientQt::handleKeyboardEvent (
> >     this=0x823e780, event=0x83f9f18)
> > 
> > (gdb) p event->type()
> > $1 = "keypress"
> 
> I can have a look, yi. Steps to reproduce?

I can reproduce it by 
1) launched QtTestBrowser, 
2) then went to google.com, 
3) typed a character in the search field on the page through the keyboard.

[Antonio Gomes]

> 
> I can reproduce it by 
> 1) launched QtTestBrowser, 
> 2) then went to google.com, 
> 3) typed a character in the search field on the page through the keyboard.

Could you tell me if this:

-    ASSERT(event->type() == eventNames().keydownEvent);
+    if (event->type() == eventNames().keydownEvent)
+        return "";

fixes it? If so, I will commit it and reopen to proper fix. I know that is the right thing ... thanks!

[Antonio Gomes]

 
> Could you tell me if this:
> 
> -    ASSERT(event->type() == eventNames().keydownEvent);
> +    if (event->type() == eventNames().keydownEvent)

err, it should be !=

[Yi Shen]

(In reply to comment #22)
> > Could you tell me if this:
> > 
> > -    ASSERT(event->type() == eventNames().keydownEvent);
> > +    if (event->type() == eventNames().keydownEvent)
> 
> err, it should be !=

I will try it and let you know. (Give me 30 minutes to rebuild it :))

[Antonio Gomes]

Created attachment 77829 [details]
patch 4 - v1

[Antonio Gomes]

Comment on attachment 77829 [details]
patch 4 - v1

Follow up asserting hit fix.

[Yi Shen]

(In reply to comment #25)
> (From update of attachment 77829 [details])
> Follow up asserting hit fix.

Hi Antonio, I tested your changes on linux, no crash can be reproduced :)

[Antonio Gomes]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 77829 [details] [details])
> > Follow up asserting hit fix.
> 
> Hi Antonio, I tested your changes on linux, no crash can be reproduced :)

Thanks Yi.

Reopen the bug for CQ to work.

[WebKit Commit Bot]

Comment on attachment 77829 [details]
patch 4 - v1

Clearing flags on attachment: 77829

Committed r74932: <http://trac.webkit.org/changeset/74932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.