Bug 23623

Summary: Windowed Flash instances aren't captured when a WebView receives a WM_PRINTCLIENT message
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Plug-insAssignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, hyatt, jhoneycutt
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on:    
Bug Blocks: 23691    
Attachments:
Description Flags
patch v1 + ChangeLog darin: review+

Description Adam Roben (:aroben) 2009-01-29 09:54:56 PST
To reproduce:

1. Go to <http://homestarrunner.com/>
2. Drag a tab out of the Safari window

The Flash instance doesn't appear in the dragged-out tab mini-window. This is because Flash isn't responding to WM_PRINTCLIENT.
Comment 1 Adam Roben (:aroben) 2009-01-29 09:56:07 PST
<rdar://problem/6513921>
Comment 2 Adam Roben (:aroben) 2009-01-29 10:18:38 PST
Created attachment 27151 [details]
patch v1 + ChangeLog
Comment 3 Darin Adler 2009-01-29 10:26:28 PST
Comment on attachment 27151 [details]
patch v1 + ChangeLog

> +    sysCallID = *reinterpret_cast<unsigned*>(pProc + 1);

This is an unaligned read. Is this the best way to do this? In other cases in the past I've done this sort of thing by fetching the bytes individually and shifting them into place. On some platforms that was required.

> +    *reinterpret_cast<unsigned*>(pProc + 1) = reinterpret_cast<unsigned>(pNewProc) - reinterpret_cast<unsigned>(pProc + 5);

Why do we need to cast pNewProc to an integer? The difference between two pointers is an integer, so normally I'd expect the variables to be cast to a pointer type. Or to intptr_t.

> +void PluginView::setUpOffscreenPaintingHooks()

Can this be a non-member function instead? I'd prefer to see it not mentioned in the header file at all.

> +void PluginView::paintWindowedPluginIntoContext(GraphicsContext* context, const IntRect& rect) const

I don't think the words "IntoContext" add much in the name of this function. The fact that the function takes a context seems to be enough.

> +        Fix Bug 23623: Windowed Flash instances don't respond to
> +        WM_PRINTCLIENT

I think the name of the bug is wrong. This is describing what's wrong with the plug-in, not what effects this has on WebKit clients nor what we're changing about WebKit. The title of the bug will still be true after it's fixed.

> -void WebView::updateBackingStore(FrameView* frameView, HDC dc, bool backingStoreCompletelyDirty)
> +void WebView::updateBackingStore(FrameView* frameView, HDC dc, bool backingStoreCompletelyDirty, bool shouldIncludeChildWindows)

Booleans stink for things like this! I much prefer enums or avoiding the boolean entirely.

r=me
Comment 4 Adam Roben (:aroben) 2009-01-29 10:41:55 PST
(In reply to comment #3)
> (From update of attachment 27151 [details] [review])
> > +    sysCallID = *reinterpret_cast<unsigned*>(pProc + 1);
> 
> This is an unaligned read. Is this the best way to do this? In other cases in
> the past I've done this sort of thing by fetching the bytes individually and
> shifting them into place. On some platforms that was required.

I don't know if it's the best way.

> > +    *reinterpret_cast<unsigned*>(pProc + 1) = reinterpret_cast<unsigned>(pNewProc) - reinterpret_cast<unsigned>(pProc + 5);
> 
> Why do we need to cast pNewProc to an integer? The difference between two
> pointers is an integer, so normally I'd expect the variables to be cast to a
> pointer type. Or to intptr_t.

The code is this way currently because that's how it was on fengyuan.com (though the original code used C-style casts). I'll try changing to casting to intptr_t.

> > +void PluginView::setUpOffscreenPaintingHooks()
> 
> Can this be a non-member function instead? I'd prefer to see it not mentioned
> in the header file at all.

It can be a non-member function if I pass in pointers to the HookedBeginPaint/HookedEndPaint functions (which, come to think of it, should start with lowercase letters). I'll make that change.

> > +void PluginView::paintWindowedPluginIntoContext(GraphicsContext* context, const IntRect& rect) const
> 
> I don't think the words "IntoContext" add much in the name of this function.
> The fact that the function takes a context seems to be enough.

The reason I added "IntoContext" is because we already have a paint() function that takes a GraphicsContext that is called for both windowed and windowless plugins, but that normally doesn't do anything for windowed plugins. So I think "IntoContext" adds a little clarity, but maybe not very much.

> > +        Fix Bug 23623: Windowed Flash instances don't respond to
> > +        WM_PRINTCLIENT
> 
> I think the name of the bug is wrong. This is describing what's wrong with the
> plug-in, not what effects this has on WebKit clients nor what we're changing
> about WebKit. The title of the bug will still be true after it's fixed.

OK, I'll retitle it to "Windowed Flash instances aren't captured when a WebView receives a WM_PRINTCLIENT message"

> > -void WebView::updateBackingStore(FrameView* frameView, HDC dc, bool backingStoreCompletelyDirty)
> > +void WebView::updateBackingStore(FrameView* frameView, HDC dc, bool backingStoreCompletelyDirty, bool shouldIncludeChildWindows)
> 
> Booleans stink for things like this! I much prefer enums or avoiding the
> boolean entirely.

I'm not sure how to avoid the boolean in this case without doing extra work every time we paint the area beneath a windowed plugin. I can switch to using an enum.

Thanks for the review!
Comment 5 Adam Roben (:aroben) 2009-01-29 11:21:37 PST
Landed in r40366