Bug 102765 – Remove unneeded null check in NodeRendererFactory::createRendererIfNeeded

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

Remove unneeded null check in NodeRendererFactory::createRendererIfNeeded

[Elliott Sprehn]

Created attachment 175136 [details]
Patch

[Ojan Vafai]

Comment on attachment 175136 [details]
Patch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:255
> +    ASSERT(m_context.parentRenderer());

Can you move this line to be above the if/else. It makes more sense to me to assert before the line that depends on the assert.

[Ojan Vafai]

Actually, even better, but it directly in the else statement.

[Brent Fulgham]

I was about to r- because I think the ASSERTs should be positioned higher in the code flow, but Ojan beat me to it.

If m_context.parentRenderer() might ever ASSERT, then it's possible the new code will blow up before having a chance to assert.

I also had a question about the behavior of parentRenderer(), which I think is probably NOT a problem, but I'd feel better if you confirmed it.

> Source/WebCore/ChangeLog:10
> +        so there's no reason to check for it again.

Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null?  From a brief look it seems like the answer is no.  But if I'm wrong, this change would modify the behavior.  Can you please confirm that this is not a problem?

[Elliott Sprehn]

(In reply to comment #3)
> Actually, even better, but it directly in the else statement.

I'm not actually asserting about the line in the else, I'm asserting about the rendererIsNeeded() on the following line after the assert. Calling that method on a context that doesn't have a style or a parentRenderer is always a mistake.

Also if I put it in the else it makes it look like if  isElementNode() is true and we don't go down the else that we may not have a renderer (which is never true). Also if we do go through that else we'll crash on the null dereference so there's no reason to ASSERT about it.

[Elliott Sprehn]

(In reply to comment #4)
> I was about to r- because I think the ASSERTs should be positioned higher in the code flow, but Ojan beat me to it.
> 
> If m_context.parentRenderer() might ever ASSERT, then it's possible the new code will blow up before having a chance to assert.

Yes, that was intended. We do this all over webkit, and it's even been requested by Apple on my patches before that I NOT assert about nulls and instead just let the program crash the null dereference.

> 
> I also had a question about the behavior of parentRenderer(), which I think is probably NOT a problem, but I'd feel better if you confirmed it.
> 
> > Source/WebCore/ChangeLog:10
> > +        so there's no reason to check for it again.
> 
> Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null?  From a brief look it seems like the answer is no.  But if I'm wrong, this change would modify the behavior.  Can you please confirm that this is not a problem?

I don't understand what you're asking. parentRenderer never returns null in this code path because shouldCreateRenderer would have returned before we got here. There's no behavior change.

[Elliott Sprehn]

Created attachment 175138 [details]
Patch for landing

[Elliott Sprehn]

(In reply to comment #7)
> Created an attachment (id=175138) [details]
> Patch for landing

I moved the assert bove the if/else.

[Brent Fulgham]

(In reply to comment #6)
> (In reply to comment #4)
> > Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null?  From a brief look it seems like the answer is no.  But if I'm wrong, this change would modify the behavior.  Can you please confirm that this is not a problem?
> 
> I don't understand what you're asking. parentRenderer never returns null in this code path because shouldCreateRenderer would have returned before we got here. There's no behavior change.

I actually went and looked at the routine, and see that you are correct.  Thanks!

[WebKit Review Bot]

Comment on attachment 175138 [details]
Patch for landing

Clearing flags on attachment: 175138

Committed r135252: <http://trac.webkit.org/changeset/135252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.