Bug 92320

Summary: HTMLAppletElement should inherit from HTMLPlugInImageElement
Product: WebKit Reporter: Anders Carlsson <andersca>
Component: New BugsAssignee: Anders Carlsson <andersca>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, gustavo, japhet, jochen, ossy, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch eric: review+, webkit-ews: commit-queue-

Description Anders Carlsson 2012-07-25 17:00:47 PDT
HTMLAppletElement should inherit from HTMLPlugInImageElement
Comment 1 Anders Carlsson 2012-07-25 17:05:21 PDT
Created attachment 154495 [details]
Patch
Comment 2 Early Warning System Bot 2012-07-25 17:58:52 PDT
Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13339446
Comment 3 WebKit Review Bot 2012-07-25 18:32:37 PDT
Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13341441
Comment 4 Early Warning System Bot 2012-07-25 19:10:44 PDT
Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13345437
Comment 5 Gyuyoung Kim 2012-07-25 19:57:56 PDT
Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13349460
Comment 6 Gustavo Noronha (kov) 2012-07-25 21:19:09 PDT
Comment on attachment 154495 [details]
Patch

Attachment 154495 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13341491
Comment 7 Anders Carlsson 2012-07-26 14:30:41 PDT
Created attachment 154753 [details]
Patch
Comment 8 Eric Seidel (no email) 2012-07-26 14:40:30 PDT
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?
Comment 9 Anders Carlsson 2012-07-26 14:41:39 PDT
(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.
Comment 10 Eric Seidel (no email) 2012-07-26 14:43:47 PDT
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?
Comment 11 Eric Seidel (no email) 2012-07-26 14:47:07 PDT
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.
Comment 12 Anders Carlsson 2012-07-26 14:50:14 PDT
(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.
Comment 13 Early Warning System Bot 2012-07-26 15:22:22 PDT
Comment on attachment 154753 [details]
Patch

Attachment 154753 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13374026
Comment 14 Anders Carlsson 2012-07-26 16:09:08 PDT
Committed r123811: <http://trac.webkit.org/changeset/123811>
Comment 15 Csaba Osztrogon√°c 2012-07-26 23:10:37 PDT
(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."
Comment 16 Anders Carlsson 2012-07-27 09:40:07 PDT
(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" ;)