Remove remaining link "prerender" code

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
2 years ago
10 days ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

({dev-doc-complete})

unspecified
mozilla59
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
We don't intend to finish the implementation which was added in bug 1315105, so we should remove the code which was added in that bug and related bugs.

However, we do want to keep the GroupedSHistory work which :freesamael worked on, as we may want to extend it in the future to improve cross-process loads in e10s-multi. 

This bug is to track removing the rest of that code.
Priority: -- → P3
Keywords: dev-doc-needed, site-compat
(Assignee)

Comment 1

2 years ago
This was never shipped or documented anywhere as far as I know - I doubt we'll need any dev-doc work.
(In reply to Michael Layzell [:mystor] from comment #1)
> This was never shipped or documented anywhere as far as I know - I doubt
> we'll need any dev-doc work.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1315105#c63
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 3

a year ago
I'm taking this to remove this unused code to start working towards bug 1419857.
Assignee: nobody → nika
(Assignee)

Comment 4

a year ago
This is the first part of removing as much of the prerendering logic as I could
find from the code.

It would be good to go over this & make sure that we actually wanted to remove
everything which I remove here. The second patch also removes a bunch of tests
which we also need to make sure we actually want to remove.

If you notice something I totaly missed let me know.

I'm doing a try push with this right now.

MozReview-Commit-ID: 2aHA6NcQPGk
Attachment #8936944 - Flags: review?(sawang)
(Assignee)

Comment 5

a year ago
MozReview-Commit-ID: KwvOcgQKheK
Attachment #8936945 - Flags: review?(sawang)
(Assignee)

Comment 6

a year ago
I found more ^.^

MozReview-Commit-ID: IuJHv3n0Uob
Attachment #8936949 - Flags: review?(sawang)
(Assignee)

Comment 7

a year ago
MozReview-Commit-ID: 5j8fNeKuHAM
Attachment #8936950 - Flags: review?(sawang)
Comment on attachment 8936944 [details] [diff] [review]
Part 1: Remove GroupedSHistory and Prerendering logic from C++ code

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

I tried my best...

::: docshell/shistory/nsSHistory.cpp
@@ +1651,1 @@
>    } else {

Remove the `else` since it early returns above.

::: dom/base/nsFrameLoader.cpp
@@ +364,2 @@
>  bool
>  nsFrameLoader::SwapBrowsersAndNotify(nsFrameLoader* aOther)

Do you plan to keep `BrowserWillChangeProcess` & `BrowserChangedProcess` stuff? Otherwise this can be removed I think.

@@ +401,2 @@
>  already_AddRefed<Promise>
>  nsFrameLoader::FireWillChangeProcessEvent()

Same here.

@@ +437,2 @@
>  void
>  nsFrameLoader::AddProcessChangeBlockingPromise(Promise& aPromise, ErrorResult& aRv)

And here. And the whole `GroupedHistoryEvent` class + webidl.

::: dom/base/nsFrameLoader.h
@@ +468,3 @@
>    // A stack-maintained reference to an array of promises which are blocking
>    // grouped history navigation
>    nsTArray<RefPtr<mozilla::dom::Promise>>* mBrowserChangingProcessBlockers;

Same here.

::: dom/ipc/TabChild.cpp
@@ +3510,2 @@
>  already_AddRefed<nsISHistory>
>  TabChild::GetRelatedSHistory()

Looks like we removed all its callers.

::: dom/webidl/FrameLoader.webidl
@@ -12,2 @@
>  interface nsIMessageSender;
> -interface nsIPartialSHistory;

We can remove these 2 bindings:
https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/dom/bindings/Bindings.conf#1742-1743,1750-1751

::: dom/xul/nsXULElement.cpp
@@ -1491,5 @@
>          (new AsyncEventDispatcher(this,
>                                    NS_LITERAL_STRING("XULFrameLoaderCreated"),
>                                    /* aBubbles */ true))->RunDOMEventWhenSafe();
> -
> -        if (AttrValueIs(kNameSpaceID_None, nsGkAtoms::prerendered,

And the GkAtoms. 

Do you happen to know how the rust atoms mapping are generated? We may want to also remove this one:
https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/servo/components/style/gecko/generated/atom_macro.rs#2220
Attachment #8936944 - Flags: review?(sawang) → review+
Comment on attachment 8936949 [details] [diff] [review]
Part 3: Remove prerendering methods from nsIWebBrowserChrome3.idl

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

One missing implementation:
https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/toolkit/components/extensions/ext-browser-content.js#315-320
Attachment #8936949 - Flags: review?(sawang) → review+
(Assignee)

Comment 10

a year ago
FYI: I want to keep around the GroupedHistoryEvent, BrowserChangedProcess, BrowserWillChangeProcess and the process blockers, because I'm going to want to use them in the future.

I will probably rename GroupedHistoryEvent.
Attachment #8936945 - Flags: review?(sawang) → review+
Attachment #8936950 - Flags: review?(sawang) → review+
(Assignee)

Comment 11

a year ago
MozReview-Commit-ID: 2aHA6NcQPGk
(Assignee)

Updated

a year ago
Attachment #8936944 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8936945 - Attachment is obsolete: true
Attachment #8936949 - Attachment is obsolete: true
Attachment #8936950 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
MozReview-Commit-ID: KwvOcgQKheK
(Assignee)

Comment 13

a year ago
MozReview-Commit-ID: IuJHv3n0Uob
(Assignee)

Comment 14

a year ago
MozReview-Commit-ID: 5j8fNeKuHAM
(Assignee)

Comment 15

a year ago
MozReview-Commit-ID: 1HKNAeI7hSV
(Assignee)

Updated

a year ago
Attachment #8937131 - Flags: review?(sawang)
Attachment #8937131 - Flags: review?(sawang) → review+
(Assignee)

Comment 16

a year ago
Comment on attachment 8937127 [details] [diff] [review]
Part 1: Remove GroupedSHistory and Prerendering logic from C++ code

I forgot that Samael isn't a DOM peer (and neither am I), so I'm going to need you to sign off on my webidl changes.

Nothing web visible has been changed.
Attachment #8937127 - Flags: review?(bugs)
So we don't want to use this stuff to handle process switches properly? Or do we not want to switch process ever for a tab?
Comment on attachment 8937127 [details] [diff] [review]
Part 1: Remove GroupedSHistory and Prerendering logic from C++ code

r+ for the .webidl.
Attachment #8937127 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

a year ago
I went through this code again, and I'm pretty sure we don't want any of this stuff even if we want to have bfcache in the first version of improved process switching.

I've updated the patch to apply on the new tree & I'm going to land it.

Comment 20

a year ago
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/156c6bee1752
Part 1: Remove GroupedSHistory and Prerendering logic from C++ code, r=freesamael, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/156088fcf75b
Part 2: Remove GroupedSHistory and Prerendering logic from JS code, r=freesamael
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89b001fd9f5
Part 3: Remove prerendering methods from nsIWebBrowserChrome3.idl, r=freesamael
https://hg.mozilla.org/integration/mozilla-inbound/rev/c61976e2d3ac
Part 4: Remove even more tests which no longer work, r=freesamael, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9855f7bfdb
Part 5: Clean up in moz.build files, r=freesamael
Duplicate of this bug: 1340872
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.