Closed Bug 1383876 Opened 3 years ago Closed 3 years ago

Remove remaining link "prerender" code

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 4 obsolete files)

210.87 KB, patch
smaug
: review+
Details | Diff | Splinter Review
94.32 KB, patch
Details | Diff | Splinter Review
8.94 KB, patch
Details | Diff | Splinter Review
11.92 KB, patch
Details | Diff | Splinter Review
975 bytes, patch
freesamael
: review+
Details | Diff | Splinter Review
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: site-compat
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
I'm taking this to remove this unused code to start working towards bug 1419857.
Assignee: nobody → nika
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)
MozReview-Commit-ID: KwvOcgQKheK
Attachment #8936945 - Flags: review?(sawang)
I found more ^.^

MozReview-Commit-ID: IuJHv3n0Uob
Attachment #8936949 - Flags: review?(sawang)
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+
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+
Attachment #8936944 - Attachment is obsolete: true
Attachment #8936945 - Attachment is obsolete: true
Attachment #8936949 - Attachment is obsolete: true
Attachment #8936950 - Attachment is obsolete: true
MozReview-Commit-ID: 5j8fNeKuHAM
MozReview-Commit-ID: 1HKNAeI7hSV
Attachment #8937131 - Flags: review?(sawang)
Attachment #8937131 - Flags: review?(sawang) → review+
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+
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.
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
You need to log in before you can comment on or make changes to this bug.