Bug 108369

Summary: Add class name for snapshotted plugin based on dimensions
Product: WebKit Reporter: Dean Jackson <dino>
Component: Plug-insAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, eric, ojan.autocc, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch simon.fraser: review+

Description Dean Jackson 2013-01-30 12:29:53 PST
Add a CSS class to the shadow root, based on dimensions. This will allow the style to apply a different layout.
Comment 1 Radar WebKit Bug Importer 2013-01-30 12:30:03 PST
<rdar://problem/13117808>
Comment 2 Dean Jackson 2013-02-11 19:13:42 PST
Created attachment 187754 [details]
Patch
Comment 3 Benjamin Poulain 2013-02-12 00:19:17 PST
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.
Comment 4 Dean Jackson 2013-02-12 10:42:02 PST
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.
Comment 5 Dean Jackson 2013-02-12 10:44:03 PST
Created attachment 187894 [details]
Patch
Comment 6 Benjamin Poulain 2013-02-12 12:24:45 PST
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). :)
Comment 7 Simon Fraser (smfr) 2013-02-12 15:25:31 PST
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)?
Comment 8 Dean Jackson 2013-02-12 15:26:53 PST
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.
Comment 9 Dean Jackson 2013-02-12 15:56:51 PST
Committed r142685: <http://trac.webkit.org/changeset/142685>