1539482 - Reduce the number of IPC calls on nsISHEntry in nsDocShell::LoadHistoryEntry

Status: RESOLVED FIXED

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

Comment 1

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1539482 - Reduce the number of IPC calls on nsISHEntry in nsDocShell::LoadHistoryEntry,

In nsDocShell::LoadHistoryEntry method, there are 13 sync IPC calls
on nsISHEntry that retrieve information from the session
history entry and create a doc shell load state object
using the retrieved information. By adding a new method
'CreateLoadInfo'on nsISHEntry, inside of which the
doc shell load state object will be created (with
appropriate data filled out) and returned, we eliminate 12
sync IPC call, resulting in just 1 IPC call to
nsISHEntry::CreateLoadInfo.

Comment 2

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1539482 - Write tests for nsISHEntry::CreateLoadInfo,

Depends on D26042

Comment 3

[Anny Gakhokidze [:annyG]]

Attachment: diff.patch

Comment 4

[Anny Gakhokidze [:annyG]]

Hi Kyle,

I was wondering if you could help shed any light on the failure I am getting here https://treeherder.mozilla.org/#/jobs?repo=try&revision=c75cc3eb7ef40979defb57553e4b68f65c6f930f&selectedJob=244926456

Just now I have discovered two things in my patch that were wrong, but even with those things fixed, the test fails (I don't have a try run with the new changes added because I haven't had time to get a try run).

The changes are attached in diff.patch

Thanks so much,
Anny

P.S. Clearly that |NS_WARNING("ANNY --- ");| is very necessary.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Do we know if we're ending up with the exact same LoadState object after these improvements?

When I was working on the initial loadstate implementation, I basically had to develop 2 pipelines (keeping the old one around while building a new one) to compare the object I was building with the mess of arguments we were getting from LoadURI to InternalLoad. That exercise might be helpful here, via implementing some sort of object equality test in DocShellLoadState, if that's even possible.

That way, at the end of LoadHistoryEntry, we could check whether our single IPC call DocShellLoadState objects are the same as the old, lots of calls version. I would suspect this will be a handy tool to have later too, since these objects are going to end up all over the place and probably katamari-ing more properties in.

Comment 6

[Anny Gakhokidze [:annyG]]

Pushed to ash branch

https://hg.mozilla.org/projects/ash/rev/6ad53b35c7b4be933a3db1e1d45fa3da8a57abad
https://hg.mozilla.org/projects/ash/rev/88d429a1f9e4dc7cfae21d3690680a30a92f879f

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eaeccea05e5
Reduce the number of IPC calls on nsISHEntry in nsDocShell::LoadHistoryEntry, r=peterv, r=nika for adding sync IPC messages
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f24a5236c9
Write tests for nsISHEntry::CreateLoadInfo, r=peterv

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5eaeccea05e5
https://hg.mozilla.org/mozilla-central/rev/a7f24a5236c9