1324359 - Consider renaming DocShell's process lock stuff to something more understandable

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Samael Wang [:freesamael] (away for now)]

Based on review comment in bug 1315105 comment 42:

> >+  // Adopting an out-of-process prerendered document is conceptually similar to
> >+  // switching dochshell's process, since it's the same browsing context from
> >+  // other browsing contexts' perspective. If we're locked in current process,
> >+  // we can not prerender out-of-process.
> >+  if (docShell->GetIsProcessLocked()) {
> It is still rather unclear what "locked" means in this context.
> Could you please file a followup to rename "locked" to something
> understandable.

We may want to find a better name for the process lock stuff.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

DocShells are always "locked" to a certain process, so the explanation in 
http://searchfox.org/mozilla-central/rev/f680e72cc6579f90b992b63ca14d923d2afea612/docshell/base/nsIDocShell.idl#1107 is rather weird.
Wouldn't something like "[infallible] readonly attribute unsigned long allowNextLoadInDifferentProcess;"
be closer to what the attribute actually is about.
The consts would need to be renamed then too. Something like ALLOW; and DENY_IN_IFRAME, DENY_HAS_RELATED_CONTEXTS, DENY_NOT_CHILD_PROCESS

Comment 2

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

Attachment: Rename and simplify the DocShell ProcessLock concept

I tried renaming & simplifying the concept in this commit. I'd love both of your inputs on this.

MozReview-Commit-ID: JTp8Q2mWqpN

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822530 [details] [diff] [review]
Rename and simplify the DocShell ProcessLock concept

Should it be NonDependent, not Nondependent ?

What does 'content reference' mean, in particular, what does "content" mean in this context? Is it same as "browsing context" / docshells like the comment then seems to hint.

And why "non" in the name. Perhaps my English is bad, but I would think that hasDependentContentReferences means that there are such references, which prevent use of a new process. (but "content" is still mystery)

What does the following mean
"+   * This value is `true` if there exist docshells within this docshell's unit
+   * of related browsing contexts which would outlive a navigation load"
Does it mean that if there are child docshells and the current top level page will enter bfcache, the property is true?


Sorry, not clear enough to me, at least not at 3am.

Would hasDependentBrowsingContextReferences work?

Comment 4

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

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8822530 [details] [diff] [review]
> Rename and simplify the DocShell ProcessLock concept
> 
> Should it be NonDependent, not Nondependent ?
>
> What does 'content reference' mean, in particular, what does "content" mean
> in this context? Is it same as "browsing context" / docshells like the
> comment then seems to hint.

What I meant by a content reference was that web content has a reference to this docshell's browsing context, either through an iframe, or .opener. 

When I was meaning by "NonDependent" was referring to such a reference which would not be destroyed by the navigation. For example, if a document has an iframe inside of it, that iframe has a reference to the given document through .parent, but we don't need to worry about that reference because that iframe "depends" on the document in question to exist. 

Perhaps a better wording would be HasNonFrameContentVisibleReferences? I could also call it something like TabGroupHasOtherToplevelWindows() which also communicates the idea I would think.

The negation could be IsOnlyToplevelInTabGroup()?

I'm sure there is a better name for this method. I'd love some feedback on my explanation and ideas.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

oh, TabGroupHasOtherToplevelWindows() might be quite good one, and/or IsOnlyToplevelInTabGroup().

Comment 6

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

Attachment: Rename and simplify the DocShell ProcessLock concept

Updated version with the new name.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8823380 [details] [diff] [review]
Rename and simplify the DocShell ProcessLock concept

>+   * This value is `true` if its corresponding unit of related browsing contexts
>+   * (TabGroup) contains only 1 toplevel window, and that window is the outer
>+   * window corresponding to this docshell. This is the case when the current
>+   * document is loaded within an iframe, and when this window was either opened
>+   * by, or has opened another document with, window.open.
I don't understand the sentence starting "This is the case..."
Almost if you started to describe the 'false' case here. Please fix


>    *
>-   * NOTE: Some loads may not consider this a hard process lock, and may wish to
>-   * ignore this reason.
>+   * If this value is `false`, it would be content-visible for a load occuring
>+   * in this docshell to be performed within a different docshell.
perhaps 'web content visible' or so


>+  // Adopting an prerendered document is similar to performing a load within a
>+  // different docshell, as the prerendering must have occured in a different
occurred, I think

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a79d5d23b48f
Rename and simplify the DocShell ProcessLock concept, r=smaug

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Francesco Lodolo [:flod]]

Comment on attachment 8823380 [details] [diff] [review]
Rename and simplify the DocShell ProcessLock concept

Review of attachment 8823380 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +314,5 @@
>  LargeAllocationSuccess=This page was loaded in a new process due to a Large-Allocation header.
>  # LOCALIZATION NOTE: Do not translate "Large-Allocation", as it is a literal header name. Do not translate GET.
>  LargeAllocationNonGetRequest=A Large-Allocation header was ignored due to the load being triggered by a non-GET request.
> +# LOCALIZATION NOTE: Do not translate "Large-Allocation", as it is a literal header name. Do not translate `window.opener`.
> +LargeAllocationRelatedBrowsingContexts=A Large-Allocation header was ignored due to the presence of windows which have a reference to this browsing context through the frame hierarchy or window.opener.

Please don't change existing strings like this
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Will file a follow-up…