Bug 108369 – Add class name for snapshotted plugin based on dimensions

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

[Dean Jackson]

Add a CSS class to the shadow root, based on dimensions. This will allow the style to apply a different layout.

[Radar WebKit Bug Importer]

<rdar://problem/13117808>

[Dean Jackson]

Created attachment 187754 [details]
Patch

[Benjamin Poulain]

Comment on attachment 187754 [details]
Patch

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

Not a clue about correctness, but some comments:

> Source/WebCore/html/HTMLPlugInImageElement.cpp:334
> +    shadowContainer->setAttribute(classAttr, classNameForShadowRootSize(document()->page()->mainFrame()->view()->contentsSize(), this));   

Aren't attribute AtomicString?
classNameForShadowRootSize() returns a String.

> Source/WebCore/page/FrameView.cpp:2484
> +        // Caution: it's possible the object was destroyed again, since loading a
> +        // plugin may run any arbitrary javascript.
> +        embeddedObject->updateWidgetPosition();

If arbitrary JavaScript can be run, can you be sure embeddedObject always exist at this point?

> Source/WebCore/page/FrameView.cpp:2501
>      objects.reserveCapacity(size);

This should use reserveInitialCapacity.

[Dean Jackson]

Comment on attachment 187754 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:334
>> +    shadowContainer->setAttribute(classAttr, classNameForShadowRootSize(document()->page()->mainFrame()->view()->contentsSize(), this));   
> 
> Aren't attribute AtomicString?
> classNameForShadowRootSize() returns a String.

Great point. I've changed everything to AtomicString.

>> Source/WebCore/page/FrameView.cpp:2484
>> +        embeddedObject->updateWidgetPosition();
> 
> If arbitrary JavaScript can be run, can you be sure embeddedObject always exist at this point?

This is existing code, so I'm not sure exactly, but I *think* this is protected by the ref and deref against the RenderArena in the calling function.

>> Source/WebCore/page/FrameView.cpp:2501
>>      objects.reserveCapacity(size);
> 
> This should use reserveInitialCapacity.

Fixed.

[Dean Jackson]

Created attachment 187894 [details]
Patch

[Benjamin Poulain]

Comment on attachment 187894 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:306
> +        return String();

I'd appreciate if you can change this to nullAtom before landing () (for builds with NO_IMPLICIT_ATOMICSTRING). :)

[Simon Fraser (smfr)]

Comment on attachment 187894 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2501
> -    objects.reserveCapacity(size);
> +    Vector<RenderObject*> objects;
> +    objects.reserveInitialCapacity(size);

Why this change (it's not explained in the changelog)?

[Dean Jackson]

Comment on attachment 187894 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:306
>> +        return String();
> 
> I'd appreciate if you can change this to nullAtom before landing () (for builds with NO_IMPLICIT_ATOMICSTRING). :)

Oops. :)

>> Source/WebCore/page/FrameView.cpp:2501
>> +    objects.reserveInitialCapacity(size);
> 
> Why this change (it's not explained in the changelog)?

Because Ben requested it.

[Dean Jackson]

Committed r142685: <http://trac.webkit.org/changeset/142685>