Bug 149741

Summary: "Plug-in is blocked" message doesn't draw correctly
Product: WebKit Reporter: Ricky Mondello <rmondello>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, conrad_shultz, rmondello, thorton
Priority: P2    
Version: Safari 9   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
First patch
none
Second attempt, addressing Tim's feedback
none
Third attempt none

Ricky Mondello
Reported 2015-10-02 00:26:29 PDT
When I set Flash to "Block" in Safari 9.0, I don't see a "Plug-in blocked" message on homestarrunner.com, but I should. The placeholder is actually on the page, but isn't drawn.
Attachments
First patch (8.56 KB, patch)
2015-10-02 00:53 PDT, Ricky Mondello
no flags
Second attempt, addressing Tim's feedback (8.51 KB, patch)
2015-10-02 01:05 PDT, Ricky Mondello
no flags
Third attempt (8.36 KB, patch)
2015-10-02 01:27 PDT, Ricky Mondello
no flags
Ricky Mondello
Comment 1 2015-10-02 00:26:57 PDT
Ricky Mondello
Comment 2 2015-10-02 00:53:14 PDT
Created attachment 262322 [details] First patch
Tim Horton
Comment 3 2015-10-02 00:59:38 PDT
Comment on attachment 262322 [details] First patch View in context: https://bugs.webkit.org/attachment.cgi?id=262322&action=review Seems reasonable to me but Anders is the best person for plugin patches; Ccing him. > Source/WebCore/ChangeLog:4 > + No newline here. > Source/WebCore/ChangeLog:6 > + <rdar://problem/22920949> > + https://bugs.webkit.org/show_bug.cgi?id=149741 These are backwards IMO (but nobody strictly follows any rule here anyway). > Source/WebCore/ChangeLog:12 > + No new tests are added. Why not? From a cursory glance, this seems testable.
Tim Horton
Comment 4 2015-10-02 01:00:40 PDT
Also, is this a regression? I am pretty sure the unavailable plugin indicator has worked for blocked Flash in the past.
Tim Horton
Comment 5 2015-10-02 01:01:18 PDT
Also, a pared down version of your first comment would make a better title than the title that you chose (bug title/commit title should describe the problem, not the solution).
Tim Horton
Comment 6 2015-10-02 01:01:36 PDT
Or even the title from the bug would be fine :D
Ricky Mondello
Comment 7 2015-10-02 01:05:50 PDT
Created attachment 262323 [details] Second attempt, addressing Tim's feedback Handle Tim's feedback. Still thinking about testing.
Ricky Mondello
Comment 8 2015-10-02 01:06:56 PDT
(In reply to comment #4) > Also, is this a regression? I am pretty sure the unavailable plugin > indicator has worked for blocked Flash in the past. Yes, this is a regression.
Tim Horton
Comment 9 2015-10-02 01:08:29 PDT
Comment on attachment 262323 [details] Second attempt, addressing Tim's feedback View in context: https://bugs.webkit.org/attachment.cgi?id=262323&action=review > Source/WebKit2/ChangeLog:4 > + You left a newline here :D Perhaps you can repurpose LayoutTests/plugins/unavailable-plugin-indicator-obscurity.html to test this somehow? Or something like it?
Tim Horton
Comment 10 2015-10-02 01:09:41 PDT
(In reply to comment #8) > (In reply to comment #4) > > Also, is this a regression? I am pretty sure the unavailable plugin > > indicator has worked for blocked Flash in the past. > > Yes, this is a regression. Do you know when? That would also be good title-fodder, of course :D
Ricky Mondello
Comment 11 2015-10-02 01:21:02 PDT
(In reply to comment #10) > (In reply to comment #8) > > (In reply to comment #4) > > > Also, is this a regression? I am pretty sure the unavailable plugin > > > indicator has worked for blocked Flash in the past. > > > > Yes, this is a regression. > > Do you know when? That would also be good title-fodder, of course :D I haven't proven it by making my own build, but I strongly suspect that r181562 was the cause.
Ricky Mondello
Comment 12 2015-10-02 01:27:22 PDT
Created attachment 262324 [details] Third attempt Actually removing the newline that Tim requested be removed. Still looking into testing.
Alexey Proskuryakov
Comment 13 2015-10-02 19:45:01 PDT
Comment on attachment 262324 [details] Third attempt View in context: https://bugs.webkit.org/attachment.cgi?id=262324&action=review > Source/WebCore/plugins/PluginData.cpp:127 > + for (unsigned i = 0; i < mimes.size(); ++i) { Please use a modern style C++ loop.
Tim Horton
Comment 14 2015-10-02 21:01:12 PDT
(In reply to comment #13) > Comment on attachment 262324 [details] > Third attempt > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262324&action=review > > > Source/WebCore/plugins/PluginData.cpp:127 > > + for (unsigned i = 0; i < mimes.size(); ++i) { > > Please use a modern style C++ loop. This file is full of parallel vectors; they need the index to index into to the other ones!
WebKit Commit Bot
Comment 15 2015-10-03 19:38:50 PDT
Comment on attachment 262324 [details] Third attempt Clearing flags on attachment: 262324 Committed r190547: <http://trac.webkit.org/changeset/190547>
WebKit Commit Bot
Comment 16 2015-10-03 19:38:56 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.