1438026 - Replace nsLayoutHistoryState and nsPresState with something easier to serialize

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

These types are fairly simple under the hood, and mostly just contain hashtables, strings, and basic types.

We will want to be able to serialize these objects in order to implement the improved session history APIs, as they save important information. It shouldn't be too difficult to replace the APIs under the hood with easier to serialize ones built on IPDL structs and unions.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add IPDLParamTraits for RefPtr<BlobImpl>

This impl supports serializing null RefPtr<BlobImpl> objects, and will handle a
failure to serialize a BlobImpl by sending a nullptr. This was the best error
handling option which I have access to inside of IPDLParamTraits unless we
implement bug 1442678. This means consumers of this interface always have to
sanely handle null blobimpls on the receiver.

MozReview-Commit-ID: 61UmQEjn7BF

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add IPDL definitions for the PresState data structures

MozReview-Commit-ID: 1p4B7czWPvZ

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Replace nsPresState with the new PresState type

MozReview-Commit-ID: JXSlpqQNIbL

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Remove the old nsPresState code completely

MozReview-Commit-ID: Cl6IaxP6xHW

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8955598 [details] [diff] [review]
Part 3: Replace nsPresState with the new PresState type

Review of attachment 8955598 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLButtonElement.cpp
@@ +28,5 @@
>  #include "mozilla/MouseEvents.h"
>  #include "mozilla/TextEvents.h"
>  #include "nsUnicharUtils.h"
>  #include "nsLayoutUtils.h"
> +#include "mozilla/PresState.h"

alphabetic order. Or something similar...

::: dom/html/HTMLInputElement.cpp
@@ +6242,5 @@
>    return aFormSubmission->AddNameValuePair(name, value);
>  }
>  
> +static nsTArray<FileContentData>
> +SaveFileContentData(const nsTArray<OwningFileOrDirectory>& aArray)

Can you please have nsTArray<> as argument instead of returning it?

@@ +6508,5 @@
>  }
>  
> +static nsTArray<OwningFileOrDirectory>
> +RestoreFileContentData(nsPIDOMWindowInner* aWindow,
> +                       const nsTArray<FileContentData>& aData)

same here.

::: dom/html/HTMLTextAreaElement.cpp
@@ +35,5 @@
>  #include "nsLinebreakConverter.h"
>  #include "nsMappedAttributes.h"
>  #include "nsPIDOMWindow.h"
>  #include "nsPresContext.h"
> +#include "mozilla/PresState.h"

Maybe move it near a mozilla/something_else include.

::: layout/base/nsLayoutHistoryState.cpp
@@ +11,5 @@
>  
>  #include "nsILayoutHistoryState.h"
>  #include "nsWeakReference.h"
>  #include "nsClassHashtable.h"
> +#include "mozilla/PresState.h"

alphabetic order.

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8955599 [details] [diff] [review]
Part 4: Remove the old nsPresState code completely

Review of attachment 8955599 [details] [diff] [review]:
-----------------------------------------------------------------

wonderful.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c1e0d112e26
Part 1: Add IPDLParamTraits for RefPtr<BlobImpl>, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dadb3852649
Part 2: Add IPDL definitions for the PresState data structures, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f394ca4f80af
Part 3: Replace nsPresState with the new PresState type, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1fab4fd5afa
Part 4: Remove the old nsPresState code completely, r=baku

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1c1e0d112e26
https://hg.mozilla.org/mozilla-central/rev/7dadb3852649
https://hg.mozilla.org/mozilla-central/rev/f394ca4f80af
https://hg.mozilla.org/mozilla-central/rev/e1fab4fd5afa

Comment 9

[:aceman]

After this change I get a permanent compile errors in files using NewPresState() and similar.
What may be going wrong? This is on Linux with gcc 7.3.
At least some of the files calling NewPresState() do #include "nsILayoutHistoryState.h" which contains a stub declaration of it.

