Bug 87418

Summary: WebBackForwardList should separate "has no current index" from the integer value of the current index
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: ahmad.saleem792, darin, eric, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Current WIP
none
Patch v1 darin: review+

Description Brady Eidson 2012-05-24 13:02:08 PDT
WebBackForwardList should separate "has no current index" from the integer value of the current index

While exploring https://bugs.webkit.org/show_bug.cgi?id=87417 we noticed much of the code is implicit and subtle, and much of that confusion is centered around using UINT_MAX to mark "No current item index"

Moving that out in to a separate bool will help clean things up a lot.
Comment 1 Brady Eidson 2012-05-24 17:36:38 PDT
Created attachment 143934 [details]
Current WIP

Not ready for review.  Just attached for posterity.
Comment 2 Brady Eidson 2012-05-25 11:01:35 PDT
That WIP went above and beyond what I suggested we should do with this bug.

I'll attach a more limited patch that just transitions to using the bool flag and will do a more comprehensive cleanup of the mechanism in https://bugs.webkit.org/show_bug.cgi?id=87513
Comment 3 Brady Eidson 2012-05-25 11:10:22 PDT
Created attachment 144103 [details]
Patch v1
Comment 4 Darin Adler 2012-05-25 13:16:26 PDT
Comment on attachment 144103 [details]
Patch v1

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

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:139
> +    if (m_hasCurrentIndex)
> +        return m_entries[m_currentIndex].get();
>      return 0;

I think this would read better with a ? : than with an if statement. Or the early return should be the "return 0" if you want to stick with an if.

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:148
> +    if (m_currentIndex && m_hasCurrentIndex)
> +        return m_entries[m_currentIndex - 1].get();
>      return 0;

Same comment as above. I think the early return should be the error case or we could use a ? : expression.

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:157
> +    if (m_entries.size() && m_hasCurrentIndex && m_currentIndex < m_entries.size() - 1)
> +        return m_entries[m_currentIndex + 1].get();
>      return 0;

And again.

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:260
> +        m_currentIndex = 0;

No need for this line of code:

1) The code above already sets m_currentIndex to 0.
2) If we set m_hasCurrentIndex to false, it doesn’t matter what m_currentIndex is set to.

> Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:141
> +    uint32_t currentIndex = currentCFIndex == -1 ? 0 : static_cast<unsigned>(currentCFIndex);

Mix of unsigned and uint32_t on this line.

> Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:142
> +    bool hasCurrentIndex = currentCFIndex > -1;

We should do this first, and then we could use it when initializing currentIndex.

> Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:190
>      // Perform a sanity check: in case we're out of range, we reset.
> -    if (m_current != NoCurrentItemIndex && m_current >= newEntries.size())
> -        m_current = NoCurrentItemIndex;
> +    if (m_hasCurrentIndex && m_currentIndex >= newEntries.size())
> +        m_hasCurrentIndex = false;

We don’t need this sanity check because it’s redundant with checks done above. I thought you were going to remove it and replace it with an assertion in this patch.
Comment 5 Brady Eidson 2012-05-25 13:38:07 PDT
(In reply to comment #4)
> > Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:190
> >      // Perform a sanity check: in case we're out of range, we reset.
> > -    if (m_current != NoCurrentItemIndex && m_current >= newEntries.size())
> > -        m_current = NoCurrentItemIndex;
> > +    if (m_hasCurrentIndex && m_currentIndex >= newEntries.size())
> > +        m_hasCurrentIndex = false;
> 
> We don’t need this sanity check because it’s redundant with checks done above. I thought you were going to remove it and replace it with an assertion in this patch.

I have a *much* more thorough patch than this one I plan to do in https://bugs.webkit.org/show_bug.cgi?id=87513

Adding the bool and doing the m_current rename seemed like a separate enough task to split it off separately.

That said, I'll make many of the tweaks you suggested here before landing, and make sure everything is addressed in bug 87513
Comment 6 Brady Eidson 2012-05-25 14:10:15 PDT
Landed in http://trac.webkit.org/changeset/118560

The real fireworks are on their way.
Comment 7 Eric Seidel (no email) 2013-01-04 00:53:52 PST
Attachment 144103 [details] was posted by a committer and has review+, assigning to Brady Eidson for commit.
Comment 8 Ahmad Saleem 2022-10-09 13:40:06 PDT
https://github.com/WebKit/WebKit/commit/78f980d4eb70fdca0a2d1800d40806c0226a53e5 <- Landed and didn't backed out.
Comment 9 Radar WebKit Bug Importer 2022-10-09 13:41:19 PDT
<rdar://problem/100952117>