Add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState restore from default

Add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState restore
from default

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment| in |checkCompleted| and restore from 
default. 

So we add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState
restore from default.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2628683003
Cr-Commit-Position: refs/heads/master@{#451111}
Committed: https://chromium.googlesource.com/chromium/src/+/1033be6d096d430b3d1fe6439550448b2374ea4c

[chaopeng, 2017-01-11 19:28:32]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 19:29:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 19:44:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:44:52]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-01-11 20:03:01]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 20:03:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 20:21:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 20:21:22]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chaopeng, 2017-01-11 21:40:48]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 21:41:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 23:14:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 23:14:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chaopeng, 2017-01-12 18:40:43]

Description was changed from

==========
Ensure processFragment called before restoreScrollPositionAndViewState

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment|. |restoreScrollPositionAndViewState| is
called in |checkCompleted| so we move processFragment above it.


BUG=672545
==========

to

==========
Ensure processFragment called before restoreScrollPositionAndViewState

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment|. |restoreScrollPositionAndViewState| is
called in |checkCompleted| so we add processFragment above it.


BUG=672545
==========

[chaopeng, 2017-01-12 19:55:38]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[chaopeng, 2017-01-12 19:56:02]

chaopeng@chromium.org changed reviewers:
+ bokan@chromium.org

[chaopeng, 2017-01-12 19:56:02]

PTAL. Thank you.

https://codereview.chromium.org/2628683003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2628683003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:648:
processFragment(m_frame->document()->url(), NavigationToDifferentDocument);
We need to keep the processFragment here (after the
m_frame->document()->implicitClose() in checkCompleted) otherwise SVG may render
to wrong position.

[commit-bot: I haz the power, 2017-01-12 19:56:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 21:33:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 21:33:26]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-01-13 16:10:03]

bokan@chromium.org changed reviewers:
+ japhet@chromium.org, majidvp@chromium.org

[bokan, 2017-01-13 16:10:03]

This looks fine to me, but I'm wondering, could we just put processFragment and
RestoreScrollPositionAndViewState together in one "setScrollFromNavigation"
method. It would always first process the fragment, then restore the history
scroll. We'd use this combine method everywhere
RestoreScrollPositionAndViewState is used today.

majidvp@, japhet@, thoughts? The issue here is that restoring history causes us
to ignore the fragment but the fragment may have been used to scroll a
subscroller into view so we really want to restore the fragment, then restore
history.

[Nate Chapin, 2017-01-18 21:08:44]

On 2017/01/13 16:10:03, bokan wrote:
> This looks fine to me, but I'm wondering, could we just put processFragment
and
> RestoreScrollPositionAndViewState together in one "setScrollFromNavigation"
> method. It would always first process the fragment, then restore the history
> scroll. We'd use this combine method everywhere
> RestoreScrollPositionAndViewState is used today.
> 
> majidvp@, japhet@, thoughts? The issue here is that restoring history causes
us
> to ignore the fragment but the fragment may have been used to scroll a
> subscroller into view so we really want to restore the fragment, then restore
> history.

That sounds promising. I like that possibility better than spreading
processFragment() calls around.

[chaopeng, 2017-01-19 17:04:56]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 17:05:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 18:47:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 18:47:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chaopeng, 2017-01-19 19:28:33]

bokan@ ptal. The way we "process fragment" is different in FrameView and
FrameLoader so I introduce different setScrollFromNavigation in FrameView and
FrameLoader.

[Nate Chapin, 2017-01-19 19:36:33]

