Bug 54230

Summary: [GTK] Implement WebPage class for WebKit2
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, buildbot, eric, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 52805    
Attachments:
Description Flags
Proposed patch
none
Proposed patch
mrobinson: review-
Proposed patch
none
Proposed patch
mrobinson: review-
Proposed patch mrobinson: review+

Description Alejandro G. Castro 2011-02-10 11:46:24 PST
Add that class and compile it inside webkit2gtk.
Comment 1 Alejandro G. Castro 2011-02-10 11:57:35 PST
Created attachment 82020 [details]
Proposed patch
Comment 2 Martin Robinson 2011-02-10 12:17:59 PST
Comment on attachment 82020 [details]
Proposed patch

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

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:83
> +static const KeyDownEntry keyDownEntries[] = {
> +    { VK_LEFT,   0,                  "MoveLeft"                                    },
> +    { VK_LEFT,   ShiftKey,           "MoveLeftAndModifySelection"                  },
> +    { VK_LEFT,   CtrlKey,            "MoveWordLeft"                                },
> +    { VK_LEFT,   CtrlKey | ShiftKey, "MoveWordLeftAndModifySelection"              },
> +    { VK_RIGHT,  0,                  "MoveRight"                                   },
> +    { VK_RIGHT,  ShiftKey,           "MoveRightAndModifySelection"                 },
> +    { VK_RIGHT,  CtrlKey,            "MoveWordRight"                               },
> +    { VK_RIGHT,  CtrlKey | ShiftKey, "MoveWordRightAndModifySelection"             },
> +    { VK_UP,     0,                  "MoveUp"                                      },
> +    { VK_UP,     ShiftKey,           "MoveUpAndModifySelection"                    },
> +    { VK_PRIOR,  ShiftKey,           "MovePageUpAndModifySelection"                },
> +    { VK_DOWN,   0,                  "MoveDown"                                    },

I think I'd rather see this done correctly up front than doing it quickly now and replacing it.  We probably need to plumb through to the UI process to do the mapping between key events and editor actions.
Comment 3 Build Bot 2011-02-10 12:39:34 PST
Attachment 82020 [details] did not build on win:
Build output: http://queues.webkit.org/results/7870424
Comment 4 WebKit Review Bot 2011-02-10 19:09:56 PST
Attachment 82020 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7874518
Comment 5 Alejandro G. Castro 2011-02-11 09:20:21 PST
(In reply to comment #2)
> (From update of attachment 82020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82020&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:83
> > +static const KeyDownEntry keyDownEntries[] = {
> > +    { VK_LEFT,   0,                  "MoveLeft"                                    },
> > +    { VK_LEFT,   ShiftKey,           "MoveLeftAndModifySelection"                  },
> > +    { VK_LEFT,   CtrlKey,            "MoveWordLeft"                                },
> > +    { VK_LEFT,   CtrlKey | ShiftKey, "MoveWordLeftAndModifySelection"              },
> > +    { VK_RIGHT,  0,                  "MoveRight"                                   },
> > +    { VK_RIGHT,  ShiftKey,           "MoveRightAndModifySelection"                 },
> > +    { VK_RIGHT,  CtrlKey,            "MoveWordRight"                               },
> > +    { VK_RIGHT,  CtrlKey | ShiftKey, "MoveWordRightAndModifySelection"             },
> > +    { VK_UP,     0,                  "MoveUp"                                      },
> > +    { VK_UP,     ShiftKey,           "MoveUpAndModifySelection"                    },
> > +    { VK_PRIOR,  ShiftKey,           "MovePageUpAndModifySelection"                },
> > +    { VK_DOWN,   0,                  "MoveDown"                                    },
> 
> I think I'd rather see this done correctly up front than doing it quickly now and replacing it.  We probably need to plumb through to the UI process to do the mapping between key events and editor actions.

Good point, I think just mac is currently doing it, probably is why they decided to do leave it.

I have to review also compilation and check why we needed includes that other ports do not need.
Comment 6 Martin Robinson 2011-03-07 11:43:22 PST
Comment on attachment 82020 [details]
Proposed patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:32
> +#include "Cursor.h"

Is this include necessary here?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:37
> +#include "Font.h"

Why is Font.h necessary here?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:47
> +    notImplemented();

I think you can remove this for now, since we don't actually have an platform initialization to do at the moment.
Comment 7 Alejandro G. Castro 2011-03-07 12:04:25 PST
(In reply to comment #6)
> (From update of attachment 82020 [details])
>
> [...]
>
> > Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:47
> > +    notImplemented();
> 
> I think you can remove this for now, since we don't actually have an platform initialization to do at the moment.

Actually I'm testing now the patch using a sendSync to get the commands list from the UI :), I was planning about adding the input method support in the same patch, hopefully tomorrow I can upload it here. So I think we can wait for that patch.
Comment 8 Martin Robinson 2011-03-07 12:23:27 PST
Comment on attachment 82020 [details]
Proposed patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:40
> +#include "CachedResourceClient.h"
> +#include "CachedResourceHandle.h"
> +#include "Cursor.h"
>  #include "DataReference.h"
>  #include "DecoderAdapter.h"
>  #include "DrawingArea.h"
> +#include "FocusController.h"
> +#include "Font.h"
>  #include "InjectedBundle.h"
>  #include "InjectedBundleBackForwardList.h"
>  #include "MessageID.h"

In fact, all of these changes seem unecessary. Please remove them.
Comment 9 Alejandro G. Castro 2011-03-07 12:27:29 PST
(In reply to comment #8)
> (From update of attachment 82020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82020&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:40
> > +#include "CachedResourceClient.h"
> > +#include "CachedResourceHandle.h"
> > +#include "Cursor.h"
> >  #include "DataReference.h"
> >  #include "DecoderAdapter.h"
> >  #include "DrawingArea.h"
> > +#include "FocusController.h"
> > +#include "Font.h"
> >  #include "InjectedBundle.h"
> >  #include "InjectedBundleBackForwardList.h"
> >  #include "MessageID.h"
> 
> In fact, all of these changes seem unecessary. Please remove them.

Yeah, completely removed in the current version of the patch, it was a problem with X includes I fixed in other patch.
Comment 10 Alejandro G. Castro 2011-03-08 10:29:49 PST
Created attachment 85059 [details]
Proposed patch

Adding message to handle commands generation in the UI process.
Comment 11 Martin Robinson 2011-03-13 16:41:40 PDT
Comment on attachment 85059 [details]
Proposed patch

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

r- because of some really minor things but this looks good to me otherwise! I'd really like to have an Apple person look over the IPC changes, since I'm not particularly familiar with that code.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:139
> +    # Keyboard support messages

Might want to make this slightly more descriptive. "Support for GTK+ platform keybindings" or something like that.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:140
> +    GenerateEditorCommands() -> (Vector<WTF::String> commandsList)

Likewise, this might be better named in a way to describe that it converts a keystroke into a editor commands.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:39
> +    // First try to interpret the command in the UI and get the commands..

Extra period here.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:43
> +    if (!WebProcess::shared().connection()->sendSync(Messages::WebPageProxy::GenerateEditorCommands(),
> +                                                     Messages::WebPageProxy::GenerateEditorCommands::Reply(pendingEditorCommands),
> +                                                     m_page->pageID(), CoreIPC::Connection::NoTimeout))
> +        return;

I don't think this return or if statement are necessary.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:61
> +            success = false;
> +            break;

If you're going to break, you can probably just return false here and dispense with the success variable altogether.
Comment 12 Alejandro G. Castro 2011-03-14 03:39:43 PDT
Yep, I would like Apple review of the modifications of the IPC, adding them to the CC.
Comment 13 Alejandro G. Castro 2011-03-20 13:30:25 PDT
Created attachment 86284 [details]
Proposed patch

Fixed all the issues, thanks for the review, I hope some Apple dev can check the IPC part of the patch.
Comment 14 WebKit Review Bot 2011-03-20 13:32:33 PDT
Attachment 86284 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:59:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Anders Carlsson 2011-03-21 16:25:14 PDT
Comment on attachment 86284 [details]
Proposed patch

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

> Source/WebKit2/UIProcess/gtk/WebView.cpp:300
> +    m_pendingEditorCommands = &commandsList;

Taking a pointer of a stack-based object like this is dangerous. What ensures that we don't end up with a dangling pointer here? Could we fix it in some other way?

> Source/WebKit2/UIProcess/gtk/WebView.cpp:309
> +    if (m_pendingEditorCommands->size() > 0)
> +        return;

This should be 

if (!m_pendingEditorCommands.isEmpty())

> Source/WebKit2/UIProcess/gtk/WebView.cpp:312
> +    static HashMap<int, const char*> keyDownCommandsMap;
> +    static HashMap<int, const char*> keyPressCommandsMap;

These should use the DEFINE_STATIC_LOCAL to avoid exit-time destructors.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:78
> +    if (pendingEditorCommands.size() > 0) {

It's better to do 

if (!pendingEditorCommands.isEmpty()) {

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:122
> +        } else {
> +            // Don't insert null or control characters as they can result in unexpected behaviour
> +            if (event->charCode() < ' ')
> +                return;
> +
> +            // Don't insert anything if a modifier is pressed
> +            if (platformEvent->ctrlKey() || platformEvent->altKey())
> +                return;
> +
> +            if (frame->editor()->insertText(platformEvent->text(), event))
> +                event->setDefaultHandled();
> +        }

This entire block has wrong indentation.
Comment 16 Alejandro G. Castro 2011-03-21 16:30:01 PDT
Thanks for the review :), I'll fix the issues.
Comment 17 Alejandro G. Castro 2011-03-25 06:10:45 PDT
(In reply to comment #15)
> (From update of attachment 86284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86284&action=review
> 
> > Source/WebKit2/UIProcess/gtk/WebView.cpp:300
> > +    m_pendingEditorCommands = &commandsList;
> 
> Taking a pointer of a stack-based object like this is dangerous. What ensures that we don't end up with a dangling pointer here? Could we fix it in some other way?
> 

Yep, you are right, in this case it is used just to pass the pointer to some callbacks that are only called through this method and do not get this pointer, this way we avoid a copy just for getting the result from the callbacks. To make sure no one uses this for other cases I'm adding a = 0, when the method finishes and asserting initially it does no have any value, and I'm adding transient to the name of the attribute.

> > Source/WebKit2/UIProcess/gtk/WebView.cpp:312
> > +    static HashMap<int, const char*> keyDownCommandsMap;
> > +    static HashMap<int, const char*> keyPressCommandsMap;
> 
> These should use the DEFINE_STATIC_LOCAL to avoid exit-time destructors.
> 

I added this to the StdLibExtras.h:

// WEBKIT_COMMA: Required when using macros like DEFINE_STATIC_LOCAL and you
// need to define a template with multiple arguments i.e.
// DEFINE_STATIC_LOCAL(HasMap<type1 COMMA type2>, ...)
#define WEBKIT_COMMA ,
Comment 18 Alejandro G. Castro 2011-03-25 06:11:15 PDT
Created attachment 86926 [details]
Proposed patch
Comment 19 Martin Robinson 2011-03-25 09:11:33 PDT
Comment on attachment 86926 [details]
Proposed patch

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

Looks good, but I think you should switch to making m_pendingEditorCommands an object and just use append(). See below.

> Source/JavaScriptCore/wtf/StdLibExtras.h:48
> +// WEBKIT_COMMA: Required when using macros like DEFINE_STATIC_LOCAL and you
> +// need to define a template with multiple arguments i.e.
> +// DEFINE_STATIC_LOCAL(HasMap<type1 COMMA type2>, ...)
> +#define WEBKIT_COMMA ,

Hrm. I'm unsure about this change. Let's hold off on this or address it in another bug.

> Source/WebKit2/UIProcess/gtk/WebView.cpp:305
> +void WebView::getEditorCommandsForKeyEvent(const NativeWebKeyboardEvent& event, Vector<WTF::String>& commandsList)

commandsList -> commandList

> Source/WebKit2/UIProcess/gtk/WebView.cpp:309
> +    m_transientPendingEditorCommands = &commandsList;

Instead of making this change, maybe it would be better to turn the m_pendingEditorCommands member into an object from a pointer and just do:
m_pendingEditorCommands.clear() here.

> Source/WebKit2/UIProcess/gtk/WebView.cpp:314
> +    gtk_bindings_activate_event(GTK_OBJECT(m_nativeWidget.get()), reinterpret_cast<GdkEventKey*>(const_cast<GdkEvent*>(event.nativeEvent())));
> +#else
> +    gtk_bindings_activate_event(G_OBJECT(m_nativeWidget.get()), reinterpret_cast<GdkEventKey*>(const_cast<GdkEvent*>(event.nativeEvent())));

Here you should use event.nativeEvent()->key to get the key event.

> Source/WebKit2/UIProcess/gtk/WebView.cpp:321
> +    if (m_transientPendingEditorCommands->isEmpty()) {
> +        m_transientPendingEditorCommands = 0;
> +        return;
> +    }
> +

Here you can just do commandsList.append(m_pendingEditorCommands);

> Source/WebKit2/UIProcess/gtk/WebView.cpp:323
> +    DEFINE_STATIC_LOCAL(HashMap<int WEBKIT_COMMA const char*>, keyDownCommandsMap, ());
> +    DEFINE_STATIC_LOCAL(HashMap<int WEBKIT_COMMA const char*>, keyPressCommandsMap, ());

For now instead of using WEBKIT_COMMA, just make a typedef like this at the top of the file:

typedef HashMap<int, const char*> IntConstCharHashMap;

and use that here. I believe it should fix the issues with DEFINE_STATIC_LOCAL.
Comment 20 Alejandro G. Castro 2011-03-25 12:06:15 PDT
(In reply to comment #19)
> (From update of attachment 86926 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86926&action=review
> 
> Looks good, but I think you should switch to making m_pendingEditorCommands an object and just use append(). See below.

Yeah, you are right, I always fall in the nonsense preoptimization. I'll review the patch, thanks for the reviews :).
Comment 21 Alejandro G. Castro 2011-03-25 12:06:38 PDT
Created attachment 86969 [details]
Proposed patch
Comment 22 Martin Robinson 2011-04-01 15:40:11 PDT
Comment on attachment 86969 [details]
Proposed patch

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

Looks good. Please ensure that you have all the necessary WebPage stubs before landing.

> Source/WebKit2/ChangeLog:1
> +2011-03-25  Amruth Raj  <amruthraj@motorola.com> and Ravi Phaneendra Kasibhatla  <ravi.kasibhatla@motorola.com> and Alejandro G. Castro  <alex@igalia.com>

Probably best to use First, Second, and here.

> Source/WebKit2/UIProcess/WebPageProxy.h:-586
> -    

I would restore this newline.

> Source/WebKit2/UIProcess/gtk/WebView.h:68
> +    // Keyboard events handling.

You can omit this coment.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:42
> +                                                m_page->pageID(), CoreIPC::Connection::NoTimeout);

Does it make sense to require a timeout for this?
Comment 23 Alejandro G. Castro 2011-04-05 05:21:52 PDT
(In reply to comment #22)
> > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:42
> > +                                                m_page->pageID(), CoreIPC::Connection::NoTimeout);
> 
> Does it make sense to require a timeout for this?

Did not give much thought about it, I would say this kind of calls should never fail because it is something that just depends on the program processes and not something from the network and the user is expecting a response ASAP.
Comment 24 Alejandro G. Castro 2011-04-05 05:55:10 PDT
Landed http://trac.webkit.org/changeset/82929
Comment 25 WebKit Review Bot 2011-04-05 06:14:08 PDT
http://trac.webkit.org/changeset/82929 might have broken Windows Release (Build) and Windows Debug (Build)