Bug 87418 – WebBackForwardList should separate "has no current index" from the integer value of the current index

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 143934 [details]
Current WIP

Not ready for review.  Just attached for posterity.

[Brady Eidson]

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

[Brady Eidson]

Created attachment 144103 [details]
Patch v1

[Darin Adler]

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.

[Brady Eidson]

(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

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/118560

The real fireworks are on their way.

[Eric Seidel (no email)]

Attachment 144103 [details] was posted by a committer and has review+, assigning to Brady Eidson for commit.

[Ahmad Saleem]

https://github.com/WebKit/WebKit/commit/78f980d4eb70fdca0a2d1800d40806c0226a53e5 <- Landed and didn't backed out.

[Radar WebKit Bug Importer]

<rdar://problem/100952117>