1559414 - Make it clear that methods which won't work past Fission are process-bound

Status: RESOLVED FIXED

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

Description

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

This patch is effectively a series of find & replaces on a series of named methods on DOM objects. The goal is to make the consequences more clear.

They aren't marked as Legacy or Deprecated, as their behaviour may be wanted in certain edge cases, but it is now clear from reading them that they aren't working unconditionally.

The goal is to prevent new features being added which depend on these methods being used, without first having them consider the impact they will have on the fission project, and the eventual need to rewrite.

I'm open to bikeshedding on naming, as usual.

Comment 1

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

Attachment: Bug 1559414 - Make it more clear that certain methods only work in-process

This patch is effectively a series of find & replaces on a series of named
methods on DOM objects. The goal is to make the consequences more clear.

They aren't marked as Legacy or Deprecated, as their behaviour may be wanted in
certain edge cases, but it is now clear from reading them that they aren't
working unconditionally.

The goal is to prevent new features being added which depend on these methods
being used, without first having them consider the impact they will have on the
fission project, and the eventual need to rewrite.

I'm open to bikeshedding on naming, as usual.

Comment 2

[Neha Kochar [:neha]]

Kashav, please get more context from Nika next week to start looking into this.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: rebase.patch

I rebased this patch to tip and got it building on inbound tip again. A couple of places where the method uses needed to be changed. Rot since Nika posted the patch, or potentially pre-existing.

Nika, are there more methods you wanted to name-mangle the definition of and convert the uses of? Or is it worthwhile to go looking for more like the ones you've already done?

Also, I have zero interest in bikeshedding the naming for refactoring scaffolding :) *SameProcess* is fine by me.

Comment 4

[Neha Kochar [:neha]]

Assigning to Kannan as he's started looking into this.

Comment 5

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

(In reply to Kannan Vijayan [:djvj] from comment #3)

Created attachment 9076923 [details] [diff] [review]
rebase.patch

I rebased this patch to tip and got it building on inbound tip again. A couple of places where the method uses needed to be changed. Rot since Nika posted the patch, or potentially pre-existing.

Nika, are there more methods you wanted to name-mangle the definition of and convert the uses of? Or is it worthwhile to go looking for more like the ones you've already done?

The big methods which haven't been changed yet are ones with super generic names, and the JS callers. For example:

• nsIDocShellTreeItem::GetParent: https://searchfox.org/mozilla-central/rev/f372470e10c8cb0691681603a1d6324dee5b3b8a/docshell/base/nsIDocShellTreeItem.idl#57
• nsIDocShellTreeItem::GetChildAt, ChildCount, etc: https://searchfox.org/mozilla-central/rev/f372470e10c8cb0691681603a1d6324dee5b3b8a/docshell/base/nsIDocShellTreeItem.idl#138,161

Also, I have zero interest in bikeshedding the naming for refactoring scaffolding :) *SameProcess* is fine by me.

The pattern I was going to use was GetSameProcessTop (or root) and GetParentIfSameProcess. I can't remember how consistent I was when I made this initial patch.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: rename-dom-sameprocess-methods.patch

Updated to tip and added SameProcess annotation for GetParent and ChildCount and GetChildAt.

Comment 7

[Kannan Vijayan [:djvj]]

So this patch isn't done yet. Try run revealed a bunch of android and probably some OSX-specific code I'd missed when getting this to compile locally. Addressing those now.

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: rename-dom-sameprocess-methods.patch

Updated patch that seems to be complete. Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15ba909ecd626846fc8f2175be59eda0c744e417&selectedJob=257721986

Comment 9

[Kannan Vijayan [:djvj]]

Attachment: Bug 1559414 - Rename unaudited pre-fission methods with SameProcess for future audit burndown. r?nika

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: Bug 1559414 - Rename unaudited pre-fission methods with SameProcess for future audit burndown. r?nika

Comment 11

[Kannan Vijayan [:djvj]]

Attachment: Bug 1559414 - Rename unaudited pre-fission methods with SameProcess for future audit burndown. r=nika

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2d3ccb209c5
Rename unaudited pre-fission methods with SameProcess for future audit burndown. r=nika

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f2d3ccb209c5

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8254990cee1b
Port bug 1559414 - s/GetSameTypeRootTreeItem/GetInProcessSameTypeRootTreeItem/. rs=bustage-fix

Comment 15

[Kannan Vijayan [:djvj]]

Sorry for the bother, jorg. Thanks for fixing this up in mailnews (I'm guessing this is thunderbird related). Going forward, how can I ensure I don't break out-of-tree builds?

There is another bug that will land shortly that might break things downstream (another pure rename patch): bug 1569262

Comment 16

[Jorg K (CEST = GMT+2)]

Hi :-)
Thanks for getting back to me. Yes, Mailnews contains the C++ backend for Thunderbird. Some of the Mozilla developers are friends of Thunderbird since the use the software themselves. We usually get CC'ed or NI'ed on the relevant bugs, or some even file bugs in Thunderbird or Mailnews for us. In case of very simple stuff, I'm happy just to push a patch into the M-C bug, like I've done here.

You can easily check using Searchfox on comm-central:
https://searchfox.org/comm-central/source, in the case of bug 1569262 it's this:
https://searchfox.org/comm-central/search?q=GetRootTreeItem&case=true&regexp=false&path=

Comment 17

[Neha Kochar [:neha]]

Fixing my mistake, changed the wrong bug.