HTMLAppletElement should inherit from HTMLPlugInImageElement

[Anders Carlsson]

HTMLAppletElement should inherit from HTMLPlugInImageElement

[Anders Carlsson]

Created attachment 154495 [details]
Patch

[Early Warning System Bot]

Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13339446

[WebKit Review Bot]

Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13341441

[Early Warning System Bot]

Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13345437

[Gyuyoung Kim]

Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13349460

[Gustavo Noronha (kov)]

Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13341491

[Anders Carlsson]

Created attachment 154753 [details]
Patch

[Eric Seidel (no email)]

So <applet> is only for java-plugin content, right?  And <embed> is only for other plugins? And <object> is for both plugins and images?  Is that correct?

[Anders Carlsson]

(In reply to comment #8)
> So <applet> is only for java-plugin content, right?  And <embed> is only for other plugins? And <object> is for both plugins and images?  Is that correct?

This is partly true, except that Java applets can be instantiated with <embed> and <object> as well.

[Eric Seidel (no email)]

So it seems the only trick in validating this patch is that we didn't give <applet> too many powers its not supposed to have, right?

[Eric Seidel (no email)]

Comment on attachment 154753 [details]
Patch

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

This seems reasonable to me.  It's very difficult to verify that this is correct from just seeing the code.  I hope we have testing... :)

> Source/WebCore/html/HTMLAppletElement.cpp:42
> +    : HTMLPlugInImageElement(tagName, document, createdByParser, ShouldNotPreferPlugInsForImages)

ShouldNotPreferPlugins?

> Source/WebCore/html/HTMLAppletElement.cpp:153
> +    renderer->setWidget(frame->loader()->subframeLoader()->createJavaAppletWidget(roundedIntSize(LayoutSize(contentWidth, contentHeight)), this, paramNames, paramValues));

Do we worry that we're now going to be exposing a different set of params to the java plugin? It's not clear to me that we are, but is one possible concern here.

[Anders Carlsson]

(In reply to comment #11)
> (From update of attachment 154753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154753&action=review
> 
> This seems reasonable to me.  It's very difficult to verify that this is correct from just seeing the code.  I hope we have testing... :)
> 
> > Source/WebCore/html/HTMLAppletElement.cpp:42
> > +    : HTMLPlugInImageElement(tagName, document, createdByParser, ShouldNotPreferPlugInsForImages)
> 
> ShouldNotPreferPlugins?

It doesn't really matter - it's impossible for an <applet> tag to end up creating an image.

> 
> > Source/WebCore/html/HTMLAppletElement.cpp:153
> > +    renderer->setWidget(frame->loader()->subframeLoader()->createJavaAppletWidget(roundedIntSize(LayoutSize(contentWidth, contentHeight)), this, paramNames, paramValues));
> 
> Do we worry that we're now going to be exposing a different set of params to the java plugin? It's not clear to me that we are, but is one possible concern here.

Nah, the only thing that could break was if we had multiple parameters with the same names and different values but doing that is undefined behavior anyway.

[Early Warning System Bot]

Comment on attachment 154753 [details]
Patch

Attachment 154753 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13374026

[Anders Carlsson]

Committed r123811: <http://trac.webkit.org/changeset/123811>

[Csaba Osztrogonác]

(In reply to comment #14)
> Committed r123811: <http://trac.webkit.org/changeset/123811>

And Qt buildfix landed in http://trac.webkit.org/changeset/123838

Why do you break Qt build regularly and intentionally? Qt EWS was red,
EWS commented the bug, but you landed your wrong patch. Please don't do 
that next time.

https://www.webkit.org/coding/contributing.html
"Your change must at least compile on all platforms."

[Anders Carlsson]

(In reply to comment #15)
> (In reply to comment #14)
> > Committed r123811: <http://trac.webkit.org/changeset/123811>
> 
> And Qt buildfix landed in http://trac.webkit.org/changeset/123838
> 
> Why do you break Qt build regularly and intentionally? Qt EWS was red,
> EWS commented the bug, but you landed your wrong patch. Please don't do 
> that next time.
> 

I did fix the error the bot reported, but there were more errors that weren't reported by the EWS bot.

The code is correct - this is a bug in the compiler which is why none of the other builds broke. Maybe if you switched to a better compiler I'd stop "intentionally breaking the build" ;)