https://codereview.chromium.org/2628683003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2628683003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1379: void
FrameLoader::setScrollFromNavigation(const KURL* url,
m_frame->document()->url() should be the correct url at this point, even for
same document navigations (the document url gets updated in
updateForSameDocumentNavigation). Alos, we don't usually pass around pointers to
KURLs.

[chaopeng, 2017-01-23 14:39:21]

Nate's Comment addressed. PTAL. Thank you.

[bokan, 2017-01-23 15:18:47]

looks fine to me but I'll defer to japhet@ for the final stamp.

[majidvp, 2017-01-23 15:38:22]

I think it is problematic that you are replacing all calls to
|restoreScrollPositionAndViewState();|
with |setScrollFromNavigation| indiscriminately. This means that in more than a
few cases we are now
first scrolling to a fragment then reverting all that by then scrolling to a
saved position.
Previously we would have done that in a lot fewer cases. 


Also, I think this patch may actually be against the current spec:
In particular see  item 8 [1] in section:
https://html.spec.whatwg.org/multipage/browsers.html#history-traversal
The spec gives precedent to UA history navigation over fragment scrolling when
doing back-forward navigation.
In particular, when the page has "history.scrollRestoration = manual" then we
shouldn't restore scroll position
or scroll to fragment but with your patch we are going to do that.

Here is the test verifying that behavior [2, 3]. Does you patch pass these
tests?

[1] "If entry is not an entry with persisted user state, but its URL's fragment
is non-null, then scroll to the fragment."
[2]
https://github.com/w3c/web-platform-tests/blob/master/html/browsers/browsing-...
[3]
https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/f...

[chaopeng, 2017-01-23 16:14:21]

On 2017/01/23 15:38:22, majidvp wrote:
> I think it is problematic that you are replacing all calls to
> |restoreScrollPositionAndViewState();|
> with |setScrollFromNavigation| indiscriminately. This means that in more than
a
> few cases we are now
> first scrolling to a fragment then reverting all that by then scrolling to a
> saved position.
> Previously we would have done that in a lot fewer cases. 
> 
> 
> Also, I think this patch may actually be against the current spec:
> In particular see  item 8 [1] in section:
> https://html.spec.whatwg.org/multipage/browsers.html#history-traversal
> The spec gives precedent to UA history navigation over fragment scrolling when
> doing back-forward navigation.
> In particular, when the page has "history.scrollRestoration = manual" then we
> shouldn't restore scroll position
> or scroll to fragment but with your patch we are going to do that.
> 
> Here is the test verifying that behavior [2, 3]. Does you patch pass these
> tests?
> 
> [1] "If entry is not an entry with persisted user state, but its URL's
fragment
> is non-null, then scroll to the fragment."
> [2]
>
https://github.com/w3c/web-platform-tests/blob/master/html/browsers/browsing-...
> [3]
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/LayoutTests/f...

Yes, I verified we can pass these tests with patch. We have the
"history.scrollRestoration = manual" check in processFragment
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/l...

[majidvp, 2017-01-24 21:53:52]

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:276: void
FrameView::setScrollFromNavigation() {
nit: please match the declaration order in header file.

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:277:
scrollToFragmentAnchor();
In this case, 
scrollToFragment does not honor history.scrollRestoration

Can it cause an issue?

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:1017: void
setScrollFromNavigation();
In the frameview case, the scroll position is 
reset after size changes (e.g., content size or frame size).

This is not necessarily happening after navigation 
so the function name seem a bit inappropriate. Maybe
setScrollAfter[Scroll?]SizeChange ?

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.h:206: void
setScrollFromNavigation(LoadStartType = NavigationToDifferentDocument);
The default argument here does not see worth the readability cost.
I suggest not having a default arg.

I think the Google C++ guideline suggests using default args when
the case for not using default is rare. But here the call site
are so few which suggests it is not worth it to me.

https://google.github.io/styleguide/cppguide.html#Default_Arguments

[chaopeng, 2017-01-25 15:22:38]

Updated, PTAL. Thank you.

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:277:
scrollToFragmentAnchor();
On 2017/01/24 21:53:52, majidvp wrote:
> In this case, 
> scrollToFragment does not honor history.scrollRestoration
> 
> Can it cause an issue?

No, in this case m_fragmentAnchor is null and scrollToFragment will do nothing
because we dont set m_fragmentAnchor in this case. 
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...

[majidvp, 2017-01-25 20:31:59]

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2628683003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:277:
scrollToFragmentAnchor();
On 2017/01/25 15:22:38, chaopeng wrote:
> On 2017/01/24 21:53:52, majidvp wrote:
> > In this case, 
> > scrollToFragment does not honor history.scrollRestoration
> > 
> > Can it cause an issue?
> 
> No, in this case m_fragmentAnchor is null and scrollToFragment will do nothing
> because we dont set m_fragmentAnchor in this case. 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...

Are you sure? If the fragmentAnchor is always nil in such cases then why do we
even bother to call scrollToFragmentAnchor?

https://codereview.chromium.org/2628683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2628683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:746:
setScrollFromNavigation(NavigationToDifferentDocument);
Are you sure this is the correct navigation type?
checkCompleted is also invoked from |FrameLoader::loadInSameDocument| which
suggests otherwise.

https://codereview.chromium.org/2628683003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/ProgrammaticScrollTest.cpp (right):

https://codereview.chromium.org/2628683003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ProgrammaticScrollTest.cpp:93:
loader.setScrollFromNavigation();
Are you sure this compiles? One argument is missing here.

[chaopeng, 2017-01-26 18:46:09]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 18:46:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 20:24:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 20:24:26]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-01-26 21:17:05]

