Bug 167268 - When Safari reloads pages with Flash objects after Flash is installed, placeholders don't paint (but do work!)
Summary: When Safari reloads pages with Flash objects after Flash is installed, placeh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-20 16:50 PST by Anders Carlsson
Modified: 2017-01-23 17:57 PST (History)
3 users (show)

See Also:


Attachments
Patch (7.26 KB, patch)
2017-01-20 16:52 PST, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (7.19 KB, patch)
2017-01-23 10:46 PST, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2017-01-23 11:43 PST, Anders Carlsson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2017-01-20 16:50:05 PST
When Safari reloads pages with Flash objects after Flash is installed, placeholders don't paint (but do work!)
Comment 1 Anders Carlsson 2017-01-20 16:52:46 PST
Created attachment 299404 [details]
Patch
Comment 2 Ricky Mondello 2017-01-20 17:30:28 PST
<rdar://problem/29857388>
Comment 3 Anders Carlsson 2017-01-23 10:46:12 PST
Created attachment 299525 [details]
Patch
Comment 4 Anders Carlsson 2017-01-23 11:43:08 PST
Created attachment 299529 [details]
Patch
Comment 5 Sam Weinig 2017-01-23 12:53:37 PST
Comment on attachment 299529 [details]
Patch

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

> Source/WebCore/page/Page.cpp:540
>      Vector<Ref<Frame>> framesNeedingReload;

Can you remove this? It seems unused now.

> Source/WebCore/page/Page.cpp:546
> +        pluginInfoProvider->refresh(reload);

Is there anything ensuring the PluginInfoProviders are kept alive for this loop? If they are tied to the page's lifetime, do we really need the two loops?

> Source/WebCore/plugins/PluginInfoProvider.cpp:57
> +    for (auto& frame : framesNeedingReload)
> +        frame->loader().reload();

For a specific page, is there any reason to reload a frame that is a child of a frame that has already been reloaded in this loop? For instance, if the main frame is reloaded, what is the value of reloading any subframes?
Comment 6 Sam Weinig 2017-01-23 12:54:54 PST
Oh, as always, is this testable?
Comment 7 Anders Carlsson 2017-01-23 12:59:52 PST
Committed r211052: <http://trac.webkit.org/changeset/211052>
Comment 8 Darin Adler 2017-01-23 17:57:38 PST
Comment on attachment 299529 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:540
>>      Vector<Ref<Frame>> framesNeedingReload;
> 
> Can you remove this? It seems unused now.

Patch landed with this unused vector still there. Should remove it!