Bug 21427 - Implement NSAPI support for WebKit/Mac inside WebCore/plugins
: Implement NSAPI support for WebKit/Mac inside WebCore/plugins
Status: RESOLVED FIXED
: WebKit
Plug-ins
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-10-07 07:00 PST by
Modified: 2008-12-08 00:10 PST (History)


Attachments
Moved implementation of _webkit_isCaseInsensitiveEqualToString to WebCore (2.91 KB, patch)
2008-10-07 07:02 PST, Tor Arne Vestbø
no flags Review Patch | Details | Formatted Diff | Diff
Move _web_encodingForResource from WebKit into WebCore (6.06 KB, patch)
2008-10-07 07:03 PST, Tor Arne Vestbø
no flags Review Patch | Details | Formatted Diff | Diff
Implement PluginPackageMac and PluginViewMac (41.70 KB, patch)
2008-10-07 07:04 PST, Tor Arne Vestbø
no flags Review Patch | Details | Formatted Diff | Diff
Rebased against ToT (4.52 KB, patch)
2008-11-10 08:13 PST, Tor Arne Vestbø
hausmann: review+
Review Patch | Details | Formatted Diff | Diff
Move _web_encodingForResource from WebKit into WebCore and change return type (7.71 KB, patch)
2008-11-10 08:13 PST, Tor Arne Vestbø
hausmann: review+
Review Patch | Details | Formatted Diff | Diff
Implement PluginPackageMac and PluginViewMac, and enable NPAPI support for QtWebKit/Mac (41.93 KB, patch)
2008-11-10 08:13 PST, Tor Arne Vestbø
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch (44.77 KB, patch)
2008-11-12 04:38 PST, Tor Arne Vestbø
hausmann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-10-07 07:00:53 PST
First steps towards NSAPI support for QtWebKit/Mac. 

PluginPackageMac and PluginViewMac should have abstractions for any Qt specific stuff, just like PluginPackage/ViewWin.

No changelog for now, I'll add that once the patches are reviewed and ready to be put in. 

Does the general approach look OK?
------- Comment #1 From 2008-10-07 07:02:46 PST -------
Created an attachment (id=24143) [details]
Moved implementation of _webkit_isCaseInsensitiveEqualToString to WebCore

Basis for patch 0003
------- Comment #2 From 2008-10-07 07:03:11 PST -------
Created an attachment (id=24144) [details]
Move _web_encodingForResource from WebKit into WebCore

Basis for patch 0003
------- Comment #3 From 2008-10-07 07:04:58 PST -------
Created an attachment (id=24145) [details]
Implement PluginPackageMac and PluginViewMac

Sorry, was referencing this one as 0003, but Bugzilla seems to remove the patch number.
------- Comment #4 From 2008-10-08 12:24:49 PST -------
On a more general note: What we're trying to achieve here is an implementation of NPAPI based on the existing PluginPackage/PluginView code that can be used across all ports that run on Mac OS X. It will take a while to become perfect, but we want to start in small steps. These patches are the basis to continue and implement more in smaller steps.
------- Comment #5 From 2008-11-10 08:13:01 PST -------
Created an attachment (id=25016) [details]
Rebased against ToT
------- Comment #6 From 2008-11-10 08:13:23 PST -------
Created an attachment (id=25017) [details]
Move _web_encodingForResource from WebKit into WebCore and change return type

Rebased against ToT
------- Comment #7 From 2008-11-10 08:13:39 PST -------
Created an attachment (id=25019) [details]
Implement PluginPackageMac and PluginViewMac, and enable NPAPI support for QtWebKit/Mac

