Refactor nsXREAppData so that Firefox -app and builtin share more codepaths, less manual memory management

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

Spinoff from bug 1306327 comment 7. Right now the normal Firefox startup path and the -app startup path are separate in annoying ways. Unifying those codepaths will make things better.
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Created attachment 8816187 [details]
> Bug 1321593 - Unify the startup paths between normal Firefox startup and
> firefox -app startup. . As part of this change, this reorganizes the
> nsXREAppData and ScopedAppData classes:
> 
> * Rename nsXREAppData to XREAppData, and make it strongly own references.
> * Separate the compiled-in appdata into its own class "StaticXREAppData"
> * Remove the owner/helper class ScopedAppData and related SetAllocatedString
> helpers
> * Remove the nsXREAppData.size field which is no longer relevant as we don't
> support cross-version embedding
> * Remove XRE_CreateAppData and XRE_FreeAppData helpers which are no longer
> necessary

Can I ask you to make each of those in separate patches? You don't need to do that in separate bugs, just separate patches.
Flags: needinfo?(benjamin)
I can separate out the -app/nsBrowserApp bits, but I don't think I can separate the other refactorings in a useful way. createappdata/freeappdata for example are tied to the ownership model, which changes at the same time.
Flags: needinfo?(benjamin)
Comment on attachment 8816447 [details]
Bug 1321593 part A - Refactor nsXREAppData: 1) make nsXREAppData strongly own its members 2) rename it to mozilla::XREAppData 3) separate out the static compiled data into StaticXREAppData 4) Remove XRE_CreateAppData and XRE_FreeAppData,

https://reviewboard.mozilla.org/r/97196/#review98024

FWIW, I don't see an obvious reason why this couldn't have been split in smaller pieces. That would have made the review much easier.
Attachment #8816447 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8816187 [details]
Bug 1321593 part B - Unify the startup path between `firefox --app` and normal Firefox startup,

https://reviewboard.mozilla.org/r/96954/#review98026
Attachment #8816187 - Flags: review?(mh+mozilla) → review+
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eae2252a519f
part A - Refactor nsXREAppData: 1) make nsXREAppData strongly own its members 2) rename it to mozilla::XREAppData 3) separate out the static compiled data into StaticXREAppData 4) Remove XRE_CreateAppData and XRE_FreeAppData 5) remove the struct size and related size-checking code which was only ever useful for cross-version compatibility, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/e531af57cd60
part B - Unify the startup path between `firefox --app` and normal Firefox startup, r=glandium
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e38abdcdd8
part A - Refactor nsXREAppData: 1) make nsXREAppData strongly own its members 2) rename it to mozilla::XREAppData 3) separate out the static compiled data into StaticXREAppData 4) Remove XRE_CreateAppData and XRE_FreeAppData 5) remove the struct size and related size-checking code which was only ever useful for cross-version compatibility, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/620f6fd44770
part B - Unify the startup path between `firefox --app` and normal Firefox startup. With fixup to properly release/not-leak xreDirectory r=glandium
It was a "simple" refcount leak in nsBrowserApp, fixed with a COMPtr.
Flags: needinfo?(benjamin)
Blocks: 1323313
https://hg.mozilla.org/mozilla-central/rev/a0e38abdcdd8
https://hg.mozilla.org/mozilla-central/rev/620f6fd44770
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.