If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[e10s] Scrollbar missing when e10s enabled

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Untriaged
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bkerensa, Assigned: mconley)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm6+, firefox40 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Scrollbar does not show up when e10s enabled but shows up fine when disabled.

Build: 	20141109030205
this works for me. Got a screenshot? what platform? any addons?
Flags: needinfo?(bkerensa)
(Reporter)

Comment 2

3 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> this works for me. Got a screenshot? what platform? any addons?

Hi Brad,

Here is the behavior with e10s on http://youtu.be/8dWzdp2omGQ this is with LastPass, Awesome Screenshot and BugzillaJS. I also disabled all addons and the behavior persisted and also tested in a new profile.
Flags: needinfo?(bkerensa)
(In reply to Benjamin Kerensa [:bkerensa] from comment #2)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> > this works for me. Got a screenshot? what platform? any addons?
> 
> Hi Brad,
> 
> Here is the behavior with e10s on http://youtu.be/8dWzdp2omGQ this is with
> LastPass, Awesome Screenshot and BugzillaJS. I also disabled all addons and
> the behavior persisted and also tested in a new profile.

Hi Benjamin, there are known problems with LastPass. Can you try with that disabled? If it still persists please try with the other two disabled as well.
Flags: needinfo?(bkerensa)
Duplicate of this bug: 1097143
Duplicate of this bug: 1097601
From bug 1097601 - I have this whenever I'm using session restore which is all the time on my main profile as I've got tab groups.

It happens on all sites. Whereas opening in non-e10s is fine.

I've also replicated this with not using tab groups, but using the History -> Restore Previous Session option.

I'm using MacBook Pro 10.8 with a retina display on the laptop, and a non-retina external display.

I noticed something this morning: I happened to be on just the laptop display, and the scrollbars were there. Now I've switched back to my normal setup I've lost them again (it took a couple of restarts though). My normal setup has the non-retina display as the "master" showing the app bar and menu bar.
(Reporter)

Comment 7

3 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3)
> (In reply to Benjamin Kerensa [:bkerensa] from comment #2)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> > > this works for me. Got a screenshot? what platform? any addons?
> > 
> > Hi Brad,
> > 
> > Here is the behavior with e10s on http://youtu.be/8dWzdp2omGQ this is with
> > LastPass, Awesome Screenshot and BugzillaJS. I also disabled all addons and
> > the behavior persisted and also tested in a new profile.
> 
> Hi Benjamin, there are known problems with LastPass. Can you try with that
> disabled? If it still persists please try with the other two disabled as
> well.

I have tried it with all addons disabled and e10s on gives me no scrollbars and e10s off acts normal.
Flags: needinfo?(bkerensa)
Benjamin & Mark, do you use an external display?
Flags: needinfo?(standard8)
Flags: needinfo?(bkerensa)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> Benjamin & Mark, do you use an external display?

I've seen this with both internal and external displays
(In reply to Dave Townsend [:mossop] from comment #9)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> > Benjamin & Mark, do you use an external display?
> 
> I've seen this with both internal and external displays

Have you seen it when you haven't had an external display plugged in during the lifetime of Firefox?

To clarify, I haven't seen this. I don't use an external display, so I'm wondering if that's a key part of reproducing this.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> (In reply to Dave Townsend [:mossop] from comment #9)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> > > Benjamin & Mark, do you use an external display?
> > 
> > I've seen this with both internal and external displays
> 
> Have you seen it when you haven't had an external display plugged in during
> the lifetime of Firefox?

Definitely, I haven't connected to an external display since leaving home on Saturday and I've restarted Firefox multiple times since then.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> Mark, do you use an external display?

Yes, I'm on an external display which is non-retina and hooked up via the thunderbolt port, via hdmi.
Flags: needinfo?(standard8)
(Reporter)

Comment 13

3 years ago
I am on a internal display when I run into this
Flags: needinfo?(bkerensa)
Assignee: nobody → mconley
tracking-e10s: --- → m7+
Summary: [e10s] Scrollbar missing when enabled → [e10s] Scrollbar missing when e10s enabled
I think this really needs to block the aurora uplift
tracking-e10s: m7+ → ?
tracking-e10s: ? → m6+
Starting to investigate this.
Status: NEW → ASSIGNED
What I would absolutely love is for some reliable STR for this bug to aid in my analysis. Anybody got some? bkerensa - are you still able to reproduce this reliably?
Flags: needinfo?(bkerensa)
(Reporter)

Comment 17

3 years ago
Hi Mike,

STR is pretty simple just enabling e10s on OSX on any nightly for me totally makes scrollbars stop working. Would you like a video? There is no step to reproduce aside from enabling e10s and them immediately not working once the browser restarts.

If I turn them off the scrolls bars are back like magic :)
Flags: needinfo?(bkerensa) → needinfo?(mconley)
Can you tell me more about your setup? Specifically:

1) What version of OS X you're running
2) What your setting is in System Preferences > General, "Show scroll bars"
3) Whether or not you have a mouse plugged in when you see this issue
4) Whether or not you can reproduce this with a new profile

