Bug 161865 – Switch remaining users of Document::inPageCache() to pageCacheState()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Switch remaining users of Document::inPageCache() to pageCacheState() as the former one is confusing (given that it returns true while the pagehide event is being fired).

[Chris Dumez]

Created attachment 288576 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 288576 [details]
Patch

Clearing flags on attachment: 288576

Committed r205818: <http://trac.webkit.org/changeset/205818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

I don't think 
  frameView.frame().document()->pageCacheState() != Document::NotInPageCache
is clearer than:
  frameView.frame().document()->inPageCache()
particularly because of the double negative. Can you add back inPageCache() as a helper?

[Chris Dumez]

(In reply to comment #4)
> I don't think 
>   frameView.frame().document()->pageCacheState() != Document::NotInPageCache
> is clearer than:
>   frameView.frame().document()->inPageCache()
> particularly because of the double negative. Can you add back inPageCache()
> as a helper?

Well, clearly it was not clear because it was not used properly and introduced security bugs.

You'd like inPageCache() as a helper for what?
frameView.frame().document()->pageCacheState() != Document::NotInPageCache means it could be either:
- about to enter page cache (a)
- in page cache (b)

previously, inPageCache() was a helper for returning true for both (a) and (b). We could re-introduce it for (b) only, which would be equivalent to:
frameView.frame().document()->pageCacheState() == Document::InPageCache

not
frameView.frame().document()->pageCacheState() != Document::NotInPageCache

[Darin Adler]

I understand the need to clearly distinguish the three different cases and not have this be a simple "in" vs. "out" of page cache. But I also agree that boolean expressions with pageCacheState are not easy to read; not really clear unless you know what the three states are, and even then pretty wording.

Maybe we can name two or three boolean-returning functions with clear unambiguous names, even if they are longer names, and use them. I think that would be better than what we currently have.

[Chris Dumez]

(In reply to comment #6)
> I understand the need to clearly distinguish the three different cases and
> not have this be a simple "in" vs. "out" of page cache. But I also agree
> that boolean expressions with pageCacheState are not easy to read; not
> really clear unless you know what the three states are, and even then pretty
> wording.
> 
> Maybe we can name two or three boolean-returning functions with clear
> unambiguous names, even if they are longer names, and use them. I think that
> would be better than what we currently have.

Yes, I think this would be nice.

It looks like the checks we're using currently are:
- pageCacheState() != NotInPageCache
-> could become aboutToEnterOrInPageCache()

- pageCacheState() == NotInPageCache
-> could become !aboutToEnterOrInPageCache()

- pageCacheState() == Document::InPageCache
-> could become enteredPageCache() or inPageCache()

- pageCacheState() == Document::AboutToEnterPageCache
-> could become aboutToEnterPageCache()

Let me know what you think.