In file included from mozilla/layout/base/nsCSSFrameConstructor.h:21:0,
                  from mozilla/layout/base/nsBidiPresUtils.cpp:18,
                  from tbird-bin/layout/base/Unified_cpp_layout_base1.cpp:2:
 /mozilla/layout/base/nsILayoutHistoryState.h:17:7: note: class type ‘nsPresState’ is incomplete
  class nsPresState; /* forward declaration */
        ^~~~~~~~~~~
 In file included from bin/layout/base/Unified_cpp_layout_base1.cpp:92:0:
 mozilla/layout/base/nsLayoutHistoryState.cpp: In member function ‘virtual nsresult nsLayoutHistoryState::AddNewPresState(const nsACString&, float, float, bool, float, bool)’:
 mozilla/layout/base/nsLayoutHistoryState.cpp:106:35: error: ‘NewPresState’ was not declared in this scope
    UniquePtr<PresState> newState = NewPresState();
                                    ^~~~~~~~~~~~
 mozilla/layout/base/nsLayoutHistoryState.cpp:106:35: note: suggested alternative: ‘GetPresState’
    UniquePtr<PresState> newState = NewPresState();
                                    ^~~~~~~~~~~~
                                    GetPresState
 mozilla/layout/base/nsLayoutHistoryState.cpp: At global scope:
 mozilla/layout/base/nsLayoutHistoryState.cpp:118:1: error: prototype for ‘void nsLayoutHistoryState::AddState(const nsCString&, mozilla::UniquePtr<mozilla::PresState>)’ does not match any in class ‘nsLayoutHistoryState’
  nsLayoutHistoryState::AddState(const nsCString& aStateKey, UniquePtr<PresState> aState)
  ^~~~~~~~~~~~~~~~~~~~
 In file included from mozilla/layout/base/nsCSSFrameConstructor.h:21:0,
                  from mozilla/layout/base/nsBidiPresUtils.cpp:18,
                  from tbird-bin/layout/base/Unified_cpp_layout_base1.cpp:2:
 mozilla/layout/base/nsILayoutHistoryState.h:74:16: error: candidate is: virtual void nsLayoutHistoryState::AddState(const nsCString&, nsPresState*)
    virtual void AddState(const nsCString & aKey, nsPresState *aState) override; \
                 ^
 mozilla/layout/base/nsILayoutHistoryState.h:74:16: note: in definition of macro ‘NS_DECL_NSILAYOUTHISTORYSTATE’
    virtual void AddState(const nsCString & aKey, nsPresState *aState) override; \
                 ^~~~~~~~
 In file included from bin/layout/base/Unified_cpp_layout_base1.cpp:92:0:
 mozilla/layout/base/nsLayoutHistoryState.cpp:124:1: error: prototype for ‘mozilla::PresState* nsLayoutHistoryState::GetState(const nsCString&)’ does not match any in class ‘nsLayoutHistoryState’
  nsLayoutHistoryState::GetState(const nsCString& aKey)
  ^~~~~~~~~~~~~~~~~~~~
 In file included from mozilla/layout/base/nsCSSFrameConstructor.h:21:0,
                  from mozilla/layout/base/nsBidiPresUtils.cpp:18,
                  from bin/layout/base/Unified_cpp_layout_base1.cpp:2:
 mozilla/layout/base/nsILayoutHistoryState.h:75:25: error: candidate is: virtual nsPresState* nsLayoutHistoryState::GetState(const nsCString&)
    virtual nsPresState * GetState(const nsCString & aKey) override; \
                          ^
 mozilla/layout/base/nsILayoutHistoryState.h:75:25: note: in definition of macro ‘NS_DECL_NSILAYOUTHISTORYSTATE’
    virtual nsPresState * GetState(const nsCString & aKey) override; \
                          ^~~~~~~~
 gmake[4]: *** [mozilla/config/rules.mk:1024: Unified_cpp_layout_base1.o] Error 1
 gmake[3]: *** [mozilla/config/recurse.mk:73: layout/base/target] Error 2

Comment 10

[Jorg K (CEST = GMT+2)]

Another developer reports this on Mac/clang:
"layout/generic/nsGfxScrollFrame.cpp:6221:32: error: use of undeclared identifier 'NewPresState'"

No such error in automation or on Windows.

Comment 11

[Jorg K (CEST = GMT+2)]

Looks like people had nsILayoutHistoryState.h in the source tree(!??).