Updated, PTAL. Thank you.

[Nate Chapin, 2017-01-26 21:25:36]

https://codereview.chromium.org/2628683003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2628683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:883: checkCompleted();
Context: I suggested removing this, because updateForSameDocumentNavigation()
will handle pulsing start/stop notifications if Frame::isLoading() is false, and
all of the logic in checkCompleted() really ought to be specific to
different-document navigations.

[majidvp, 2017-01-26 21:49:50]

https://codereview.chromium.org/2628683003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2628683003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:883: checkCompleted();
On 2017/01/26 21:25:35, Nate Chapin wrote:
> Context: I suggested removing this, because updateForSameDocumentNavigation()
> will handle pulsing start/stop notifications if Frame::isLoading() is false,
and
> all of the logic in checkCompleted() really ought to be specific to
> different-document navigations.

Interesting. That works. Should we add a DCHECK in checkCompleted() to
document this assumption?

[Nate Chapin, 2017-01-26 21:58:40]

On 2017/01/26 21:49:50, majidvp wrote:
>
https://codereview.chromium.org/2628683003/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):
> 
>
https://codereview.chromium.org/2628683003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/FrameLoader.cpp:883: checkCompleted();
> On 2017/01/26 21:25:35, Nate Chapin wrote:
> > Context: I suggested removing this, because
updateForSameDocumentNavigation()
> > will handle pulsing start/stop notifications if Frame::isLoading() is false,
> and
> > all of the logic in checkCompleted() really ought to be specific to
> > different-document navigations.
> 
> Interesting. That works. Should we add a DCHECK in checkCompleted() to
> document this assumption?

DCHECKing would require some stack state to indicate that we're inside a
same-document navigation, wouldn't it? I'd rather not add such state if we can
avoid it.

[majidvp, 2017-01-30 16:46:53]

The patch lgtm. 

I was wondering if there is a need to have additional tests that covers this
particular 
ordering between fragment scrolling and saves scroll position restoration.
Here are the major cases we should cover and looking around I found the current
coverage to be:
 1. Only fragment scrolls: 
external/wpt/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html,
http/tests/navigation/anchor-frames-*.html
 2. Only saved scroll position:
fast/loader/scroll-position-restoration-for-history-api.html,
fast/loader/scroll-position-restored-on-back-at-load-event.html,
fast/history/scroll-restoration/scroll-restoration-*.html
 3. Fragment and scroll position together
     a. with 'history.scrollRestoraion:manual':
fast/history/scroll-restoration/scroll-restoration-fragment-navigation-*.html
(cross-doc and same-doc)
     b. with 'history.scrollRestoration:auto':
fast/loader/scroll-restore-overrides-fragment.html (only cross-doc)

Perhaps that is enough coverage? 

I wish there were all in the same place rather than scattered all over. Maybe I
will just do that :)/
Also noticed the at least one test in the imported wpt tests for
external/wpt/html/browsers/browsing-the-web/scroll-to-fragid/
is failing. Didn't look that important but it is nice to fix to improve interop.

[chaopeng, 2017-01-30 17:11:47]

On 2017/01/30 16:46:53, majidvp wrote:
> The patch lgtm. 
> 
> I was wondering if there is a need to have additional tests that covers this
> particular 
> ordering between fragment scrolling and saves scroll position restoration.
> Here are the major cases we should cover and looking around I found the
current
> coverage to be:
>  1. Only fragment scrolls: 
>
external/wpt/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html,
> http/tests/navigation/anchor-frames-*.html
>  2. Only saved scroll position:
> fast/loader/scroll-position-restoration-for-history-api.html,
> fast/loader/scroll-position-restored-on-back-at-load-event.html,
> fast/history/scroll-restoration/scroll-restoration-*.html
>  3. Fragment and scroll position together
>      a. with 'history.scrollRestoraion:manual':
> fast/history/scroll-restoration/scroll-restoration-fragment-navigation-*.html
> (cross-doc and same-doc)
>      b. with 'history.scrollRestoration:auto':
> fast/loader/scroll-restore-overrides-fragment.html (only cross-doc)
> 
> Perhaps that is enough coverage? 
> 
> I wish there were all in the same place rather than scattered all over. Maybe
I
> will just do that :)/
> Also noticed the at least one test in the imported wpt tests for
> external/wpt/html/browsers/browsing-the-web/scroll-to-fragid/
> is failing. Didn't look that important but it is nice to fix to improve
interop.

