Bug 108369 - Add class name for snapshotted plugin based on dimensions
Summary: Add class name for snapshotted plugin based on dimensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-01-30 12:29 PST by Dean Jackson
Modified: 2013-02-12 15:56 PST (History)
5 users (show)

See Also:


Attachments
Patch (15.93 KB, patch)
2013-02-11 19:13 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (16.84 KB, patch)
2013-02-12 10:44 PST, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>