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

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

 /*
  * Copyright (C) 2006, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2012 Research In Motion Limited. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 41 matching lines @@
   ~HistoryItem();
 
   const String& UrlString() const;
   KURL Url() const;
 
   const Referrer& GetReferrer() const;
 
   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) {}
+  };
+
+  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;
   }
 
-  bool DidSaveScrollOrScaleState() const {
-    return did_save_scroll_or_scale_state_;
-  }
-
-  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();
   const Vector<String>& GetDocumentState();
   void SetDocumentState(const Vector<String>&);
   void SetDocumentState(DocumentState*);
   void ClearDocumentState();
 
   void SetURL(const KURL&);
   void SetURLString(const String&);
@@ skipping 26 matching lines @@
   ResourceRequest GenerateResourceRequest(WebCachePolicy);
 
   DECLARE_TRACE();
 
  private:
   HistoryItem();
 
   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
   // sibling frames when only a subframe navigates.
   int64_t item_sequence_number_;
 
   // If two HistoryItems have the same document sequence number, then they
   // refer to the same instance of a document. Traversing history from one
   // such HistoryItem to another preserves the document.
   int64_t document_sequence_number_;
 
   // Type of the scroll restoration for the history item determines if scroll
   // position should be restored when it is loaded during history traversal.
   HistoryScrollRestorationType scroll_restoration_type_;
 
   // Support for HTML5 History
   RefPtr<SerializedScriptValue> state_object_;
 
   // info used to repost form data
   RefPtr<EncodedFormData> form_data_;
   AtomicString form_content_type_;
 };  // class HistoryItem
 
 }  // namespace blink
 
 #endif  // HISTORYITEM_H