Bug 54230

Summary: [GTK] Implement WebPage class for WebKit2
Product: WebKit Reporter: Alejandro G. Castro <alex@igalia.com>
Component: WebKit GtkAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, andersca@apple.com, buildbot@hotmail.com, eric@webkit.org, sam@webkit.org, webkit.review.bot@gmail.com
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 From 2011-02-10 11:46:24 PST
Add that class and compile it inside webkit2gtk.
------- Comment #1 From 2011-02-10 11:57:35 PST -------
Created an attachment (id=82020) [details]
Proposed patch
------- Comment #2 From 2011-02-10 12:17:59 PST -------
(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.
------- Comment #3 From 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 From 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 From 2011-02-11 09:20:21 PST -------
(In reply to comment #2)
> (From update of attachment 82020 [details] [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 From 2011-03-07 11:43:22 PST -------
(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: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 From 2011-03-07 12:04:25 PST -------
(In reply to comment #6)
> (From update of attachment 82020 [details] [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 From 2011-03-07 12:23:27 PST -------
(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.
------- Comment #9 From 2011-03-07 12:27:29 PST -------
(In reply to comment #8)
> (From update of attachment 82020 [details] [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 From 2011-03-08 10:29:49 PST -------
Created an attachment (id=85059) [details]
Proposed patch

Adding message to handle commands generation in the UI process.
------- Comment #11 From 2011-03-13 16:41:40 PST -------
(From update of attachment 85059 [details])
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 From 2011-03-14 03:39:43 PST -------
Yep, I would like Apple review of the modifications of the IPC, adding them to the CC.
------- Comment #13 From 2011-03-20 13:30:25 PST -------
Created an attachment (id=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 From 2011-03-20 13:32:33 PST -------
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 From 2011-03-21 16:25:14 PST -------
(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?

> 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 From 2011-03-21 16:30:01 PST -------
Thanks for the review :), I'll fix the issues.
------- Comment #17 From 2011-03-25 06:10:45 PST -------
(In reply to comment #15)
> (From update of attachment 86284 [details] [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 From 2011-03-25 06:11:15 PST -------
Created an attachment (id=86926) [details]
Proposed patch
------- Comment #19 From 2011-03-25 09:11:33 PST -------
(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.

> 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 From 2011-03-25 12:06:15 PST -------
(In reply to comment #19)
> (From update of attachment 86926 [details] [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 From 2011-03-25 12:06:38 PST -------
Created an attachment (id=86969) [details]
Proposed patch
------- Comment #22 From 2011-04-01 15:40:11 PST -------
(From update of attachment 86969 [details])
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 From 2011-04-05 05:21:52 PST -------
(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 From 2011-04-05 05:55:10 PST -------
Landed http://trac.webkit.org/changeset/82929
------- Comment #25 From 2011-04-05 06:14:08 PST -------
http://trac.webkit.org/changeset/82929 might have broken Windows Release (Build) and Windows Debug (Build)