Thanks!
Flags: needinfo?(mconley) → needinfo?(bkerensa)
(Reporter)

Comment 19

3 years ago
10.10.3 (also happened in 10.10)
Automatically based on mouse or trackpad (OSX Default)
Mouse is plugged in (Unplugged mouse still happens with trackpad)
Can reproduce with fresh profile
Flags: needinfo?(bkerensa)
Still no luck reproducing. :(

:tracy, got any cycles to help me figure out what I need to do to see this?
Flags: needinfo?(twalker)
I've tried toggling various scroll settings in both Nightly and the System prefs, Session restoring, using external or laptop screen.  I am not having any success reproducing this either.  Something non-obvious is in play.
Flags: needinfo?(twalker)
allowed today's nightly to update and restart caused scrolls bars to disappear for me this morning.

Here are the JS warnings, errors and logs

flags argument of String.prototype.{search,match,replace} is deprecated d3f13b20b98c56fc4227aea92752bc90.js:1681:15
mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create browser.js:5292:2
TypeError: this.controller is undefined browserPlacesViews.js:258:0
uncaught exception: 2147942487 <unknown>
OpenGL compositor Initialized Succesfully.
Version: 2.1 NVIDIA-8.26.28 310.40.55b01
Vendor: NVIDIA Corporation
Renderer: NVIDIA GeForce GT 750M OpenGL Engine
FBO Texture Target: TEXTURE_2D
uncaught exception: 2147942487 <unknown>
OpenGL compositor Initialized Succesfully.
Version: 2.1 NVIDIA-8.26.28 310.40.55b01
Vendor: NVIDIA Corporation
Renderer: NVIDIA GeForce GT 750M OpenGL Engine
FBO Texture Target: TEXTURE_2D
1428419585176	FirefoxAccounts	ERROR	FxA rejecting with error NO_ACCOUNT, details: undefined Log.jsm:749:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
unsafe CPOW usage BrowserUtils.jsm:115:0
unsafe CPOW usage findbar.xml:740:0
unsafe CPOW usage findbar.xml:743:0
unsafe CPOW usage findbar.xml:746:0
Created attachment 8589141 [details]
restart.js

I was finally able to reproduce this. My STR (likely not minimal, but it's a start):

0) Make sure Firefox is set up to restore your tabs / windows from last time when starting up
1) Open a single tab in Firefox that is long (requires a vertical scrollbar). The root document needs to be long - no CSS trickery (so a dxr page is no good. This Bugzilla bug is probably a good page to load, actually).
2) Make sure devtools.chrome.enabled and devtools.debugger.remote-enabled are set to true
3) Open up the Scratchpad from DevTools, and in the titlebar, set Environment to Browser
4) Paste the script I've attached to this bug ("restart.js") into the Scratchpad, and click "Run"

The browser should restart and load the test page. Every now and then, the vertical scrollbar is missing.
Attachment #8589141 - Attachment mime type: application/force-download → text/plain
Ok, I have more minimal STR:

