Bug 44280

Summary: Add modifier key info to policy client functions in WebKit2
Product: WebKit Reporter: Sam Weinig <sam>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch aroben: review+

Description Sam Weinig 2010-08-19 11:40:00 PDT 
Add modifier key info to policy client functions in WebKit2.
Comment 1 Sam Weinig 2010-08-19 11:51:06 PDT 
Created attachment 64883 [details]
Patch
Comment 2 Adam Roben (:aroben) 2010-08-19 11:56:56 PDT 
Comment on attachment 64883 [details]
Patch

> @@ -521,21 +521,23 @@ void WebPageProxy::didReceiveMessage(Cor
>          case WebPageProxyMessage::DecidePolicyForNavigationAction: {
>              uint64_t frameID;
>              uint32_t navigationType;
> +            uint32_t modifiers;
>              String url;
>              uint64_t listenerID;
> -            if (!arguments->decode(CoreIPC::Out(frameID, navigationType, url, listenerID)))
> +            if (!arguments->decode(CoreIPC::Out(frameID, navigationType, modifiers, url, listenerID)))
>                  return;
> -            decidePolicyForNavigationAction(webFrame(frameID), static_cast<NavigationType>(navigationType), url, listenerID);
> +            decidePolicyForNavigationAction(webFrame(frameID), static_cast<NavigationType>(navigationType), static_cast<WebEvent::Modifiers>(modifiers), url, listenerID);
>              break;
>          }
>          case WebPageProxyMessage::DecidePolicyForNewWindowAction: {
>              uint64_t frameID;
>              uint32_t navigationType;
> +            uint32_t modifiers;

Can we use WebEvent::Modifiers here?


> +static uint32_t modifiersForNavigationAction(const NavigationAction& navigationAction)
> +{
> +    uint32_t modifiers = 0;
> +    if (const UIEventWithKeyState* keyStateEvent = findEventWithKeyState(const_cast<Event*>(navigationAction.event()))) {
> +        if (keyStateEvent->shiftKey())
> +            modifiers |= WebEvent::ShiftKey;
> +        if (keyStateEvent->ctrlKey())
> +            modifiers |= WebEvent::ControlKey;
> +        if (keyStateEvent->altKey())
> +            modifiers |= WebEvent::AltKey;
> +        if (keyStateEvent->metaKey())
> +            modifiers |= WebEvent::MetaKey;
> +    }
> +
> +    return modifiers;
> +}

And here?

>  void WebFrameLoaderClient::dispatchDecidePolicyForMIMEType(FramePolicyFunction function, const String& MIMEType, const ResourceRequest& request)
>  {
>      WebPage* webPage = m_frame->page();
> @@ -436,8 +455,10 @@ void WebFrameLoaderClient::dispatchDecid
>      // FIXME: Pass the frame name.
>      const String& url = request.url().string(); // FIXME: Pass entire request.
>  
> +    uint32_t modifiers = modifiersForNavigationAction(navigationAction);
> +
>      WebProcess::shared().connection()->send(WebPageProxyMessage::DecidePolicyForNewWindowAction, webPage->pageID(),
> -                                            CoreIPC::In(m_frame->frameID(), (uint32_t)navigationAction.type(), url, listenerID));
> +                                            CoreIPC::In(m_frame->frameID(), (uint32_t)navigationAction.type(), modifiers, url, listenerID));
>  }

And here?

>  void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(FramePolicyFunction function, const NavigationAction& navigationAction, const ResourceRequest& request, PassRefPtr<FormState>)
> @@ -451,8 +472,10 @@ void WebFrameLoaderClient::dispatchDecid
>      // FIXME: Pass more than just the navigation action type.
>      const String& url = request.url().string(); // FIXME: Pass entire request.
>  
> +    uint32_t modifiers = modifiersForNavigationAction(navigationAction);
> +
>      WebProcess::shared().connection()->send(WebPageProxyMessage::DecidePolicyForNavigationAction, webPage->pageID(),
> -                                            CoreIPC::In(m_frame->frameID(), (uint32_t)navigationAction.type(), url, listenerID));
> +                                            CoreIPC::In(m_frame->frameID(), (uint32_t)navigationAction.type(), modifiers, url, listenerID));
>  }

And here?

r=me
Comment 3 Sam Weinig 2010-08-19 12:02:05 PDT 
(In reply to comment #2)
> (From update of attachment 64883 [details])
> > @@ -521,21 +521,23 @@ void WebPageProxy::didReceiveMessage(Cor
> >          case WebPageProxyMessage::DecidePolicyForNavigationAction: {
> >              uint64_t frameID;
> >              uint32_t navigationType;
> > +            uint32_t modifiers;
> >              String url;
> >              uint64_t listenerID;
> > -            if (!arguments->decode(CoreIPC::Out(frameID, navigationType, url, listenerID)))
> > +            if (!arguments->decode(CoreIPC::Out(frameID, navigationType, modifiers, url, listenerID)))
> >                  return;
> > -            decidePolicyForNavigationAction(webFrame(frameID), static_cast<NavigationType>(navigationType), url, listenerID);
> > +            decidePolicyForNavigationAction(webFrame(frameID), static_cast<NavigationType>(navigationType), static_cast<WebEvent::Modifiers>(modifiers), url, listenerID);
> >              break;
> >          }
> >          case WebPageProxyMessage::DecidePolicyForNewWindowAction: {
> >              uint64_t frameID;
> >              uint32_t navigationType;
> > +            uint32_t modifiers;
> 
> Can we use WebEvent::Modifiers here?
> 

No. We need to give an explicit size when encoding/decoding. This will all be better with your typed IPC.
Comment 4 Sam Weinig 2010-08-19 14:28:09 PDT 
Landed in r65699.