Closed
Bug 1096093
Opened 11 years ago
Closed 10 years ago
[e10s] Scrollbar missing when e10s enabled
Categories
(Firefox :: Untriaged, defect)
Firefox
Untriaged
Tracking
()
RESOLVED
FIXED
Firefox 40
People
(Reporter: bkerensa, Assigned: mconley)
References
Details
Attachments
(5 files, 1 obsolete file)
Scrollbar does not show up when e10s enabled but shows up fine when disabled.
Build: 20141109030205
Comment 1•11 years ago
|
||
this works for me. Got a screenshot? what platform? any addons?
Flags: needinfo?(bkerensa)
Reporter | ||
Comment 2•11 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)
Comment 3•11 years ago
|
||
(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)
Comment 6•11 years ago
|
||
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•11 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)
Comment 8•11 years ago
|
||
Benjamin & Mark, do you use an external display?
Flags: needinfo?(standard8)
Flags: needinfo?(bkerensa)
Comment 9•11 years ago
|
||
(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
Comment 10•11 years ago
|
||
(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)
Comment 11•11 years ago
|
||
(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)
Comment 12•11 years ago
|
||
(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•11 years ago
|
||
I am on a internal display when I run into this
Flags: needinfo?(bkerensa)
Updated•11 years ago
|
Assignee: nobody → mconley
tracking-e10s:
--- → m7+
Updated•11 years ago
|
Summary: [e10s] Scrollbar missing when enabled → [e10s] Scrollbar missing when e10s enabled
Updated•11 years ago
|
Assignee | ||
Comment 16•10 years ago
|
||
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•10 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)
Assignee | ||
Comment 18•10 years ago
|
||
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•10 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)
Assignee | ||
Comment 20•10 years ago
|
||
Still no luck reproducing. :(
:tracy, got any cycles to help me figure out what I need to do to see this?
Flags: needinfo?(twalker)
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
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
Assignee | ||
Comment 23•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8589141 -
Attachment mime type: application/force-download → text/plain
Assignee | ||
Comment 24•10 years ago
|
||
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.
Comment 25•10 years ago
|
||
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?
Assignee | ||
Comment 26•10 years ago
|
||
(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.
Assignee | ||
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
/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/
Assignee | ||
Comment 29•10 years ago
|
||
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/
Assignee | ||
Comment 30•10 years ago
|
||
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.
Assignee | ||
Comment 31•10 years ago
|
||
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/
Assignee | ||
Comment 32•10 years ago
|
||
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/
Assignee | ||
Comment 33•10 years ago
|
||
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)
Comment 34•10 years ago
|
||
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.
Comment 35•10 years ago
|
||
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) {
...
}
Assignee | ||
Comment 36•10 years ago
|
||
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)
Assignee | ||
Comment 37•10 years ago
|
||
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/
Assignee | ||
Comment 38•10 years ago
|
||
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)
Comment 39•10 years ago
|
||
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>()".
Assignee | ||
Comment 40•10 years ago
|
||
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.
Comment 41•10 years ago
|
||
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.
Comment 42•10 years ago
|
||
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.
Assignee | ||
Comment 43•10 years ago
|
||
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/
Assignee | ||
Comment 44•10 years ago
|
||
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)
Assignee | ||
Comment 45•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
Comment 49•10 years ago
|
||
>+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•10 years ago
|
Attachment #8594783 -
Flags: review?(bugs) → review+
Comment 50•10 years ago
|
||
(I have no idea why you're using InfallibleTArray and not shorter nsTArray)
![]() |
||
Comment 51•10 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•10 years ago
|
Attachment #8594783 -
Flags: review?(jmathies) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8594783 -
Flags: review+
Assignee | ||
Comment 52•10 years ago
|
||
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/
Assignee | ||
Comment 53•10 years ago
|
||
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)
Assignee | ||
Comment 54•10 years ago
|
||
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•10 years ago
|
||
![]() |
||
Updated•10 years ago
|
Attachment #8594783 -
Flags: review?(jmathies)
![]() |
||
Comment 56•10 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•10 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•10 years ago
|
||
Comment 59•10 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Comment 61•10 years ago
|
||
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)
Assignee | ||
Comment 62•10 years ago
|
||
Try looks green - relanding...
Assignee | ||
Comment 63•10 years ago
|
||
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•10 years ago
|
||
Comment 65•10 years ago
|
||
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
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Assignee | ||
Comment 66•10 years ago
|
||
bugnotes |
Assignee | ||
Comment 67•10 years ago
|
||
Attachment #8594783 -
Attachment is obsolete: true
Assignee | ||
Comment 68•10 years ago
|
||
Assignee | ||
Comment 69•10 years ago
|
||
Assignee | ||
Comment 70•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•