1443355 - Remove nsIWebPageDescriptor

Status: NEW

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

Description

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

This interface has 2 members: the nsIWebPageDescriptor::loadPage function and the nsIWebPageDescriptor::pageDescriptor property.

(N.B. this interface is only implemented on nsDocShell)

# nsIWebPageDescriptor::loadPage

It looks like this function is exclusively used by view-source code. There are exactly 3 callers, here's a few notes:

1. This code path in openInExternalEditor is, I'm pretty sure, unreachable (as I can't find any codepath which sets data.pageDescriptor - aCallBack only is set in one callsite, and it doesn't mutate its data argument: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/base/content/browser.js#2663-2667). I think we can just remove it entirely (:bz's comment suggests it's broken anyway).
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/content/viewSourceUtils.js#240-254

2. I think this code is unreachable too. It's only called from setCharacterSet, which is only called in the "ViewSource:SetCharacterSet" message callback, and I can't find any copies of that string which could be used to call that function: 
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/content/viewSource-content.js#637-666
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/content/viewSource-content.js#129-131

3. This is the final remaining consumer. It will sometimes be passed a pageDescriptor for the docShell which holds the document we want to view the source of. We should probably just add a nsIDocShell::loadViewSourceFor(nsIDocShell* aOtherDocShell) which just pulls the needed information directly, and trash this function.
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/content/viewSource-content.js#298-300


# nsIWebPageDescriptor::currentDescriptor

The currentDescriptor of a nsIWebPageDescriptor is always a nsISHEntry. Namely, the actual current nsISHEntry is cloned, and has some fields nulled out, then it is returned. It has a few consumers:

1. It is fetched sometimes to pass to loadPage - as I mention above we can remove those consumers.

2. Devtools uses it to get a nsIWebPageDescriptor::GetCurrentDescriptor to get a nsISHEntry to pull post data out of. We should do this some other way. The easiest for now would be to grab the original nsISHEntry without going through the cloning rigamarole.
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/devtools/shared/webconsole/network-helper.js#181-193

3. WebBrowserPersistLocalDocument uses this to get the current nsISHEntry to get the cache key and the post data. This data is sent to the parent process if it's not already there. Moving Session History to the parent process will simplify the x-process stuff, but for now we can just grab the real nsISHEntry in GetHistory() rather than getting the cloned currentDescriptor one.
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp#169-225
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/dom/webbrowserpersist/WebBrowserPersistDocumentChild.cpp#61-71

4. nsWebBrowserPersist grabs the cache key with this. We should just get the real nsISHEntry:
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/dom/webbrowserpersist/nsWebBrowserPersist.cpp#1354-1376

5. contentAreaUtils.js:saveDocument() uses it to get the cache key. Again we can just use the real nsISHEntry. That being said, I'm pretty sure this code is completely broken and never gets the key, as we QI the key to nsISupportsPRUInt32, and then calls .data on it twice (it should only do that once), meaning an exception is always raised...
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/content/contentAreaUtils.js#216-227

6. contentAreaUtils.js:continueSave() then gets the session history for post data again. Like before, we can just use the normal nsISHEntry fetching route.
https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/content/contentAreaUtils.js#440

# Changes:

I think we should perhaps consider adding getters for the cache key & the post data for the current document to nsIDocShell, and use those in the places where the descriptor is being abused, add a mechanism for performing that view source load directly, and then rip out the interface completely.



Adtnl. note: ViewSource:LoadSourceDeprecated looks completely unused, and can probably be deleted: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/content/viewSource-content.js#113-116

Fun fact: It seems that there is a bug 317377 [Make nsIWebPageDescriptor more useful] which I suppose will be fixed by deleting the interface ^_^

Comment 1

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

Figure I should make sure there's no important reason why we need to keep this interface around, before I spend time killing it.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am happy to review View Source changes here, if desired.

Bug 1418403 is a recent change that led to some of the dead code you are finding here.  Looks like the sending side for some messages was removed, but the listeners are still around.  Let me know if you intend to work on this soon.  I filed bug 1418403 for cleaning up this dead code.

(In reply to Nika Layzell [:mystor] from comment #0)
> # nsIWebPageDescriptor::loadPage
> 
> It looks like this function is exclusively used by view-source code. There
> are exactly 3 callers, here's a few notes:
> 
> 1. This code path in openInExternalEditor is, I'm pretty sure, unreachable
> (as I can't find any codepath which sets data.pageDescriptor - aCallBack
> only is set in one callsite, and it doesn't mutate its data argument:
> https://searchfox.org/mozilla-central/rev/
> bffd3e0225b65943364be721881470590b9377c1/browser/base/content/browser.
> js#2663-2667). I think we can just remove it entirely (:bz's comment
> suggests it's broken anyway).
> https://searchfox.org/mozilla-central/rev/
> bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/
> content/viewSourceUtils.js#240-254

I agree, we never get a page descriptor in this code path anymore.  It was possible via a deprecated arg before bug 1418403.

> 2. I think this code is unreachable too. It's only called from
> setCharacterSet, which is only called in the "ViewSource:SetCharacterSet"
> message callback, and I can't find any copies of that string which could be
> used to call that function: 
> https://searchfox.org/mozilla-central/rev/
> bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/
> content/viewSource-content.js#637-666
> https://searchfox.org/mozilla-central/rev/
> bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/
> content/viewSource-content.js#129-131

Correct, the sender of that message was removed in bug 1418403, so this path can also be removed.

> Adtnl. note: ViewSource:LoadSourceDeprecated looks completely unused, and
> can probably be deleted:
> https://searchfox.org/mozilla-central/rev/
> bffd3e0225b65943364be721881470590b9377c1/toolkit/components/viewsource/
> content/viewSource-content.js#113-116

Yes, it's now dead code after bug 1418403.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Oops, meant to say I filed bug 1443371 for the cleanup.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the idea here was to have a thing for view-source.  And we wanted to make it be basically an opaque token.

The fact that people are assuming they can QI that token nsISHEntry is ... sigh.  XPCOM strikes again.

I would have no problem with changing to an API where we just bypass the opaque token and have a "tell this docshell to view the source of this other docshell" mechanism, as long as we never need to do this cross-process.

Comment 5

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

Alright, that makes a lot of sense. I'll wait for :jryans bug to land then I'll purge the rest of the consumers.