Bug 50396 – REGRESSION(r72783): DOMActivate fires multiple times from input type=file

• Component: DOM
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

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).

[Dimitri Glazkov (Google)]

Created attachment 75417 [details]
Patch

[Darin Adler]

Comment on attachment 75417 [details]
Patch

Is there any way to make a regression test for this? Does this only affect <input type=file>?

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

Created attachment 75502 [details]
Patch

[Dimitri Glazkov (Google)]

Now with a layout test :)

[Darin Adler]

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.

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

Committed r73270: <http://trac.webkit.org/changeset/73270>