Bug 45054 - Share more code between HTMLObjectElement and HTMLEmbedElement
Summary: Share more code between HTMLObjectElement and HTMLEmbedElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 45049
  Show dependency tree / graph
 
Reported: 2010-09-01 13:16 PDT by Eric Seidel (no email)
Modified: 2010-09-02 04:29 PDT (History)
3 users (show)

See Also:

Attachments
Patch (29.68 KB, patch)
2010-09-01 13:25 PDT, Eric Seidel (no email) 		dglazkov: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-09-01 13:16:18 PDT 
Share more code between HTMLObjectElement and HTMLEmbedElement
Comment 1 Eric Seidel (no email) 2010-09-01 13:25:57 PDT 
Created attachment 66254 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2010-09-01 13:39:13 PDT 
Comment on attachment 66254 [details]
Patch

This looks great!

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

> WebCore/html/HTMLObjectElement.h:64
> +    
Extra whitespace here.

> WebCore/html/HTMLPlugInElement.cpp:70
> +    
And here.
Comment 3 Eric Seidel (no email) 2010-09-01 19:14:21 PDT 
Whitespace has been cleaned up some in later patches.  Gonna see if this cq's clean.
Comment 4 WebKit Commit Bot 2010-09-01 21:30:21 PDT 
Comment on attachment 66254 [details]
Patch

Rejecting patch 66254 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20898 test cases.
svg/custom/getsvgdocument.html -> failed

Exiting early after 1 failures. 18060 tests run.
370.48s total testing time

18059 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
26 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3926037
Comment 5 Eric Seidel (no email) 2010-09-01 23:31:41 PDT 
Fixing.  Will repost for landing shortly.
Comment 6 Eric Seidel (no email) 2010-09-02 01:37:36 PDT 
I'm going to land this in smaller pieces.  I can't seem to find the regression in the moving code, but I'll make sure all the pieces I land land w/o regression.
Comment 7 Eric Seidel (no email) 2010-09-02 02:10:40 PDT 
Landing this in two pieces.  First part landed as r66650.
Comment 8 Eric Seidel (no email) 2010-09-02 03:06:32 PDT 
Piece 2 as r66653
Comment 9 Eric Seidel (no email) 2010-09-02 03:22:26 PDT 
Actually it's going to be 3 pieces.  I've reduced the diff causing the failure, now just trying to figure out what part of it is wrong. :)
Comment 10 Eric Seidel (no email) 2010-09-02 03:58:45 PDT 
I guess it's going to be 4 pieces.  #3 was r66657.  I'm down to only like 50 lines left.  Still haven't found the bug.
Comment 11 Eric Seidel (no email) 2010-09-02 04:25:07 PDT 
Turns out the insanity that is:

void HTMLPlugInImageElement::recalcStyle(StyleChange ch)
{
    // FIXME: Why is this necessary?  Manual re-attach is almost always wrong.
    if (!useFallbackContent() && needsWidgetUpdate() && renderer() && !isImageType()) {
        detach();
        attach();
    }
    HTMLPlugInElement::recalcStyle(ch);
}

needed to be moved from <object> down to the base class too.  Who knows why.  This code is black magic.
Comment 12 Eric Seidel (no email) 2010-09-02 04:29:36 PDT 
Committed r66659: <http://trac.webkit.org/changeset/66659>