Closed
Bug 1397426
Opened 7 years ago
Closed 6 years ago
Make tab browser warming API not set the docshell to be active
Categories
(Firefox :: Tabbed Browser, enhancement, P1)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 59
People
(Reporter: mconley, Assigned: mconley)
References
Details
(Whiteboard: [reserve-photon-performance])
Attachments
(12 files, 3 obsolete files)
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
In bug 1385453, an API was added to warm up tabs that we think we're likely to switch to. That landed, and great results were seen in Telemetry. Unfortunately, it also caused a number of regressions. In particular, warming up background tabs caused those tabs to think they were in the foreground (because we were setting the docshell to be active), and that meant it'd do things like: 1) Fire the Page Visibility API stuff for the page JS so that the page thought it was visible (bug 1395735) 2) Unblock paused media - like YouTube videos that had been opened in the background (bug 1394455) In bug 1394455, I did some investigation and determined that warming up the tab without setting the docshell as active is possible, but would require some significant changes to TabChild and the async tab switcher (which is already really fragile). Because we were so close to the end of the cycle, I opted to disable tab warming instead, and have filed this bug to do the work necessary to re-enable it.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [reserve-photon-performance][triage]
Assignee | ||
Comment 1•7 years ago
|
||
In bug 1394455, I found that it was possible to cause a TabChild to paint and upload its layers without activating the DocShell. The steps to do this are very similar to the ones already in InternalSetDocShellIsActive, and will likely need to be factored out into one or more helper methods. Once that's done, I'll add some IPC messages, "renderLayers" and "clearLayers", which correspond to warming and unwarming a particular TabChild. I think that should be pretty straight forward. After that, the trick will be to modify the async tab switcher to use this new mechanism. I'm really loathe to add a new state to the tab states - I suspect that will break lots of things. What I think I want to do instead is similar to my original approach to bug 1385453; have a fixed-size collection of warming tabs. When a tab is warmed, we send the renderLayers message for that tab. When we exceed the maximum warming tabs threshold, we call clearLayers on the oldest warmed tab. When a tab is switched to, we remove it from the warming collection. If a tab goes away, we remove it from the warming collection. If we do it this way, the logic for activating and deactivating the docShells can stay the same. What do you think of this vague plan, billm? Or should I try adding some kind of STATE_WARMING and STATE_UNWARMING instead? While I wait to hear back (since billm is on PTO until the 18th), I'll get the native layer patch written up.
Flags: needinfo?(wmccloskey)
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: [reserve-photon-performance][triage] → [reserve-photon-performance]
I guess I was hoping that the tab switcher would only care about the renderLayers/clearLayers stuff. That is, we could just substitute renderLayers/clearLayers for the places where we set docShellIsActive now. Actually setting docShellIsActive could be done as kind of an afterthought, somewhere around here: http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/base/content/tabbrowser.xml#4558
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 3•7 years ago
|
||
I love it - that's how I'll roll. Thanks!
Updated•7 years ago
|
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: P3 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8911917 -
Flags: review?(wmccloskey)
Attachment #8911918 -
Flags: review?(wmccloskey)
Attachment #8911919 -
Flags: review?(wmccloskey)
Attachment #8911920 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 8•7 years ago
|
||
My try push shows these patches crashing all over the place: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f4872d73bc9115ff50d7ed0b5a46433e21d707e Clearly not ready for review yet.
Updated•7 years ago
|
Flags: qe-verify? → qe-verify-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
It turns out that the crasher I was dealing with (and have a patch for in this stack) has been fixed by bevis in bug 1399707, so I'll remove that patch from this collection now.
Depends on: 1399707
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8913431 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8913432 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8911917 [details] Bug 1397426 - Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell. https://reviewboard.mozilla.org/r/183306/#review197416 ::: dom/ipc/TabChild.cpp:2600 (Diff revision 6) > + if (aIsActive) { > + Unused << RecvRenderLayers(true, mLayerObserverEpoch); > + } else if (!aPreserveLayers) { > + Unused << RecvRenderLayers(false, mLayerObserverEpoch); > + } I don't actually think this is necessary anymore since TabParent::SetDocShellIsActive is calling RenderLayers before sending the SetDocShellIsActive message.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8911917 [details] Bug 1397426 - Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell. https://reviewboard.mozilla.org/r/183306/#review200266 Looks good. Thanks. ::: dom/ipc/PBrowser.ipdl:788 (Diff revision 7) > - * Update the child side docShell active (resource use) state. > + * Update the child side docShell active (resource use) state. If layers > + * aren't already being rendered for this child via RenderLayers, they > + * will be. Similarly, if a docShell is made inactive and layer rendering > + * hasn't already been disabled, this will then disable them (unless > + * aPreserveLayers is set to true). This comment isn't really true. Sending the SetDocShellIsActive message only changes the active state, not the visible state. The TabParent takes care of setting them both, but that's not really relevant in this comment. ::: dom/ipc/TabChild.cpp:2577 (Diff revision 7) > mPendingDocShellPreserveLayers); > } > } > > void > TabChild::InternalSetDocShellIsActive(bool aIsActive, bool aPreserveLayers) Seems like aPreserveLayers is no longer used here. ::: dom/ipc/TabChild.cpp:2587 (Diff revision 7) > + if (aIsActive) { > + if (!sActiveTabs) { > + sActiveTabs = new nsTHashtable<nsPtrHashKey<TabChild>>(); > + } > + sActiveTabs->PutEntry(this); > + } else { > + if (sActiveTabs) { > + sActiveTabs->RemoveEntry(this); > + // We don't delete sActiveTabs here when it's empty since that > + // could cause a lot of churn. Instead, we wait until ~TabChild. > + } > + } This is going to break Quantum DOM, which is the only user of this data structure. It really wants to keep track of the visible tabs rather than the active tabs. Can you rename this (to sVisibleTabs or something) and move the tracking to RenderLayers? ::: dom/ipc/TabChild.cpp:2603 (Diff revision 7) > + } > +} > + > +mozilla::ipc::IPCResult > +TabChild::RecvSetDocShellIsActive(const bool& aIsActive, > + const bool& aPreserveLayers) It looks like aPreserveLayers isn't used anywhere in the child. Can we remove it and just track that state in the parent? ::: dom/ipc/TabParent.cpp:2956 (Diff revision 7) > + > + mRenderingLayers = aEnabled; > + > + // Preserve layers means that attempts to stop rendering layers > + // will be ignored. > + if (!(!aEnabled && mPreserveLayers)) { Suppose we call RenderLayers(false) when mPreserveLayers is true. Then we call RenderLayers(true). The last call will still ask the child to paint and wait to display the tab until it gets the ForcePaintNoOp message. Could we instead just not send anything and act as if the layers are present? I think you would get that effect if you avoided setting mRenderLayers to false in the RenderLayers(false) call. So maybe you could just move this condition up to the |if (aEnabled == mRenderLayers)| condition? That would also allow you to avoid the double negation, which is a tad confusing.
Attachment #8911917 -
Flags: review?(wmccloskey) → review+
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8911919 [details] Bug 1397426 - Fix block comment formatting in nsITabParent.idl. https://reviewboard.mozilla.org/r/183310/#review200808
Attachment #8911919 -
Flags: review?(wmccloskey) → review+
Comment 47•7 years ago
|
||
mozreview-review |
Comment on attachment 8911920 [details] Bug 1397426 - Stop disabling tab warming in browser_bug343515.js. https://reviewboard.mozilla.org/r/183312/#review200810
Attachment #8911920 -
Flags: review?(wmccloskey) → review+
Comment 48•7 years ago
|
||
mozreview-review |
Comment on attachment 8911918 [details] Bug 1397426 - Make async tab switcher use new nsITabParent renderLayers function. https://reviewboard.mozilla.org/r/183308/#review200812 ::: browser/base/content/tabbrowser.xml:4628 (Diff revision 9) > + // If we've reached the point where the requested tab has entered > + // the loaded state, but the DocShell is still not yet active, we > + // should activate it. > + let canCheckDocShellState = !requestedBrowser.isRemoteBrowser || > + requestedBrowser.frameLoader.tabParent; > + > + if (requestedTabState == this.STATE_LOADED && > + canCheckDocShellState && > + !requestedBrowser.docShellIsActive && > + !this.minimizedOrFullyOccluded) { > + requestedBrowser.docShellIsActive = true; > + this.logState("Set requested tab docshell to active"); > + } I feel like this should go inside the |if (showTab === this.requestedTab) {| branch (after line 4617). I don't think we should run this code if we're not switching to requestedTab.
Attachment #8911918 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 49•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8911917 [details] Bug 1397426 - Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell. https://reviewboard.mozilla.org/r/183306/#review200266 > This is going to break Quantum DOM, which is the only user of this data structure. It really wants to keep track of the visible tabs rather than the active tabs. Can you rename this (to sVisibleTabs or something) and move the tracking to RenderLayers? Are you sure we want to do that? At least reading this comment, it seems like GetActiveTabs is supposed to return the tabs that are considered to be "in the foreground". Tabs that are warming shouldn't, I imagine, be included in that set. Or am I misunderstanding?
Assignee | ||
Comment 50•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8911918 [details] Bug 1397426 - Make async tab switcher use new nsITabParent renderLayers function. https://reviewboard.mozilla.org/r/183308/#review200812 > I feel like this should go inside the |if (showTab === this.requestedTab) {| branch (after line 4617). I don't think we should run this code if we're not switching to requestedTab. That's where I put it originally, but I think there's a problem there - the "blank" case, where the `showTab` becomes the `this.requestedTab` despite us not having gotten layers for it yet. As I understand it, we have to set the docShell to be active once the layers have finally arrived and the switch has more or less completed. Without that, I was getting some pretty gnarly test failures. Or am I misunderstanding?
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #49) > Comment on attachment 8911917 [details] > Bug 1397426 - Add IPC interface to tell TabChild's to render and clear > layers, distinct from setting the active state on the DocShell. > > https://reviewboard.mozilla.org/r/183306/#review200266 > > > This is going to break Quantum DOM, which is the only user of this data structure. It really wants to keep track of the visible tabs rather than the active tabs. Can you rename this (to sVisibleTabs or something) and move the tracking to RenderLayers? > > Are you sure we want to do that? At least reading this comment, it seems > like GetActiveTabs is supposed to return the tabs that are considered to be > "in the foreground". Tabs that are warming shouldn't, I imagine, be included > in that set. > > Or am I misunderstanding? Yeah, GetActiveTabs should probably be renamed to GetVisibleTabs. It's my fault that the two things got conflated. The only use of GetActiveTabs is to label the vsync message: http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/ipc/glue/BackgroundChildImpl.cpp#653 Vsync runs the refresh driver, which processes any tab whose PresShell is active. So the visible tabs need to be on that list.
Flags: needinfo?(wmccloskey)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #50) > Comment on attachment 8911918 [details] > Bug 1397426 - Make async tab switcher use new nsITabParent renderLayers > function. > > https://reviewboard.mozilla.org/r/183308/#review200812 > > > I feel like this should go inside the |if (showTab === this.requestedTab) {| branch (after line 4617). I don't think we should run this code if we're not switching to requestedTab. > > That's where I put it originally, but I think there's a problem there - the > "blank" case, where the `showTab` becomes the `this.requestedTab` despite us > not having gotten layers for it yet. > > As I understand it, we have to set the docShell to be active once the layers > have finally arrived and the switch has more or less completed. Without > that, I was getting some pretty gnarly test failures. > > Or am I misunderstanding? If we moved the code, wouldn't we still set the docshell to active in the blank case? It seems like all the conditions on those branches would still be true. I guess it just seems weird that your code is making requestedTab be the active docshell even though it's not clear that showTab == this.requestedTab. If they're not the same, why should we set requestedTab to active yet?
Assignee | ||
Comment 54•7 years ago
|
||
Quick update here - I've been applying the feedback, and have been dealing with some test fallout. This stuff is really touchy, I guess. :/ Hopefully I'll have a new set of patches up today.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 68•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review203260 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/ipc/TabParent.cpp:2962 (Diff revision 1) > + { > + MOZ_ASSERT(NS_IsMainThread()); > + } > + > +private: > + NS_IMETHOD Run() override { Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment 69•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review203262 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/ipc/TabParent.cpp:2963 (Diff revision 2) > + { > + MOZ_ASSERT(NS_IsMainThread()); > + } > + > +private: > + NS_IMETHOD Run() override { Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Assignee | ||
Comment 70•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review203260 > Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override] I'm not super sure what clangbot wants me to do here. The `virtual` is coming from the `NS_IMETHOD` macro, and I think I'd rather use the macro than hand-roll the definition of `NS_IMETHOD` without `virtual` for this single usage. So, uh, sorry clangbot - I think I'm going to drop this (unless this is going to burn me on landing somehow).
Comment 71•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review203260 > I'm not super sure what clangbot wants me to do here. The `virtual` is coming from the `NS_IMETHOD` macro, and I think I'd rather use the macro than hand-roll the definition of `NS_IMETHOD` without `virtual` for this single usage. > > So, uh, sorry clangbot - I think I'm going to drop this (unless this is going to burn me on landing somehow). Apologies for this unhelpful warning from clangbot, dropping it was the right call. (In general, clangbot warnings can be safely ignored, and won't burn you on landing in anyway. Unfortunately, these warnings will get reported again for each patch revision, so you may have to drop them multiple times. Sorry about that!) More in this issue: We've decided to silence defects found inside code that is expanded from macros (because they're usually quite noisy and not helpful, especially when the macro is in third-party code). However, for this particular issue, clang-tidy doesn't give us any indication that the redundant keyword comes from a macro, so clangbot reports it anyway. Given that the clang-tidy rule "modernize-use-override" causes these problems, and that it's unclear if having it enabled brings any value to our code base, I think we should consider disabling the rule entirely.
Comment 72•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review203882 ::: dom/ipc/TabParent.cpp:2975 (Diff revision 2) > + // If RenderLayers has been called, and the requested > + // rendering state already exists, we don't have to ask > + // the TabChild to do anything. Instead, we short-circuit, > + // and queue the layer update events for the caller, since > + // they wouldn't normally be coming on their own. > + RefPtr<LayerTreeUpdateRunnable> runnable = > + new LayerTreeUpdateRunnable(this, mLayerTreeEpoch, aEnabled); > + NS_DispatchToMainThread(runnable); Can you explain why this is needed? It makes me worried that the state tracking in tabbrowser.xml is getting screwed up. If that happens, we can end up with races. I'd rather fix the tabbrowser code than add extra checks here.
Comment 73•6 years ago
|
||
mozreview-review |
Comment on attachment 8926449 [details] Bug 1397426 - Rename TabChild's notion of "active tabs" to "visible tabs" and move logic into renderLayers. https://reviewboard.mozilla.org/r/197690/#review203884
Attachment #8926449 -
Flags: review?(wmccloskey) → review+
Comment 74•6 years ago
|
||
mozreview-review |
Comment on attachment 8926450 [details] Bug 1397426 - Activate docShells immediately for imageDocument pages when switching tabs with warming enabled. https://reviewboard.mozilla.org/r/197692/#review203886 I don't get this one either. I'm sorry I didn't ask earlier on IRC. But it's better to have this information in bugs anyway.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 82•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review205656 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/ipc/TabParent.cpp:2963 (Diff revision 3) > + { > + MOZ_ASSERT(NS_IsMainThread()); > + } > + > +private: > + NS_IMETHOD Run() override { Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment hidden (mozreview-request) |
Comment 84•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review206056 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/ipc/TabParent.cpp:2963 (Diff revision 3) > + { > + MOZ_ASSERT(NS_IsMainThread()); > + } > + > +private: > + NS_IMETHOD Run() override { Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Assignee | ||
Comment 85•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review203882 > Can you explain why this is needed? It makes me worried that the state tracking in tabbrowser.xml is getting screwed up. If that happens, we can end up with races. I'd rather fix the tabbrowser code than add extra checks here. We talked about this IRL - basically, there are cases when the selected tab's docshell is inactive, but preserving layers when we're either minimized or fully occluded. When tab switches are initialized in that state, and we set STATE_LOADING on the tab in question, and layers are already available in the compositor, we need the MozLayerTreeReady event to fire again so that the front-end knows to enter STATE_LOADED. I ended up taking billm's advice here, and only firing this extra MozLayerTreeReady iff we're preserving layers.
Comment hidden (mozreview-request) |
Comment 87•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review206110 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/ipc/TabParent.cpp:2963 (Diff revision 3) > + { > + MOZ_ASSERT(NS_IsMainThread()); > + } > + > +private: > + NS_IMETHOD Run() override { Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment 88•6 years ago
|
||
mozreview-review |
Comment on attachment 8929538 [details] Bug 1397426 - Add hasLayers to nsITabParent and use it in the async tab switcher. https://reviewboard.mozilla.org/r/200814/#review206478 This looks fine, but I don't think I've reviewed anything that defines renderingLayers. Is that coming up? ::: dom/interfaces/base/nsITabParent.idl:39 (Diff revision 2) > > /** > + * True if layers are being rendered and the compositor has reported > + * receiving them. > + */ > + readonly attribute boolean renderingLayers; I think hasLayers would be a better name here. "rendering" makes it sound like the "renderLayers" call has started and not yet finished.
Attachment #8929538 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 89•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8929538 [details] Bug 1397426 - Add hasLayers to nsITabParent and use it in the async tab switcher. https://reviewboard.mozilla.org/r/200814/#review206478 renderingLayers was defined in the second patch in the series - the one with the commit message, "Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r?billm", which you haven't reviewed yet. > I think hasLayers would be a better name here. "rendering" makes it sound like the "renderLayers" call has started and not yet finished. Good idea.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8926450 -
Attachment is obsolete: true
Comment 101•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review207522 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/ipc/TabParent.cpp:2963 (Diff revision 4) > + { > + MOZ_ASSERT(NS_IsMainThread()); > + } > + > +private: > + NS_IMETHOD Run() override { Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment 103•6 years ago
|
||
mozreview-review |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review209958 ::: dom/ipc/TabParent.cpp:2979 (Diff revision 4) > + RefPtr<LayerTreeUpdateRunnable> runnable = > + new LayerTreeUpdateRunnable(this, mLayerTreeEpoch, aEnabled); It might be more concise to do this using a C++ lambda (and then delete LayerTreeUpdateRunnable entirely). Up to you though. Just make sure to keep the TabParent in a RefPtr if you do that though.
Attachment #8926448 -
Flags: review?(wmccloskey) → review+
Comment 104•6 years ago
|
||
mozreview-review |
Comment on attachment 8931049 [details] Bug 1397426 - Rename mRenderingLayers to mRenderLayers. https://reviewboard.mozilla.org/r/202144/#review209960
Attachment #8931049 -
Flags: review?(wmccloskey) → review+
Comment 105•6 years ago
|
||
mozreview-review |
Comment on attachment 8931051 [details] Bug 1397426 - TabChild::MakeHidden shouldn't cause script to run. https://reviewboard.mozilla.org/r/202148/#review209968
Attachment #8931051 -
Flags: review?(wmccloskey) → review+
Comment 106•6 years ago
|
||
mozreview-review |
Comment on attachment 8931052 [details] Bug 1397426 - Expose renderLayers state via nsITabParent and lazily retrieve initial tab states in async tab switcher. https://reviewboard.mozilla.org/r/202150/#review209972 ::: browser/base/content/tabbrowser.xml:4480 (Diff revision 1) > - let tabIsLoaded = browser.renderingLayers; > + for (let tab of this.tabbrowser.tabs) { > + if (tab == initialTab && this.minimizedOrFullyOccluded) { > + continue; > + } > > - if (!this.minimizedOrFullyOccluded) { > + if (tab.linkedPanel) { Why are you checking linkedPanel? I don't know what that is.
Attachment #8931052 -
Flags: review?(wmccloskey) → review+
Comment 107•6 years ago
|
||
mozreview-review |
Comment on attachment 8931050 [details] Bug 1397426 - Make TabParent's assume they're rendering layers by default on construction. https://reviewboard.mozilla.org/r/202146/#review209976
Attachment #8931050 -
Flags: review?(wmccloskey) → review+
Sorry, I've been on PTO this week.
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 109•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8931052 [details] Bug 1397426 - Expose renderLayers state via nsITabParent and lazily retrieve initial tab states in async tab switcher. https://reviewboard.mozilla.org/r/202150/#review209972 > Why are you checking linkedPanel? I don't know what that is. We have this thing where tabs can be "discarded", where there's actually no <xul:browser> linked to them yet, and they're linked lazily. That's how restore-on-demand works these days (with great start-up performance gains). We've also exposed an API to WebExtensions for discarding tabs that [people are starting to play with](https://addons.mozilla.org/en-US/firefox/addon/dormancy/). Accessing the browser immediately causes the browser to be instantiated and inserted into the DOM. The current way to detect the presence of a browser already being in the DOM is to check the true-ness of `linkedPanel`. So what I'm doing here is avoiding instantiating lazy browsers unnecessarily.
Assignee | ||
Comment 110•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926448 [details] Bug 1397426 - When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. https://reviewboard.mozilla.org/r/197688/#review209958 > It might be more concise to do this using a C++ lambda (and then delete LayerTreeUpdateRunnable entirely). Up to you though. Just make sure to keep the TabParent in a RefPtr if you do that though. Good idea, thanks.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 122•6 years ago
|
||
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/261be8ec0554 Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell. r=billm https://hg.mozilla.org/integration/autoland/rev/2cd697170fb3 Rename mRenderingLayers to mRenderLayers. r=billm https://hg.mozilla.org/integration/autoland/rev/4dd20bf8c0f6 When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r=billm https://hg.mozilla.org/integration/autoland/rev/4643e46ff8d3 Rename TabChild's notion of "active tabs" to "visible tabs" and move logic into renderLayers. r=billm https://hg.mozilla.org/integration/autoland/rev/a5b279d39c01 Make async tab switcher use new nsITabParent renderLayers function. r=billm https://hg.mozilla.org/integration/autoland/rev/5580c145af58 Fix block comment formatting in nsITabParent.idl. r=billm https://hg.mozilla.org/integration/autoland/rev/e7a6fe9f9ce7 Stop disabling tab warming in browser_bug343515.js. r=billm https://hg.mozilla.org/integration/autoland/rev/53bb2bc2b676 Add hasLayers to nsITabParent and use it in the async tab switcher. r=billm https://hg.mozilla.org/integration/autoland/rev/b9b2895b11a3 Make TabParent's assume they're rendering layers by default on construction. r=billm https://hg.mozilla.org/integration/autoland/rev/78073667ddc6 TabChild::MakeHidden shouldn't cause script to run. r=billm https://hg.mozilla.org/integration/autoland/rev/8994162ee112 Expose renderLayers state via nsITabParent and correctly set initial tab states in async tab switcher. r=billm
Comment 123•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/261be8ec0554 https://hg.mozilla.org/mozilla-central/rev/2cd697170fb3 https://hg.mozilla.org/mozilla-central/rev/4dd20bf8c0f6 https://hg.mozilla.org/mozilla-central/rev/4643e46ff8d3 https://hg.mozilla.org/mozilla-central/rev/a5b279d39c01 https://hg.mozilla.org/mozilla-central/rev/5580c145af58 https://hg.mozilla.org/mozilla-central/rev/e7a6fe9f9ce7 https://hg.mozilla.org/mozilla-central/rev/53bb2bc2b676 https://hg.mozilla.org/mozilla-central/rev/b9b2895b11a3 https://hg.mozilla.org/mozilla-central/rev/78073667ddc6 https://hg.mozilla.org/mozilla-central/rev/8994162ee112
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Assignee | ||
Comment 124•6 years ago
|
||
I'm hearing enough reports about permanent tab spinners that I'm pretty much ready to back this out. I'm going to give myself another hour or so to figure it out, and then I'll get a backout happening.
Assignee | ||
Comment 125•6 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe85d516c0f https://hg.mozilla.org/integration/mozilla-inbound/rev/3a037cd20cec https://hg.mozilla.org/integration/mozilla-inbound/rev/27eb802dd2ed https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8441396122 https://hg.mozilla.org/integration/mozilla-inbound/rev/35f218c01175 https://hg.mozilla.org/integration/mozilla-inbound/rev/90dfea411080 https://hg.mozilla.org/integration/mozilla-inbound/rev/94528f4b8ff9 https://hg.mozilla.org/integration/mozilla-inbound/rev/6b919b7a1161 https://hg.mozilla.org/integration/mozilla-inbound/rev/15b69b6abbf8 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9967b4da458 https://hg.mozilla.org/integration/mozilla-inbound/rev/fd75899e1f87
Updated•6 years ago
|
Comment 126•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd75899e1f87 https://hg.mozilla.org/mozilla-central/rev/a9967b4da458 https://hg.mozilla.org/mozilla-central/rev/15b69b6abbf8 https://hg.mozilla.org/mozilla-central/rev/6b919b7a1161 https://hg.mozilla.org/mozilla-central/rev/94528f4b8ff9 https://hg.mozilla.org/mozilla-central/rev/90dfea411080 https://hg.mozilla.org/mozilla-central/rev/35f218c01175 https://hg.mozilla.org/mozilla-central/rev/ba8441396122 https://hg.mozilla.org/mozilla-central/rev/27eb802dd2ed https://hg.mozilla.org/mozilla-central/rev/3a037cd20cec https://hg.mozilla.org/mozilla-central/rev/ffe85d516c0f
Target Milestone: Firefox 59 → ---
Assignee | ||
Comment 127•6 years ago
|
||
Just a heads up for the people who are watching, I think I've more or less sorted the intermittent perma-spinners that folks were suffering from due to these patches. Some silly errors on my part, which I'll enumerate when I post a new set of patches. I'm currently in the process of trying to address the tps Talos regressions that these patches also introduced (bug 1422685).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 139•6 years ago
|
||
These latest patches _should_ address the perma spinners people were experiencing, and should avoid the tps regression (bug 1422685). A few minor changes: 1) In "When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r=billm", I goofed when applying review feedback, and accidentally cast a uint64_t into a bool when passing some variables to a lambda function. This greatly confused things, and has been fixed. 2) I also got rid of code handling an impossible case where we'd short-circuit and fire MozLayerTreeCleared if we're preserving layers, and unload layers (which can never happen if we're preserving). That has simplified things a bit. 3) I put back the code that made sure that the initial tab had its layers preservation set to false. This was accidentally removed during a refactor. 4) Instead of initializing all of the tab states in the async tab switcher init() function, I do it lazily from within getTabState. This seems to have eliminated the tps regression. Now pushing to try.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 153•6 years ago
|
||
mozreview-review |
Comment on attachment 8939692 [details] Bug 1397426 - Ensure we attempt to activate docShell of current tab after reacting to sidemode or occlusionstate change events. https://reviewboard.mozilla.org/r/209986/#review216364
Attachment #8939692 -
Flags: review?(bill.mccloskey) → review+
Comment 154•6 years ago
|
||
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b0c38584a789 Add IPC interface to tell TabChild's to render and clear layers, distinct from setting the active state on the DocShell. r=billm https://hg.mozilla.org/integration/autoland/rev/7dc566272fee Rename mRenderingLayers to mRenderLayers. r=billm https://hg.mozilla.org/integration/autoland/rev/4dd5d1624aba When short-circuiting a TabParent::RenderLayers call, still fire the layer tree event if we've been preserving layers. r=billm https://hg.mozilla.org/integration/autoland/rev/f77d887fffcb Rename TabChild's notion of "active tabs" to "visible tabs" and move logic into renderLayers. r=billm https://hg.mozilla.org/integration/autoland/rev/44d6efb9fa69 Make async tab switcher use new nsITabParent renderLayers function. r=billm https://hg.mozilla.org/integration/autoland/rev/d39683d8f79f Fix block comment formatting in nsITabParent.idl. r=billm https://hg.mozilla.org/integration/autoland/rev/f6e07e976045 Stop disabling tab warming in browser_bug343515.js. r=billm https://hg.mozilla.org/integration/autoland/rev/3fa35817d29d Add hasLayers to nsITabParent and use it in the async tab switcher. r=billm https://hg.mozilla.org/integration/autoland/rev/707a2b0aede1 Make TabParent's assume they're rendering layers by default on construction. r=billm https://hg.mozilla.org/integration/autoland/rev/8605e2ebcc92 TabChild::MakeHidden shouldn't cause script to run. r=billm https://hg.mozilla.org/integration/autoland/rev/c592bf74cad1 Expose renderLayers state via nsITabParent and lazily retrieve initial tab states in async tab switcher. r=billm https://hg.mozilla.org/integration/autoland/rev/2cfdaedf6359 Ensure we attempt to activate docShell of current tab after reacting to sidemode or occlusionstate change events. r=billm
Comment 155•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b0c38584a789 https://hg.mozilla.org/mozilla-central/rev/7dc566272fee https://hg.mozilla.org/mozilla-central/rev/4dd5d1624aba https://hg.mozilla.org/mozilla-central/rev/f77d887fffcb https://hg.mozilla.org/mozilla-central/rev/44d6efb9fa69 https://hg.mozilla.org/mozilla-central/rev/d39683d8f79f https://hg.mozilla.org/mozilla-central/rev/f6e07e976045 https://hg.mozilla.org/mozilla-central/rev/3fa35817d29d https://hg.mozilla.org/mozilla-central/rev/707a2b0aede1 https://hg.mozilla.org/mozilla-central/rev/8605e2ebcc92 https://hg.mozilla.org/mozilla-central/rev/c592bf74cad1 https://hg.mozilla.org/mozilla-central/rev/2cfdaedf6359
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Comment 156•6 years ago
|
||
sorry to be a downer, but I am getting the infinite spinning wheel on tabs.
Flags: needinfo?(mconley)
Updated•6 years ago
|
Flags: needinfo?(mconley)
You need to log in
before you can comment on or make changes to this bug.
Description
•