Rebased against ToT
------- Comment #8 From 2008-11-12 02:23:27 PST -------
(From update of attachment 25016 [details])
Looks good to me, but we should probably mention the reason behind this in the ChangeLog
------- Comment #9 From 2008-11-12 02:24:06 PST -------
(From update of attachment 25017 [details])
Simple patch, looks good to me. We should also mention the reason here.
------- Comment #10 From 2008-11-12 02:38:14 PST -------
(From update of attachment 25019 [details])
> From 97e0aaaa77ee021c33364169ad55a8c001573073 Mon Sep 17 00:00:00 2001
> From: =?utf-8?q?Tor=20Arne=20Vestb=C3=B8?= <tavestbo@trolltech.com>
> Date: Wed, 17 Sep 2008 16:56:24 +0200
> Subject: [PATCH] =?utf-8?q?2008-11-10=20=20Tor=20Arne=20Vestb=C3=B8=20=20<tavestbo@trolltech.com>?=
> MIME-Version: 1.0
> Content-Type: text/plain; charset=utf-8
> Content-Transfer-Encoding: 8bit
> 
>         Reviewed by NOBODY (OOPS!).
> 
>         Implement PluginPackageMac and PluginViewMac, and enable NPAPI support for QtWebKit/Mac

It seems the ChangeLog is missing here.

> --- a/WebCore/plugins/PluginView.cpp
> +++ b/WebCore/plugins/PluginView.cpp
> @@ -138,8 +138,12 @@ void PluginView::frameRectsChanged() const
>  
>  void PluginView::handleEvent(Event* event)
>  {
> -    if (!m_plugin || m_isWindowed)
> +    if (!m_plugin)
>          return;
> +#if !defined(XP_MACOSX)
> +    if (m_isWindowed)
> +        return;
> +#endif

This #ifdef probably deserves a comment.

> @@ -419,6 +423,12 @@ NPError PluginView::setValue(NPPVariable variable, void* value)
>      case NPPVpluginTransparentBool:
>          m_isTransparent = value;
>          return NPERR_NO_ERROR;
> +#if defined(XP_MACOSX)
> +    case NPPVpluginDrawingModel:
> +	return NPERR_NO_ERROR;
> +    case NPPVpluginEventModel:
> +	return NPERR_NO_ERROR;
> +#endif

The indentation is off here.

>          PlatformPluginWidget platformPluginWidget() const { return m_window; }
> +        void setPlatformPluginWidget(PlatformPluginWidget widget) {
> +            if (widget != m_window)
> +                m_window = widget;
> +        }

The assignment is probably faster enough to not require a check for inequality :)


> +static int modifiersForEvent(UIEventWithKeyState *event)

Misplaced * :)


The rest looks good to me
------- Comment #11 From 2008-11-12 02:38:49 PST -------
(From update of attachment 25019 [details])
Ooops, accidentially r+'ed instead of r-. The comments need to be addressed :)
------- Comment #12 From 2008-11-12 02:52:53 PST -------
First two patches landed in r38332 and r38333
------- Comment #13 From 2008-11-12 04:38:10 PST -------
Created an attachment (id=25095) [details]
Updated patch

Fixed comments
------- Comment #14 From 2008-11-12 06:29:18 PST -------
(From update of attachment 25095 [details])
>  public:
>          PlatformPluginWidget platformPluginWidget() const { return m_window; }
> +        void setPlatformPluginWidget(PlatformPluginWidget widget) {
> +                m_window = widget;
> +        }

From the looks of it other code in WebCore puts simple inline setters in one line of code.

> +    if (!dispatchNPEvent(record)) {
> +        LOG(Events, "PluginView::setFocus(): Get-focus event not accepted");
> +    }
[...]
> +    if (!m_isStarted) {
> +        // TODO: Draw the "missing plugin" image
> +        return;
> +    }
[...]
> +    if (!dispatchNPEvent(event)) {
> +        LOG(Events, "PluginView::paint(): Paint event not accepted");
> +    }
[...]
> +    if (!dispatchNPEvent(record)) {
> +        LOG(Events, "PluginView::handleKeyboardEvent(): Keyboard event type %d not accepted",
> +                record.what);
> +    } else {
> +        event->setDefaultHandled();
> +    }
[...]
> +    if (!dispatchNPEvent(record)) {
> +        LOG(Events, "PluginView::nullEventTimerFired(): Null event not accepted");
> +    }

All of these one line control clauses should not use braces.

I'm really nitpicking here now :-), so r+ under the condition that you fix them before landing.
------- Comment #15 From 2008-12-02 16:48:58 PST -------
Have these patches all been landed?  If so, why is this bug still open?
------- Comment #16 From 2008-12-02 23:43:46 PST -------
Ah, sorry Mark, yes they've been landed in r38332, r38333, and r38371. Closing.