0) Make sure Firefox is set up to restore your tabs / windows from last time when starting up and is using e10s.
1) Make sure a mouse is plugged in, and make sure that OS X is configured to show scrollbars "Automatically based on mouse or trackpad" (that's in System Preferences > General).
2) Open this bug in one tab, and then again in a second tab.
3) Quit the browser
4) Start the browser
5) Note that the initial re-opened browser has overlay scrollbars instead of the inline scrollbars.
6) Switch to the other tab - notice that the scrollbar track is just a blank area.
Maybe it's helpful or not, but if you restore session from about:home this bug does not reproduce.  What is it doing that's different from update or on restart session restore?
(In reply to [:tracy] Tracy Walker - QA Mentor from comment #25)
> Maybe it's helpful or not, but if you restore session from about:home this
> bug does not reproduce.  What is it doing that's different from update or on
> restart session restore?

I suspect the flurry of remote browsers being restored on a restart is contributing to a race that we're less likely to hit after start-up has completed. I also suspect that if about:home was a tall page and had a scrollbar, we'd hit this again. I will actually try that experiment.
So I've done a bit of analysis, and here are my findings:

Suppose we're starting Firefox and it automatically restores a session where the selected browser has a vertical scrollbar.

In single-process Firefox on post-Lion OS X, we don't initially draw the scrollbar, because we assume that we're using overlay scrollbars. We're constantly polling this nsLookAndFeel metric for overlay scrollbars and it's returning "true".

OS X has noticed that we're asking about the scrollbars, and after it's done an analysis of the peripherals and user preferences, it tells us "Actually, we're supposed to use permanent scrollbars here". nsChildView.mm hears that via the NSPreferredScrollerStyleDidChangeNotification that gets handled by ChildView's scrollbarSystemMetricChanged method. This causes nsPresContext to realize that the theme has changed, and it flushes all caches of the look and feel, and recomputes the layout of the page to draw the scrollbars correctly.

In the multi-process case, the look-and-feel metric changes, but the child never hears about the scrollbarSystemMetricChanged event, because the content process doesn't have a ChildView. So the nsPresContext never gets ThemeChanged called on it, and we don't re-lay out the page.

So that's the problem. I'm evaluating solutions now.
Created attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - [WIP] Cache overlay scrollbar LookAndFeel metric, and invalidate in the content process when parent sends Refresh message. r=?

Pull down this commit:

hg pull -r 58f48da582c157e3a25d53dc194840afc31d15cf https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Cache overlay scrollbar LookAndFeel metric, and allow parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?

Pull down these commits:

hg pull -r 078699a501806305a33d51617e71527be08c8dd6 https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/7305/#review6069

::: dom/ipc/PBrowser.ipdl:676
(Diff revision 2)
> +    ThemeChanged();

Add documentation.

::: layout/base/nsPresContext.cpp:1745
(Diff revision 2)
> +  // Recursively notify all remote leaf descendants that the
> +  // resolution of the user interface has changed.

This comment is wrong.

::: widget/LookAndFeel.h:589
(Diff revision 2)
> +   * TODO

TODO.

::: widget/nsXPLookAndFeel.cpp:709
(Diff revision 2)
> +

Remove extra newline.

::: widget/nsXPLookAndFeel.cpp:715
(Diff revision 2)
> +

Remove extra newline.
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Cache overlay scrollbar LookAndFeel metric, and allow parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?

Pull down these commits:

hg pull -r 1d6f0c882c01500fbcce22a0cbeb71a7c91ee17c https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?

Pull down these commits:

hg pull -r 4abd57038185299027d51eedd5524439293e0abb https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?

Pull down these commits:

hg pull -r 4abd57038185299027d51eedd5524439293e0abb https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594783 - Flags: review?(mstange)
Attachment #8594783 - Flags: review?(jmathies)
Attachment #8594783 - Flags: review?(bugs)
https://reviewboard.mozilla.org/r/7307/#review6071

::: widget/LookAndFeel.h:592
(Diff revision 4)
> +  static void GetIntCache(InfallibleTArray<LookAndFeelInt>* lookAndFeelIntCache);

I think this can be
static InfallibleTArray<LookAndFeelInt> GetIntCache();
nowadays.

::: widget/LookAndFeel.h:593
(Diff revision 4)
> +  static void SetIntCache(InfallibleTArray<LookAndFeelInt>* lookAndFeelIntCache);

Better make this take a const InfallibleTArray<LookAndFeelInt>& aLookAndFeelIntCache.
https://reviewboard.mozilla.org/r/7329/#review6073

Ship It!

::: widget/cocoa/nsLookAndFeel.mm:677
(Diff revision 1)
> +  int32_t length = lookAndFeelIntCache->Length();
> +  for (int32_t i = 0; i < length; ++i) {

for (auto entry : *lookAndFeelIntCache) {
  ...
}

Updated

3 years ago
Depends on: 1156538
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?

Pull down these commits:

hg pull -r 4abd57038185299027d51eedd5524439293e0abb https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594783 - Flags: review?(mstange)
Attachment #8594783 - Flags: review?(jmathies)
Attachment #8594783 - Flags: review?(bugs)
Depends on: 1156934
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=?.

Pull down these commits:

hg pull -r 969f30cc8c7a05541111f92f994aabb92d6e5fdf https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=?.

Pull down these commits:

hg pull -r 969f30cc8c7a05541111f92f994aabb92d6e5fdf https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594783 - Flags: review?(mstange)
Attachment #8594783 - Flags: review?(jmathies)
Attachment #8594783 - Flags: review?(bugs)
https://reviewboard.mozilla.org/r/7307/#review6147

::: widget/nsXPLookAndFeel.h:84
(Diff revision 5)
> +  virtual void SetIntCacheImpl(const InfallibleTArray<LookAndFeelInt>& lookAndFeelIntCache) {};

This semicolon is unexpected. Does it compile?

::: widget/nsXPLookAndFeel.cpp:710
(Diff revision 5)
> +  InfallibleTArray<LookAndFeelInt> lookAndFeelIntCache;
> +  return lookAndFeelIntCache;

I think this can just be "return InfallibleTArray<LookAndFeelInt>()".
https://reviewboard.mozilla.org/r/7307/#review6149

> This semicolon is unexpected. Does it compile?

It does indeed, but I can remove it.

> I think this can just be "return InfallibleTArray<LookAndFeelInt>()".

Okie doke, I'll update the patch.
https://reviewboard.mozilla.org/r/7327/#review6151

Ship It!

::: dom/ipc/TabParent.cpp:967
(Diff revision 4)
> +    InfallibleTArray<LookAndFeelInt> lookAndFeelCache =
> +      LookAndFeel::GetIntCache();
> +
> +    unused << SendThemeChanged(lookAndFeelCache);

You don't need the lookAndFeelCache intermediate variable here.
https://reviewboard.mozilla.org/r/7329/#review6153

Ship It!

::: widget/cocoa/nsLookAndFeel.mm:32
(Diff revision 2)
>  nsLookAndFeel::nsLookAndFeel() : nsXPLookAndFeel()
>  {
> +  mUseOverlayScrollbars = -1;
> +  mUseOverlayScrollbarsCached = false;
> +
> +  mAllowOverlayScrollbarsOverlap = -1;
> +  mAllowOverlayScrollbarsOverlapCached = false;

Please initialize these members in the member initializer list, and not in the constructor body.
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=mstange.

Pull down these commits:

hg pull -r f5ff3638d841e7e5297538acd0e1bd6866dec6ae https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=mstange.

Pull down these commits:

hg pull -r f5ff3638d841e7e5297538acd0e1bd6866dec6ae https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594783 - Flags: review?(mstange)
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=?
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=mstange.

Pull down these commits:

hg pull -r 3d61e9e8599c0e7b59db677cdbf67a5a734450e5 https://reviewboard-hg.mozilla.org/gecko/

Comment 46

3 years ago
https://reviewboard.mozilla.org/r/7329/#review6159

::: widget/cocoa/nsLookAndFeel.mm:652
(Diff revision 4)
> +nsLookAndFeel::GetIntCacheImpl()

so here's a question - on Windows I can go in and set ui.useOverlayScrollbars to true and get overlay behavior. This works with e10s. I'm assuming we can do the same on mac, but I'm wondering if the change takes effect?

Comment 47

3 years ago
https://reviewboard.mozilla.org/r/7307/#review6163

::: dom/ipc/ContentChild.cpp:18
(Diff revision 6)
>  #include "CrashReporterChild.h"

On this particular patch, which you flagged me on, r+.

Comment 48

3 years ago
https://reviewboard.mozilla.org/r/7307/#review6165

Ship It!
>+bool
>+TabChild::RecvThemeChanged(InfallibleTArray<LookAndFeelInt>&& aLookAndFeelIntCache)
>+{
>+  LookAndFeel::SetIntCache(aLookAndFeelIntCache);
>+  nsCOMPtr<nsIDocument> document(GetDocument());
>+  nsCOMPtr<nsIPresShell> presShell = document->GetShell();
>+  nsRefPtr<nsPresContext> presContext = presShell->GetPresContext();
>+  presContext->ThemeChanged();

document->GetShell() may return null, and so may presShell->GetPresContext();


Not sure about the coding style for */LookAndFeel.h, but
2 spaces for indentation and aFoo naming for arguments?


widget/WidgetMessageUtils.h
new code should follow Mozilla coding style. So, 2 spaces for indentation.

InfallibleTArray<LookAndFeelInt> GetIntCacheImpl();
huh, I _hate_ implicit ctor+Move(). So very much hides that we aren't doing copying. Oh well, if other people
can live with that insanity, I can survive too (but it does make reviewing slower since one need to look at the ctor implementation).

Updated

3 years ago
Attachment #8594783 - Flags: review?(bugs) → review+
(I have no idea why you're using InfallibleTArray and not shorter nsTArray)

Comment 51

3 years ago
(In reply to Olli Pettay [:smaug] from comment #50)
> (I have no idea why you're using InfallibleTArray and not shorter nsTArray)

I've wondered this myself. It seems to be used in ipdl interfaces only for some reason.

Updated

3 years ago
Attachment #8594783 - Flags: review?(jmathies) → review+
Attachment #8594783 - Flags: review+
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=jimm.
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=smaug.
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=mstange.

Pull down these commits:

hg pull -r ad01a0e24fd924ea9d88b1744ae63a4a864cb09a https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

/r/7307 - Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
/r/7327 - Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=smaug.
/r/7329 - Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=mstange.

Pull down these commits:

hg pull -r bba18dfa277315e48699d55523942cf9dd1fe6f3 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594783 - Flags: review?(jmathies)
Hey jimm,

I pushed my last set of patches up to Try, and oranged out because the LookAndFeel stuff was getting initted when the ContentChild was starting up, and without a display (like when running xpcshell tests), that was causing our GTK self-tests to fail.

After consulting with karlt, my solution is to have LookAndFeel in the content process query the parent only when nsXPLookAndFeel::Init is called (so only when layout/painting needs LookAndFeel).

Here's the interdiff:

https://reviewboard.mozilla.org/r/7307/diff/7-8/

Here's the new try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e057bc88610

Comment 55

3 years ago
https://reviewboard.mozilla.org/r/7307/#review6261

looks good.

Updated

3 years ago
Attachment #8594783 - Flags: review?(jmathies)

Comment 56

3 years ago
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley

https://reviewboard.mozilla.org/r/7305/#review6155

::: widget/cocoa/nsLookAndFeel.mm:652
(Diff revision 5)
> +nsLookAndFeel::GetIntCacheImpl()

It's not officially supported, but the overlay scrollbar setting can be set on other platforms. Is there any way we can move this out to XPLookAndFeel so it applies to all platforms?

Comment 57

3 years ago
(In reply to Jim Mathies [:jimm] from comment #56)
> Comment on attachment 8594783 [details]
> MozReview Request: bz://1096093/mconley
> 
> https://reviewboard.mozilla.org/r/7305/#review6155
> 
> ::: widget/cocoa/nsLookAndFeel.mm:652
> (Diff revision 5)
> > +nsLookAndFeel::GetIntCacheImpl()
> 
> It's not officially supported, but the overlay scrollbar setting can be set
> on other platforms. Is there any way we can move this out to XPLookAndFeel
> so it applies to all platforms?

^^^^^^

ignore

Comment 58

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9c7cd773a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed8157d26a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c19dd86f63
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9226985&repo=mozilla-inbound
Flags: needinfo?(mconley)

Comment 60

3 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b70530c5898
https://hg.mozilla.org/integration/mozilla-inbound/rev/805d4233dc4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d13a8cde5695
Looks like I was missing a header file that b2g didn't use.

Here's a new try that's looking pretty green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f87eaa8bf913

I'll wait for these last few to come in and re-land.
Flags: needinfo?(mconley)
Try looks green - relanding...
https://reviewboard.mozilla.org/r/7305/#review6299

> It's not officially supported, but the overlay scrollbar setting can be set on other platforms. Is there any way we can move this out to XPLookAndFeel so it applies to all platforms?

I'll open up a follow-up to investigate this, and link it to bug 1096093.

Comment 64

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5b0c1a9a86
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc6d2d9c5dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5edb5f659d7
https://hg.mozilla.org/mozilla-central/rev/fc5b0c1a9a86
https://hg.mozilla.org/mozilla-central/rev/4cc6d2d9c5dc
https://hg.mozilla.org/mozilla-central/rev/a5edb5f659d7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
See Also: → bug 1158798
Depends on: 1158798
Created attachment 8598685 [details]
Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1096093.html
Depends on: 1163872
Comment on attachment 8594783 [details]
MozReview Request: bz://1096093/mconley
Attachment #8594783 - Attachment is obsolete: true
Created attachment 8618581 [details]
MozReview Request: Bug 1096093 - Have Cocoa widget backend cache overlay scrollbar metrics. r=mstange.
Created attachment 8618582 [details]
MozReview Request: Bug 1096093 - Add infrastructure for LookAndFeel metric caching, and allowing the parent process to send down cache to content process. r=?
Created attachment 8618583 [details]
MozReview Request: Bug 1096093 - Send ThemeRefresh message from parent down to content process. r=smaug.
You need to log in before you can comment on or make changes to this bug.