Can node->renderer()->node() == node ever be false?
Dear WebKit, I've been looking at this line here and it doesn't seem to make sense to me: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2153 It looks like the loop in question will always exit early, because it short-circuits to node->renderer()->node() == node, which seems like it always will be true. At least, that's what the layout tests say when I remove it. The loop was added here: http://trac.webkit.org/changeset/25057/trunk/WebCore/page/EventHandler.cpp Can the elders and the wizards of WebKit shine the light upon my forehead and provide me with the mirth of understanding? :DG<
On Sep 14, 2010, at 6:46 PM, Dimitri Glazkov wrote:
I've been looking at this line here and it doesn't seem to make sense to me: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2153
It looks like the loop in question will always exit early, because it short-circuits to node->renderer()->node() == node, which seems like it always will be true. At least, that's what the layout tests say when I remove it.
I don't see anything in that loop that is equivalent to node->renderer()->node() == node. All I see are null-checks. Note that line 2154 declares a new variable with the name "node". I don't know anything else about this code or what you're asking, though. -Adam
Sorry, I meant node->renderer()->node() != 0. My bad. This loop will always exit in in the first iteration. :DG< On Tue, Sep 14, 2010 at 4:45 PM, Adam Roben <aroben@apple.com> wrote:
On Sep 14, 2010, at 6:46 PM, Dimitri Glazkov wrote:
I've been looking at this line here and it doesn't seem to make sense to me: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2153
It looks like the loop in question will always exit early, because it short-circuits to node->renderer()->node() == node, which seems like it always will be true. At least, that's what the layout tests say when I remove it.
I don't see anything in that loop that is equivalent to node->renderer()->node() == node. All I see are null-checks. Note that line 2154 declares a new variable with the name "node".
I don't know anything else about this code or what you're asking, though.
-Adam
On Sep 14, 2010, at 5:41 PM, Dimitri Glazkov wrote:
Sorry, I meant node->renderer()->node() != 0. My bad. This loop will always exit in in the first iteration.
It definitely is possible for renderers to have a null result from node(). I do not know for sure that it's impossible for node->renderer()->node() to be null under any circumstances. Anonymous renderers and inline continuations are among the ways a null node pointers. It might be that in all such circumstances, the renderer won't be returned by any node's renderer() method. It would be worth some analysis. - Maciej
:DG<
On Tue, Sep 14, 2010 at 4:45 PM, Adam Roben <aroben@apple.com> wrote:
On Sep 14, 2010, at 6:46 PM, Dimitri Glazkov wrote:
I've been looking at this line here and it doesn't seem to make sense to me: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2153
It looks like the loop in question will always exit early, because it short-circuits to node->renderer()->node() == node, which seems like it always will be true. At least, that's what the layout tests say when I remove it.
I don't see anything in that loop that is equivalent to node->renderer()->node() == node. All I see are null-checks. Note that line 2154 declares a new variable with the name "node".
I don't know anything else about this code or what you're asking, though.
-Adam
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Sounds good, I will look into this. FWIW, when I remove this loop, I see no regressions in layout tests, including the tests added with the same commit as the loop. :DG< On Tue, Sep 14, 2010 at 5:55 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 14, 2010, at 5:41 PM, Dimitri Glazkov wrote:
Sorry, I meant node->renderer()->node() != 0. My bad. This loop will always exit in in the first iteration.
It definitely is possible for renderers to have a null result from node(). I do not know for sure that it's impossible for node->renderer()->node() to be null under any circumstances. Anonymous renderers and inline continuations are among the ways a null node pointers. It might be that in all such circumstances, the renderer won't be returned by any node's renderer() method. It would be worth some analysis.
- Maciej
:DG<
On Tue, Sep 14, 2010 at 4:45 PM, Adam Roben <aroben@apple.com> wrote:
On Sep 14, 2010, at 6:46 PM, Dimitri Glazkov wrote:
I've been looking at this line here and it doesn't seem to make sense to me: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2153
It looks like the loop in question will always exit early, because it short-circuits to node->renderer()->node() == node, which seems like it always will be true. At least, that's what the layout tests say when I remove it.
I don't see anything in that loop that is equivalent to node->renderer()->node() == node. All I see are null-checks. Note that line 2154 declares a new variable with the name "node".
I don't know anything else about this code or what you're asking, though.
-Adam
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Ok, after following down a few rabbit holes, I can with some degree of certainty say that node->renderer()->node() == 0 can only at the end of RenderWidget::destroy(), but that is clearly not the case we're fishing for. Even if we modify the loop to start at the parent, we're jumping into the render tree to find a node to fire an event on, which is unhealthy for various layering reasons. In summary, to quote the great protagonists from a TV show: "Exterminate! Exterminate!" :DG< On Tue, Sep 14, 2010 at 7:19 PM, Dimitri Glazkov <dglazkov@chromium.org> wrote:
Sounds good, I will look into this. FWIW, when I remove this loop, I see no regressions in layout tests, including the tests added with the same commit as the loop.
:DG<
On Tue, Sep 14, 2010 at 5:55 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 14, 2010, at 5:41 PM, Dimitri Glazkov wrote:
Sorry, I meant node->renderer()->node() != 0. My bad. This loop will always exit in in the first iteration.
It definitely is possible for renderers to have a null result from node(). I do not know for sure that it's impossible for node->renderer()->node() to be null under any circumstances. Anonymous renderers and inline continuations are among the ways a null node pointers. It might be that in all such circumstances, the renderer won't be returned by any node's renderer() method. It would be worth some analysis.
- Maciej
:DG<
On Tue, Sep 14, 2010 at 4:45 PM, Adam Roben <aroben@apple.com> wrote:
On Sep 14, 2010, at 6:46 PM, Dimitri Glazkov wrote:
I've been looking at this line here and it doesn't seem to make sense to me: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2153
It looks like the loop in question will always exit early, because it short-circuits to node->renderer()->node() == node, which seems like it always will be true. At least, that's what the layout tests say when I remove it.
I don't see anything in that loop that is equivalent to node->renderer()->node() == node. All I see are null-checks. Note that line 2154 declares a new variable with the name "node".
I don't know anything else about this code or what you're asking, though.
-Adam
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Any takers? :) https://bugs.webkit.org/show_bug.cgi?id=45821 :DG< On Tue, Sep 14, 2010 at 9:16 PM, Dimitri Glazkov <dglazkov@chromium.org> wrote:
Ok, after following down a few rabbit holes, I can with some degree of certainty say that node->renderer()->node() == 0 can only at the end of RenderWidget::destroy(), but that is clearly not the case we're fishing for.
Even if we modify the loop to start at the parent, we're jumping into the render tree to find a node to fire an event on, which is unhealthy for various layering reasons.
In summary, to quote the great protagonists from a TV show: "Exterminate! Exterminate!"
:DG<
On Tue, Sep 14, 2010 at 7:19 PM, Dimitri Glazkov <dglazkov@chromium.org> wrote:
Sounds good, I will look into this. FWIW, when I remove this loop, I see no regressions in layout tests, including the tests added with the same commit as the loop.
:DG<
On Tue, Sep 14, 2010 at 5:55 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 14, 2010, at 5:41 PM, Dimitri Glazkov wrote:
Sorry, I meant node->renderer()->node() != 0. My bad. This loop will always exit in in the first iteration.
It definitely is possible for renderers to have a null result from node(). I do not know for sure that it's impossible for node->renderer()->node() to be null under any circumstances. Anonymous renderers and inline continuations are among the ways a null node pointers. It might be that in all such circumstances, the renderer won't be returned by any node's renderer() method. It would be worth some analysis.
- Maciej
:DG<
On Tue, Sep 14, 2010 at 4:45 PM, Adam Roben <aroben@apple.com> wrote:
On Sep 14, 2010, at 6:46 PM, Dimitri Glazkov wrote:
I've been looking at this line here and it doesn't seem to make sense to me: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2153
It looks like the loop in question will always exit early, because it short-circuits to node->renderer()->node() == node, which seems like it always will be true. At least, that's what the layout tests say when I remove it.
I don't see anything in that loop that is equivalent to node->renderer()->node() == node. All I see are null-checks. Note that line 2154 declares a new variable with the name "node".
I don't know anything else about this code or what you're asking, though.
-Adam
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
participants (3)
-
Adam Roben
-
Dimitri Glazkov
-
Maciej Stachowiak