This patch is for fixing ReloadUrlWithAnchor test flaky(80%) in trybot. I think
this is a cover for this particular order issue.

[Nate Chapin, 2017-02-02 17:18:29]

On 2017/01/30 17:11:47, chaopeng wrote:
> On 2017/01/30 16:46:53, majidvp wrote:
> > The patch lgtm. 
> > 
> > I was wondering if there is a need to have additional tests that covers this
> > particular 
> > ordering between fragment scrolling and saves scroll position restoration.
> > Here are the major cases we should cover and looking around I found the
> current
> > coverage to be:
> >  1. Only fragment scrolls: 
> >
>
external/wpt/html/browsers/browsing-the-web/scroll-to-fragid/scroll-to-anchor-name.html,
> > http/tests/navigation/anchor-frames-*.html
> >  2. Only saved scroll position:
> > fast/loader/scroll-position-restoration-for-history-api.html,
> > fast/loader/scroll-position-restored-on-back-at-load-event.html,
> > fast/history/scroll-restoration/scroll-restoration-*.html
> >  3. Fragment and scroll position together
> >      a. with 'history.scrollRestoraion:manual':
> >
fast/history/scroll-restoration/scroll-restoration-fragment-navigation-*.html
> > (cross-doc and same-doc)
> >      b. with 'history.scrollRestoration:auto':
> > fast/loader/scroll-restore-overrides-fragment.html (only cross-doc)
> > 
> > Perhaps that is enough coverage? 
> > 
> > I wish there were all in the same place rather than scattered all over.
Maybe
> I
> > will just do that :)/
> > Also noticed the at least one test in the imported wpt tests for
> > external/wpt/html/browsers/browsing-the-web/scroll-to-fragid/
> > is failing. Didn't look that important but it is nice to fix to improve
> interop.
> 
> This patch is for fixing ReloadUrlWithAnchor test flaky(80%) in trybot. I
think
> this is a cover for this particular order issue.

LGTM if majidvp is happy with the state of testing.

[majidvp, 2017-02-02 19:23:19]

> LGTM if majidvp is happy with the state of testing.

Based on what I have seen and a chat with chaopeng current 
test coverage seems sufficient.

[chaopeng, 2017-02-02 19:24:10]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-02-02 19:24:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 19:26:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 19:26:36]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chaopeng, 2017-02-02 19:53:35]

Description was changed from

==========
Ensure processFragment called before restoreScrollPositionAndViewState

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment|. |restoreScrollPositionAndViewState| is
called in |checkCompleted| so we add processFragment above it.


BUG=672545
==========

to

==========
Ensure processFragment called before restoreScrollPositionAndViewState

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment|. |restoreScrollPositionAndViewState| is
called in |checkCompleted| so we add processFragment above it.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[chaopeng, 2017-02-02 19:56:08]

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

[chaopeng, 2017-02-02 19:56:09]

creis@, This is the patch for the flaky test
NavigationControllerBrowserTest.ReloadWithUrlAnchor. PTAL. Thank you.

[chaopeng, 2017-02-02 19:58:28]

Description was changed from

==========
Ensure processFragment called before restoreScrollPositionAndViewState

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment|. |restoreScrollPositionAndViewState| is
called in |checkCompleted| so we add processFragment above it.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure processFragment called before restoreScrollPositionAndViewState

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment|. |restoreScrollPositionAndViewState| is
called in |checkCompleted| so we add processFragment above it.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[chaopeng, 2017-02-02 19:58:28]

chaopeng@chromium.org changed reviewers:
- creis@chromium.org

[chaopeng, 2017-02-02 20:00:51]

chaopeng@chromium.org changed reviewers:
+ alexmos@chromium.org

[chaopeng, 2017-02-02 20:00:52]

