| Summary: | "Plug-in is blocked" message doesn't draw correctly | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ricky Mondello <rmondello> | ||||||||
| Component: | Plug-ins | Assignee: | 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
Ricky Mondello
2015-10-02 00:26:29 PDT
Created attachment 262322 [details]
First patch
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. Also, is this a regression? I am pretty sure the unavailable plugin indicator has worked for blocked Flash in the past. 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). Or even the title from the bug would be fine :D Created attachment 262323 [details]
Second attempt, addressing Tim's feedback
Handle Tim's feedback. Still thinking about testing.
(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. 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? (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 (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. Created attachment 262324 [details]
Third attempt
Actually removing the newline that Tim requested be removed. Still looking into testing.
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. (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! Comment on attachment 262324 [details] Third attempt Clearing flags on attachment: 262324 Committed r190547: <http://trac.webkit.org/changeset/190547> All reviewed patches have been landed. Closing bug. |