1343900 - Deduplicate logic between nsAboutRedirector and AboutRedirector

Status: NEW

(Core :: General, enhancement)

Description

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

Right now nsAboutRedirector and AboutRedirector share a lot of duplicated code. It would be nice if they could instead use a shared set of common underlying, potentially with a set of macros for defining specific redirects which can be split between docshell and the browser.

This would remove the need to keep these two implementations of nsIAboutModule in sync.

Comment 1

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

Ni?-ing gijs to see if Firefox has any objections to merging this logic back together.

Comment 2

[:Gijs (he/him)]

(In reply to Michael Layzell [:mystor] from comment #0)
> This would remove the need to keep these two implementations of
> nsIAboutModule in sync.

Except these aren't the only 2 implementations, I think...

https://dxr.mozilla.org/mozilla-central/search?q=nsIAboutModule

Add-ons and tests create their own, about:cache(-entry) has its own... it's messy.

Of course, unifying the browser/ and docshell/ ones might still be a good idea. But I don't think it gets us a single implementation of that interface.

Comment 3

[:Gijs (he/him)]

(In reply to Michael Layzell [:mystor] from comment #1)
> Ni?-ing gijs to see if Firefox has any objections to merging this logic back
> together.

I'm not sure what code this is and how easy it is given comment #2. I wouldn't object to merging the logic, but I expect the other toolkit consumers that we're supposed to not care about but still care about a little bit (or whatever policy is these days) don't want to / expect to build/ship browser/ and its HTML files that underpin those about: pages.


TBH, I'd just as soon move the entire implementation into one/multiple js components. I don't think there's a lot of reasons to keep the module in C++.

Comment 4

[Michal Novotny [:michal]]

Doesn't look like a networking bug.

Comment 5

[Adrian Florinescu [:aflorinescu]]

:Gijs, could you please advise what component would be the best fit for this issue?

Comment 6

[:Gijs (he/him)]

(In reply to Adrian Florinescu [:AdrianSV] from comment #5)
> :Gijs, could you please advise what component would be the best fit for this
> issue?

I don't really know. I think Core::Networking (given that the about protocol implementation lives there) or Core/Firefox::General, or maybe Core::Document navigation given that nsAboutRedirector lives in docshell (for reasons I don't really understand).

Moving to Core::General because I don't think the component is really important at this point.

Needinfo Michael for comments #2 / 3. :-)

Comment 7

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

(In reply to :Gijs from comment #2)
> Except these aren't the only 2 implementations, I think...
> 
> https://dxr.mozilla.org/mozilla-central/search?q=nsIAboutModule
> 
> Add-ons and tests create their own, about:cache(-entry) has its own... it's
> messy.
> 
> Of course, unifying the browser/ and docshell/ ones might still be a good
> idea. But I don't think it gets us a single implementation of that interface.

I recognize that there are multiple implementations of the interface but it seems to me at least like they all have the same basic structure: a bunch of data which represents the redirects which the implementation supports, and then some common code which is all slightly different but which does the same thing in actually performing the URL rewrite.

My goal with this was to suggest that at least the 2 C++ implementations could share that data->nsIAboutModule implementation. Perhaps this would be best served by moving the code to JS and having a common implementation which each of the about modules can base themselves on.

I'm not particularly attached to this suggestion, I just found the current implementation a bit surprising when I was trying to figure out how the logic works last week.