alexmos@chromium.org: PTAL, Thank you.

[alexmos, 2017-02-02 22:06:31]

LGTM

[chaopeng, 2017-02-02 22:17:37]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-02-02 22:17:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2628683003/#ps220001
(title: "format")

[commit-bot: I haz the power, 2017-02-02 22:18:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 23:54:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 23:54:40]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chaopeng, 2017-02-08 21:23:24]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[chaopeng, 2017-02-08 21:24:00]

Nate, PTAL.

[commit-bot: I haz the power, 2017-02-08 21:24:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 00:05:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 00:05:16]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-02-13 18:19:48]

Description was changed from

==========
Ensure processFragment called before restoreScrollPositionAndViewState

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment|. |restoreScrollPositionAndViewState| is
called in |checkCompleted| so we add processFragment above it.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add DidSaveScrollState flag to prevent restoreScrollPositionAndViewState restore
from default

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment| in |checkCompleted| and restore from 
default. 

So we add DidSaveScrollState flag to prevent restoreScrollPositionAndViewState
restore from default.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[chaopeng, 2017-02-13 18:21:13]

Change fix to " Add DidSaveScrollState flag to prevent
restoreScrollPositionAndViewState restore from default".

majidvp@ japhet@ PTAL.

[majidvp, 2017-02-14 20:42:17]

https://codereview.chromium.org/2628683003/diff/280001/content/common/page_st...
File content/common/page_state_serialization.cc (right):

https://codereview.chromium.org/2628683003/diff/280001/content/common/page_st...
content/common/page_state_serialization.cc:510:
WriteInteger(state.scroll_offset.y(), obj);
Is did_save_scroll_state is false, then why are we writing scroll_offset and
visual_viewport_scroll_offset? Presumably we can skip writing/reading.  (Maybe
even scale factor?)

https://codereview.chromium.org/2628683003/diff/280001/content/common/page_st...
content/common/page_state_serialization.cc:518:
WriteBoolean(state.did_save_scroll_state, obj);
Why here? I feel if anything, it should be written just before the scroll
position.

https://codereview.chromium.org/2628683003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2628683003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:287:
m_currentItem->setDidSaveScrollState(true);
should we set this flag inside HistoryItem::setScrollOffset and
HistoryItem::setVisualViewportScrollOffset instead of here. That way we never
risk setting the value but not the flag.

https://codereview.chromium.org/2628683003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:425:
m_currentItem->setDidSaveScrollState(oldItem->didSaveScrollState());
Perhaps copy over the scroll state only if the didSaveScrollState is true?

https://codereview.chromium.org/2628683003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2628683003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/HistoryItem.h:151: bool
m_didSaveScrollState;
nit: maybe move this next to scroll state related fields.

[majidvp, 2017-02-14 21:00:58]

https://codereview.chromium.org/2628683003/diff/280001/content/common/page_st...
File content/common/page_state_serialization.cc (right):

https://codereview.chromium.org/2628683003/diff/280001/content/common/page_st...
content/common/page_state_serialization.cc:700: did_save_scroll_state(false),
I think this is the incorrect initial value. In particular
this is used when we have not persisted the field (i.e. version < 24).
In such cases, I believe the legacy behaviour is to always assume that
there is a saved scroll state saved. Which suggests this should be 'true'.

[chaopeng, 2017-02-15 01:24:48]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 01:26:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chaopeng, 2017-02-15 01:31:15]

Patchset #16 (id:300001) has been deleted

[chaopeng, 2017-02-15 01:54:12]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[chaopeng, 2017-02-15 01:54:40]

Updated, PTAL.

[commit-bot: I haz the power, 2017-02-15 01:55:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 02:55:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 02:55:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chaopeng, 2017-02-15 03:08:05]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 03:09:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 05:23:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 05:23:38]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-02-15 16:37:23]

Description was changed from

==========
Add DidSaveScrollState flag to prevent restoreScrollPositionAndViewState restore
from default

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment| in |checkCompleted| and restore from 
default. 

So we add DidSaveScrollState flag to prevent restoreScrollPositionAndViewState
restore from default.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState
restore
from default

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment| in |checkCompleted| and restore from 
default. 

So we add DidSaveScrollOrScaleState flag to prevent
restoreScrollPositionAndViewState
restore from default.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[majidvp, 2017-02-15 17:37:32]

