Clean up the page state saving mechanism.

Clean up the page state saving mechanism.

BUG=416184
TEST=as in bug

Committed: https://crrev.com/133fac58dfe9ac1d2b0071ce107825adaad67cc7
Cr-Commit-Position: refs/heads/master@{#302372}

[Avi (use Gerrit), 2014-10-31 19:25:41]

avi@chromium.org changed reviewers:
+ creis@chromium.org

[Avi (use Gerrit), 2014-10-31 19:25:41]

I've been sitting on this one for a while; this was intended to fix the history
smashing back when I thought it might be the fault of the page state system.

This fixes a lot of naming issues (three different names for the same thing!)
and makes it a straight-up "dirty" thing.

[Avi (use Gerrit), 2014-10-31 19:59:30]

On 2014/10/31 19:25:41, Avi wrote:
> I've been sitting on this one for a while; this was intended to fix the
history
> smashing back when I thought it might be the fault of the page state system.
> 
> This fixes a lot of naming issues (three different names for the same thing!)
> and makes it a straight-up "dirty" thing.

Looks like a test went red, though on only one bot so far. I'd be interested in
your thoughts, if not a complete review.

[Charlie Reis, 2014-10-31 20:45:10]

On Fri, Oct 31, 2014 at 12:59 PM, <avi@chromium.org> wrote:

> On 2014/10/31 19:25:41, Avi wrote:
>
>> I've been sitting on this one for a while; this was intended to fix the
>>
> history
>
>> smashing back when I thought it might be the fault of the page state
>> system.
>>
>
>  This fixes a lot of naming issues (three different names for the same
>> thing!)
>> and makes it a straight-up "dirty" thing.
>>
>
> Looks like a test went red, though on only one bot so far. I'd be
> interested in
> your thoughts, if not a complete review.
>

I think that might be https://crbug.com/429190, hence unrelated.  I'm
looking at the rest now.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2014-10-31 21:38:49]

Wow, tricky to review!  But only because of the mess that was there before-- so
many entry points to SendUpdateState...  I think it made more sense originally
when they passed in different entries from different call sites.

Anyway, much cleaner now.  The dirty bit is a slight behavior change but
shouldn't have an observable difference.  LGTM with a few nits.

https://codereview.chromium.org/689163002/diff/1/content/public/renderer/rend...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/689163002/diff/1/content/public/renderer/rend...
content/public/renderer/render_view.h:99: virtual bool
GetPageStateSentImmediately() const = 0;
nit: IsPageStateSentImmediately()

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:1909: if (!page_state_dirty_ || page_id_ ==
-1 || !webview())
Just want to verify that I understand the implications of page_state_dirty_.  I
agree that it makes sense to avoid sending the IPC if we don't need to, and that
we'll set it to true when state changes happen.

It looks like we currently don't send an initial UpdateState right after commit,
so there's no change in that case.

We do currently send UpdateState for pages that don't need it right before we
leave (either in DidCommit or OnSwapOut), but I can't imagine that's required. 
If there's any downside to this change, it's that it might make our browser-side
UpdateState DCHECK harder to reproduce, since it'll only happen if scrolling or
other state changes are occurring.  But that's not a good reason to skip the
dirty bit.  So seems ok to me.

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.h:780: // browser. (It will be send on a
delay, but that doesn't concern callers.)
nit: send -> sent

[Avi (use Gerrit), 2014-10-31 22:11:12]

avi@chromium.org changed reviewers:
+ sky@chromium.org

[Avi (use Gerrit), 2014-10-31 22:11:13]

Scott, can you look at chrome/renderer/chrome_render_view_observer.cc ? Just a
rename of something.

https://codereview.chromium.org/689163002/diff/1/content/public/renderer/rend...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/689163002/diff/1/content/public/renderer/rend...
content/public/renderer/render_view.h:99: virtual bool
GetPageStateSentImmediately() const = 0;
On 2014/10/31 21:38:49, Charlie Reis wrote:
> nit: IsPageStateSentImmediately()

Done.

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:1909: if (!page_state_dirty_ || page_id_ ==
-1 || !webview())
On 2014/10/31 21:38:49, Charlie Reis wrote:
> Just want to verify that I understand the implications of page_state_dirty_. 
I
> agree that it makes sense to avoid sending the IPC if we don't need to, and
that
> we'll set it to true when state changes happen.
> 
> It looks like we currently don't send an initial UpdateState right after
commit,
> so there's no change in that case.
> 
> We do currently send UpdateState for pages that don't need it right before we
> leave (either in DidCommit or OnSwapOut), but I can't imagine that's required.

> If there's any downside to this change, it's that it might make our
browser-side
> UpdateState DCHECK harder to reproduce, since it'll only happen if scrolling
or
> other state changes are occurring.  But that's not a good reason to skip the
> dirty bit.  So seems ok to me.

Exactly. This is an intentional change, where we don't send updates if there
wasn't a reason to send one.

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/689163002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.h:780: // browser. (It will be send on a
delay, but that doesn't concern callers.)
On 2014/10/31 21:38:49, Charlie Reis wrote:
> nit: send -> sent

Done.

[sky, 2014-10-31 22:37:02]

LGTM

[Avi (use Gerrit), 2014-11-01 00:21:16]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-11-01 00:22:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/689163002/20001

[commit-bot: I haz the power, 2014-11-01 01:04:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-01 01:04:49]

Patchset 2 (id:??) landed as
https://crrev.com/133fac58dfe9ac1d2b0071ce107825adaad67cc7
Cr-Commit-Position: refs/heads/master@{#302372}

[Avi (use Gerrit), 2014-11-02 18:12:26]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/694963003/ by avi@chromium.org.

The reason for reverting is: This is a likely candidate for having broken lots
of layout tests....

[Avi (use Gerrit), 2014-11-02 18:15:40]

On 2014/11/02 18:12:26, Avi wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/694963003/ by mailto:avi@chromium.org.
> 
> The reason for reverting is: This is a likely candidate for having broken lots
> of layout tests....

vsevik wrote:

I think https://codereview.chromium.org/689163002 might have broken a number of
layout tests related to history and nvigation:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...