Bug 50396 - REGRESSION(r72783): DOMActivate fires multiple times from input type=file
Summary: REGRESSION(r72783): DOMActivate fires multiple times from input type=file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 49986
  Show dependency treegraph
 
Reported: 2010-12-02 09:40 PST by Dimitri Glazkov (Google)
Modified: 2010-12-03 09:56 PST (History)
1 user (show)

See Also:


Attachments
Patch (4.39 KB, patch)
2010-12-02 15:01 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2010-12-03 09:23 PST, Dimitri Glazkov (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2010-12-02 09:40:55 PST
This results in a confused ChromeClient being called twice, and file upload dialog popping up multiple times in multiprocess port (Chromium and I bet WebKit2 as well).
Comment 1 Dimitri Glazkov (Google) 2010-12-02 15:01:55 PST
Created attachment 75417 [details]
Patch
Comment 2 Darin Adler 2010-12-02 15:12:47 PST
Comment on attachment 75417 [details]
Patch

Is there any way to make a regression test for this? Does this only affect <input type=file>?
Comment 3 Dimitri Glazkov (Google) 2010-12-02 15:19:44 PST
(In reply to comment #2)
> (From update of attachment 75417 [details])
> Is there any way to make a regression test for this? Does this only affect <input type=file>?

I am writing a test, sorry :) Didn't mean to mark for review.
Comment 4 Dimitri Glazkov (Google) 2010-12-03 09:23:33 PST
Created attachment 75502 [details]
Patch
Comment 5 Dimitri Glazkov (Google) 2010-12-03 09:23:57 PST
Now with a layout test :)
Comment 6 Darin Adler 2010-12-03 09:36:04 PST
Comment on attachment 75502 [details]
Patch

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

> WebCore/dom/Node.cpp:1205
> +    if (!node)
> +        return false;
> +    if (this == node)
> +        return true;
> +    for (ContainerNode* n = node->parentOrHostNode(); n; n = n->parentOrHostNode()) {
> +        if (n == this)
> +            return true;
> +    }
> +    return false;

I like the way you wrote this, but I would have written it this way:

    for (Node* n = node; n; n = n->parentOrHostNode()) {
        if (n == this)
            return true;
    }
    return false;

No need for the special handling of the node itself.
Comment 7 Dimitri Glazkov (Google) 2010-12-03 09:41:45 PST
(In reply to comment #6)
> (From update of attachment 75502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75502&action=review
> 
> > WebCore/dom/Node.cpp:1205
> > +    if (!node)
> > +        return false;
> > +    if (this == node)
> > +        return true;
> > +    for (ContainerNode* n = node->parentOrHostNode(); n; n = n->parentOrHostNode()) {
> > +        if (n == this)
> > +            return true;
> > +    }
> > +    return false;
> 
> I like the way you wrote this, but I would have written it this way:
> 
>     for (Node* n = node; n; n = n->parentOrHostNode()) {
>         if (n == this)
>             return true;
>     }
>     return false;
> 
> No need for the special handling of the node itself.

I like it. I'll change before landing.
Comment 8 Dimitri Glazkov (Google) 2010-12-03 09:56:18 PST
Committed r73270: <http://trac.webkit.org/changeset/73270>