Bug 149741 – "Plug-in is blocked" message doesn't draw correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Ricky Mondello]

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.

[Ricky Mondello]

<rdar://problem/22920949>

[Ricky Mondello]

Created attachment 262322 [details]
First patch

[Tim Horton]

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]

Also, is this a regression? I am pretty sure the unavailable plugin indicator has worked for blocked Flash in the past.

[Tim Horton]

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]

Or even the title from the bug would be fine :D

[Ricky Mondello]

Created attachment 262323 [details]
Second attempt, addressing Tim's feedback

Handle Tim's feedback. Still thinking about testing.

[Ricky Mondello]

(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 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]

(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]

(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]

Created attachment 262324 [details]
Third attempt

Actually removing the newline that Tim requested be removed. Still looking into testing.

[Alexey Proskuryakov]

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]

(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 on attachment 262324 [details]
Third attempt

Clearing flags on attachment: 262324

Committed r190547: <http://trac.webkit.org/changeset/190547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.