This look good if japhet@ is OK with the idea.

I am not an expert on the serialization format. I suggested to put the new
field earlier and use its value to skip write/read the others dependent field
(i.e., scroll state). But there is  a comment that suggests adding new fields
at the end. I don't fully understand why so please go  with whatever approach
the OWNER's prefer.

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/HistoryItem.h:63: void
setDidSaveScrollOrScaleState(bool hasSaveScrollOrScaleState) {
nit: I think the bool name should match the setter/memeber name i.e.,
|didSaveScrollOrScaleState|

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/ProgrammaticScrollTest.cpp (right):

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ProgrammaticScrollTest.cpp:61:
loader.currentItem()->setDidSaveScrollOrScaleState(true);
I don't think this is needed!

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ProgrammaticScrollTest.cpp:92:
loader.currentItem()->setDidSaveScrollOrScaleState(true);
ditto.

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right):

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:930:
item.setDidSaveScrollOrScaleState(true);
and here.

https://codereview.chromium.org/2628683003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:963:
item.setDidSaveScrollOrScaleState(true);
and here.

[Nate Chapin, 2017-02-15 17:53:34]

LGTM once majidvp's last round of comments are addressed.

[chaopeng, 2017-02-15 18:12:27]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-02-15 18:12:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, alexmos@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2628683003/#ps360001
(title: "majid comment addressed")

[commit-bot: I haz the power, 2017-02-15 18:13:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2017-02-15 18:38:26]

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

[Charlie Reis, 2017-02-15 18:38:27]

japhet@: Shouldn't we be adding a PageStateSerialization BackwardsCompat test
for this (any time a new format is added)?  That would go in
page_state_serialization_unittest.cc.  I haven't made such changes myself, but
that's my understanding.

[chaopeng, 2017-02-15 18:39:22]

The CQ bit was unchecked by chaopeng@chromium.org

[chaopeng, 2017-02-15 18:41:17]

On 2017/02/15 18:38:27, Charlie Reis wrote:
> japhet@: Shouldn't we be adding a PageStateSerialization BackwardsCompat test
> for this (any time a new format is added)?  That would go in
> page_state_serialization_unittest.cc.  I haven't made such changes myself, but
> that's my understanding.

Should I add one like this:

TEST_F(PageStateSerializationTest, BackwardsCompat_v23) {
  TestBackwardsCompat(23);
}

[Charlie Reis, 2017-02-15 18:43:24]

On 2017/02/15 18:41:17, chaopeng wrote:
> On 2017/02/15 18:38:27, Charlie Reis wrote:
> > japhet@: Shouldn't we be adding a PageStateSerialization BackwardsCompat
test
> > for this (any time a new format is added)?  That would go in
> > page_state_serialization_unittest.cc.  I haven't made such changes myself,
but
> > that's my understanding.
> 
> Should I add one like this:
> 
> TEST_F(PageStateSerializationTest, BackwardsCompat_v23) {
>   TestBackwardsCompat(23);
> }

