Changing scroll and view state in onpopstate shouldn't overwrite back/forward state restore

third_party/WebKit/Source/core/loader/HistoryItem.h

Index: third_party/WebKit/Source/core/loader/HistoryItem.h
diff --git a/third_party/WebKit/Source/core/loader/HistoryItem.h b/third_party/WebKit/Source/core/loader/HistoryItem.h
index 4c5729c8ed32229d3eb78f877733ffbe24e79783..8795b0088cb4fc2e5b68ef01b48a027f0d52cd68 100644
--- a/third_party/WebKit/Source/core/loader/HistoryItem.h
+++ b/third_party/WebKit/Source/core/loader/HistoryItem.h
@@ -59,20 +59,37 @@ class CORE_EXPORT HistoryItem final
   EncodedFormData* FormData();
   const AtomicString& FormContentType() const;
 
-  void SetDidSaveScrollOrScaleState(bool did_save_scroll_or_scale_state) {
-    did_save_scroll_or_scale_state_ = did_save_scroll_or_scale_state;
-  }
+  class ScrollAndViewState : public GarbageCollected<ScrollAndViewState> {

[majidvp, 2017/06/26 16:19:26]

I consider scroll position to be part of the view state. In that light it seems
that a just calling this History::ViewState is more appropriate.

[Nate Chapin, 2017/07/11 22:17:59]

Done.

+   public:
+    static ScrollAndViewState* Create() { return new ScrollAndViewState; }
+
+    ScrollOffset visual_viewport_scroll_offset_;
+    ScrollOffset scroll_offset_;
+    float page_scale_factor_;
+
+    DEFINE_INLINE_TRACE() {}
+
+   private:
+    ScrollAndViewState() : page_scale_factor_(0) {}
+  };
 
-  bool DidSaveScrollOrScaleState() const {
-    return did_save_scroll_or_scale_state_;
+  ScrollAndViewState* GetScrollAndViewState() const {
+    return scroll_and_view_state_;
+  }
+  void ClearScrollAndViewState() { scroll_and_view_state_ = nullptr; }
+  ScrollAndViewState* CopyScrollAndViewState() const {
+    if (!scroll_and_view_state_)
+      return nullptr;
+    ScrollAndViewState* copy = ScrollAndViewState::Create();
+    copy->visual_viewport_scroll_offset_ =
+        scroll_and_view_state_->visual_viewport_scroll_offset_;
+    copy->scroll_offset_ = scroll_and_view_state_->scroll_offset_;
+    copy->page_scale_factor_ = scroll_and_view_state_->page_scale_factor_;

[majidvp, 2017/06/26 16:19:26]

This feels like it should be a copy constructor on the ScrollAndViewState class.

[Nate Chapin, 2017/07/11 22:17:59]

Reworked class to not be GCed, so it can have a copy constructor :)

+    return copy;
   }
 
-  const ScrollOffset& VisualViewportScrollOffset() const;
   void SetVisualViewportScrollOffset(const ScrollOffset&);
-  const ScrollOffset& GetScrollOffset() const;
   void SetScrollOffset(const ScrollOffset&);
-
-  float PageScaleFactor() const;
   void SetPageScaleFactor(float);
 
   Vector<String> GetReferencedFilePaths();
@@ -119,13 +136,11 @@ class CORE_EXPORT HistoryItem final
   String url_string_;
   Referrer referrer_;
 
-  bool did_save_scroll_or_scale_state_;
-  ScrollOffset visual_viewport_scroll_offset_;
-  ScrollOffset scroll_offset_;
-  float page_scale_factor_;
   Vector<String> document_state_vector_;
   Member<DocumentState> document_state_;
 
+  Member<ScrollAndViewState> scroll_and_view_state_;
+
   // If two HistoryItems have the same item sequence number, then they are
   // clones of one another. Traversing history from one such HistoryItem to
   // another is a no-op. HistoryItem clones are created for parent and