Bug 135011

Summary: CSS JIT: Clean up return path
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: CSSAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 134835    
Attachments:
Description Flags
Patch benjamin: review+, benjamin: commit-queue-

Description Yusuke Suzuki 2014-07-16 23:32:42 PDT
Before adding early return path for PseudoElement, we need to simplify the existing return path in CSS JIT.
Comment 1 Yusuke Suzuki 2014-07-16 23:33:54 PDT
Created attachment 235054 [details]
Patch
Comment 2 Benjamin Poulain 2014-07-16 23:49:12 PDT
Comment on attachment 235054 [details]
Patch

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

This looks correct to me.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1193
> +    StackAllocator::StackReference baselineStackReference;

I think "baseline" is a little too generic for the name. One could think it is the top of the stack for this function.

What about startOfDiscardableStack, temporaryStackBase, ... or something like that?

> Source/WebCore/cssjit/StackAllocator.h:156
> +    void popAndDiscardUpToBaseline()
> +    {
> +        if (!m_offsetFromTop)
> +            return;
> +        m_assembler.addPtr(JSC::MacroAssembler::TrustedImm32(m_offsetFromTop), JSC::MacroAssembler::stackPointerRegister);
> +        m_offsetFromTop = 0;
> +    }
> +

This is unused.
Comment 3 Yusuke Suzuki 2014-07-16 23:58:22 PDT
Comment on attachment 235054 [details]
Patch

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

Thank you for your comments. They are reasonable. I'll reflect your comments and land it :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1193
>> +    StackAllocator::StackReference baselineStackReference;
> 
> I think "baseline" is a little too generic for the name. One could think it is the top of the stack for this function.
> 
> What about startOfDiscardableStack, temporaryStackBase, ... or something like that?

Right. `temporaryStackBase` looks suitable for me :) I'll change it.

>> Source/WebCore/cssjit/StackAllocator.h:156
>> +
> 
> This is unused.

Oops! Thank you, removing.
Comment 4 Yusuke Suzuki 2014-07-17 00:58:55 PDT
Committed r171177: <http://trac.webkit.org/changeset/171177>