I think you have to create a dat file (see
https://codereview.chromium.org/1138543002 for a recent example), though I'm not
sure how.  Also, I'm not sure if yours should be 23 or 24-- did we miss one? 
Nate would know.

[majidvp, 2017-02-15 18:50:18]

Pretty sure we need such a test.

On Wed, Feb 15, 2017 at 1:43 PM <creis@chromium.org> wrote:

> On 2017/02/15 18:41:17, chaopeng wrote:
> > On 2017/02/15 18:38:27, Charlie Reis wrote:
> > > japhet@: Shouldn't we be adding a PageStateSerialization
> BackwardsCompat
> test
> > > for this (any time a new format is added)? That would go in
> > > page_state_serialization_unittest.cc. I haven't made such changes
> myself,
> but
> > > that's my understanding.
> >
> > Should I add one like this:
> >
> > TEST_F(PageStateSerializationTest, BackwardsCompat_v23) {
> > TestBackwardsCompat(23);
> > }
>
> I think you have to create a dat file (see
> https://codereview.chromium.org/1138543002 for a recent example), though
> I'm not
> sure how. Also, I'm not sure if yours should be 23 or 24-- did we miss
> one?
> Nate would know.
>
> https://codereview.chromium.org/2628683003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[majidvp, 2017-02-15 18:50:18]

Pretty sure we need such a test.

On Wed, Feb 15, 2017 at 1:43 PM <creis@chromium.org> wrote:

> On 2017/02/15 18:41:17, chaopeng wrote:
> > On 2017/02/15 18:38:27, Charlie Reis wrote:
> > > japhet@: Shouldn't we be adding a PageStateSerialization
> BackwardsCompat
> test
> > > for this (any time a new format is added)? That would go in
> > > page_state_serialization_unittest.cc. I haven't made such changes
> myself,
> but
> > > that's my understanding.
> >
> > Should I add one like this:
> >
> > TEST_F(PageStateSerializationTest, BackwardsCompat_v23) {
> > TestBackwardsCompat(23);
> > }
>
> I think you have to create a dat file (see
> https://codereview.chromium.org/1138543002 for a recent example), though
> I'm not
> sure how. Also, I'm not sure if yours should be 23 or 24-- did we miss
> one?
> Nate would know.
>
> https://codereview.chromium.org/2628683003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nate Chapin, 2017-02-15 19:05:02]

On 2017/02/15 18:50:18, majidvp wrote:
> Pretty sure we need such a test.
> 
> On Wed, Feb 15, 2017 at 1:43 PM <mailto:creis@chromium.org> wrote:
> 
> > On 2017/02/15 18:41:17, chaopeng wrote:
> > > On 2017/02/15 18:38:27, Charlie Reis wrote:
> > > > japhet@: Shouldn't we be adding a PageStateSerialization
> > BackwardsCompat
> > test
> > > > for this (any time a new format is added)? That would go in
> > > > page_state_serialization_unittest.cc. I haven't made such changes
> > myself,
> > but
> > > > that's my understanding.
> > >
> > > Should I add one like this:
> > >
> > > TEST_F(PageStateSerializationTest, BackwardsCompat_v23) {
> > > TestBackwardsCompat(23);
> > > }
> >
> > I think you have to create a dat file (see
> > https://codereview.chromium.org/1138543002 for a recent example), though
> > I'm not
> > sure how. Also, I'm not sure if yours should be 23 or 24-- did we miss
> > one?
> > Nate would know.

Yes, test is needed, sorry I missed that earlier.

dat file required, see DumpExpectedPageStateForBackwardsCompat for how to
generate that. This should be BackwardsCompat_v23 (i.e., ensuring we can still
import state from v23 format even though it's no longer current).

> >
> > https://codereview.chromium.org/2628683003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[chaopeng, 2017-02-15 20:19:46]

Added Backwards test. PTAL, thank you.

[Nate Chapin, 2017-02-16 19:16:55]

lgtm

[chaopeng, 2017-02-16 19:17:33]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-02-16 19:17:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2628683003/#ps380001
(title: "add dump file")

[commit-bot: I haz the power, 2017-02-16 19:19:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 22:09:07]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1487272653151960,
"parent_rev": "dda9580da15e121fc0a6273a30cc9e49584a469f", "commit_rev":
"1033be6d096d430b3d1fe6439550448b2374ea4c"}

[commit-bot: I haz the power, 2017-02-16 22:10:02]

Description was changed from

==========
Add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState
restore
from default

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment| in |checkCompleted| and restore from 
default. 

So we add DidSaveScrollOrScaleState flag to prevent
restoreScrollPositionAndViewState
restore from default.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add DidSaveScrollOrScaleState flag to prevent restoreScrollPositionAndViewState
restore
from default

This flaky is caused by |FrameLoader::restoreScrollPositionAndViewState| called
before |FrameLoader::processFragment| in |checkCompleted| and restore from 
default. 

So we add DidSaveScrollOrScaleState flag to prevent
restoreScrollPositionAndViewState
restore from default.


BUG=672545
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2628683003
Cr-Commit-Position: refs/heads/master@{#451111}
Committed:
https://chromium.googlesource.com/chromium/src/+/1033be6d096d430b3d1fe6439550...
==========

[commit-bot: I haz the power, 2017-02-16 22:10:04]

Committed patchset #19 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/1033be6d096d430b3d1fe6439550...

[chaopeng, 2017-02-22 18:52:27]

A revert of this CL (patchset #19 id:380001) has been created in
https://codereview.chromium.org/2710063002/ by chaopeng@chromium.org.

The reason for reverting is: Memory regression.
https://bugs.chromium.org/p/chromium/issues/detail?id=694569.