Closed Bug 1334642 Opened 7 years ago Closed 7 years ago

Synchronous flush when activating/deactivating a window

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.4 - May 1
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- verified
firefox57 --- verified

People

(Reporter: ehsan.akhgari, Assigned: sfoster)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [photon-performance] [qa-commented])

Attachments

(2 files, 7 obsolete files)

Trying out utils.elementsRestyled
1.13 KB, text/plain
Details
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.
59 bytes, text/x-review-board-request
mconley
: review+
dao
: review+
Details 
See this profile where we spend ~200ms doing this: <https://clptr.io/2jFRaGq>.

The problem is inferFromText doing a synchronous flush.  It would be nice if this could be replaced with some other way for computing this...
Hm. Hey jaws, this looks like we're doing a calculation to figure out if we should be applying brighttext or not. Why do we do this for every activation / deactivation? Is it not sufficient to do it once when a lw-theme is applied?
Flags: needinfo?(jaws)
(In reply to Mike Conley (:mconley) from comment #1)
> Hm. Hey jaws, this looks like we're doing a calculation to figure out if we
> should be applying brighttext or not. Why do we do this for every activation
> / deactivation? Is it not sufficient to do it once when a lw-theme is
> applied?

Forwarding to Dão who wrote the patch in bug 1012629 which is causing this.
Blocks: 1012629
Component: Toolbars and Customization → Theme
Flags: needinfo?(jaws) → needinfo?(dao+bmo)
(In reply to Mike Conley (:mconley) from comment #1)
> Hm. Hey jaws, this looks like we're doing a calculation to figure out if we
> should be applying brighttext or not. Why do we do this for every activation
> / deactivation? Is it not sufficient to do it once when a lw-theme is
> applied?

This isn't about lw-themes, it's about OS themes that change colors when a window becomes inactive. For instance, the title bar in classic Windows themes.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Mike Conley (:mconley) from comment #1)
> > Hm. Hey jaws, this looks like we're doing a calculation to figure out if we
> > should be applying brighttext or not. Why do we do this for every activation
> > / deactivation? Is it not sufficient to do it once when a lw-theme is
> > applied?
> 
> This isn't about lw-themes, it's about OS themes that change colors when a
> window becomes inactive. For instance, the title bar in classic Windows
> themes.

Can we measure the active/inactive color once per session and reuse it?
Themes can change while Firefox is running, but in practice that's an edge case which we don't support well, so getting these colors just once per session seems okay.
Priority: -- → P3
(In reply to Dão Gottwald [:dao] from comment #5)
> Themes can change while Firefox is running, but in practice that's an edge
> case which we don't support well, so getting these colors just once per
> session seems okay.

FWIW the widget layer notifies Gecko about these theme changes (see nsPresContext::ThemeChanged()).  Right now these aren't exposed in a programmable way to JS, but we can for example dispatch a chrome only event to the window or something.
(In reply to :Ehsan Akhgari from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Themes can change while Firefox is running, but in practice that's an edge
> > case which we don't support well, so getting these colors just once per
> > session seems okay.
> 
> FWIW the widget layer notifies Gecko about these theme changes (see
> nsPresContext::ThemeChanged()).  Right now these aren't exposed in a
> programmable way to JS, but we can for example dispatch a chrome only event
> to the window or something.

Last time I checked there was already an observer notification or something, but there are other problems with theme changes while Firefox is running (the SVG cache, for instance). In any case, as comment #5 says, the problem in this bug could be fixed by caching the results for the active/inactive state.
(In reply to :Gijs from comment #7)
> (In reply to :Ehsan Akhgari from comment #6)
> > (In reply to Dão Gottwald [:dao] from comment #5)
> > > Themes can change while Firefox is running, but in practice that's an edge
> > > case which we don't support well, so getting these colors just once per
> > > session seems okay.
> > 
> > FWIW the widget layer notifies Gecko about these theme changes (see
> > nsPresContext::ThemeChanged()).  Right now these aren't exposed in a
> > programmable way to JS, but we can for example dispatch a chrome only event
> > to the window or something.
> 
> Last time I checked there was already an observer notification or something,
> but there are other problems with theme changes while Firefox is running
> (the SVG cache, for instance).

Have you filed them?  We clear a bunch of caches when this happens, it could be we need to clear more.
Blocks: 1336241
(In reply to :Ehsan Akhgari from comment #8)
> (In reply to :Gijs from comment #7)
> > (In reply to :Ehsan Akhgari from comment #6)
> > > (In reply to Dão Gottwald [:dao] from comment #5)
> > > > Themes can change while Firefox is running, but in practice that's an edge
> > > > case which we don't support well, so getting these colors just once per
> > > > session seems okay.
> > > 
> > > FWIW the widget layer notifies Gecko about these theme changes (see
> > > nsPresContext::ThemeChanged()).  Right now these aren't exposed in a
> > > programmable way to JS, but we can for example dispatch a chrome only event
> > > to the window or something.
> > 
> > Last time I checked there was already an observer notification or something,
> > but there are other problems with theme changes while Firefox is running
> > (the SVG cache, for instance).
> 
> Have you filed them?  We clear a bunch of caches when this happens, it could
> be we need to clear more.

I think fixing this hasn't been a priority. We don't have the time to invest, and people don't change their OS theme so frequently that it's been an issue. I'm not sure off-hand if there are current open bugs on file - there probably are, about the symptoms (the curvy sides of tabs end up being miscoloured, on Windows, if you switch themes while Fx is up).
Blocks: UIJank
Another profile where there is a really bad pause caused by inferFromText(): <https://clptr.io/2lg0BzK>
Is there anything here that the Platform layer could provide here to make this not require the style flush and manual calculation of luminance with caching? Could our themes statically define what they expect their toolbar font colors to be, for example?
ni? for comment 11
Flags: needinfo?(dao+bmo)
(In reply to Mike Conley (:mconley) from comment #11)
> Is there anything here that the Platform layer could provide here to make
> this not require the style flush and manual calculation of luminance with
> caching? Could our themes statically define what they expect their toolbar
> font colors to be, for example?

Linux and Windows can use different platform colors for the menu bar vs. other toolbars, Windows can use different colors for inactive vs. active windows. We could certainly generate a list of these colors and compute them without a style flush in the browser window, and somehow communicate the results back to our themes, and then still take care of lightweight themes. To me that doesn't seem preferable over doing what we do today + caching.
Flags: needinfo?(dao+bmo)
I think perhaps a better way would be to register a weak reflow observer (using nsIDocShell::AddWeakReflowObserver) in the first browser window, and the first time the reflow is done run the inferFromText() code to get the colors and then cache and reuse them for the rest of the life time of the browser.  This should require no reflows and no hardcoding of colors.
(In reply to :Ehsan Akhgari from comment #14)
> I think perhaps a better way would be to register a weak reflow observer
> (using nsIDocShell::AddWeakReflowObserver) in the first browser window, and
> the first time the reflow is done run the inferFromText() code to get the
> colors [...]

Note that we also need to make sure not to call inferFromText too early, i.e. after we've applied a lightweight theme, after we've determined whether to draw tabs in the title bar (this affects the tab bar text color) etc.
Excuse the oversimplification, but IIUC we're paying a perf penalty to fetch colours which rarely change. Can we store the colours and pay the penalty only when they change?
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #16)
> Excuse the oversimplification, but IIUC we're paying a perf penalty to fetch
> colours which rarely change. Can we store the colours and pay the penalty
> only when they change?

Yes, see comment #4 / comment #5. Someone needs to go write this code. Nobody has time for this. If you have time, feel free to write the code and I will be happy to review it.
(In reply to Dão Gottwald [:dao] from comment #13)

> We could certainly generate a list of these colors and compute them
> without a style flush in the browser window, and somehow communicate the
> results back to our themes, and then still take care of lightweight themes.
> To me that doesn't seem preferable over doing what we do today + caching.

Even with caching for the session, wouldn't that still mean the first window opened would need to pay this price for this synchronous flush?

That would be ideal to get rid of. Especially when getComputedStyle() is just some native color we already know down in UXTheme. Themes/addons would make it a bit more complex than just that, alas... But with the new theming API that's at least feasible to track? (Still a bit ugly, but being fast ain't easy.)

And what about if Photon switches to SVG icons plus bug 1058040? (Which, AIUI, lets us say "draw this shape with color X"). Seems like that would also need the machinery in place to know when X changes so we can update icons, and so that same tracking would still be needed.
(In reply to Justin Dolske [:Dolske] from comment #18)
> (In reply to Dão Gottwald [:dao] from comment #13)
> 
> > We could certainly generate a list of these colors and compute them
> > without a style flush in the browser window, and somehow communicate the
> > results back to our themes, and then still take care of lightweight themes.
> > To me that doesn't seem preferable over doing what we do today + caching.
> 
> Even with caching for the session, wouldn't that still mean the first window
> opened would need to pay this price for this synchronous flush?

No, see comment 14.

> That would be ideal to get rid of. Especially when getComputedStyle() is
> just some native color we already know down in UXTheme. Themes/addons would
> make it a bit more complex than just that, alas... But with the new theming
> API that's at least feasible to track? (Still a bit ugly, but being fast
> ain't easy.)
> 
> And what about if Photon switches to SVG icons plus bug 1058040? (Which,
> AIUI, lets us say "draw this shape with color X"). Seems like that would
> also need the machinery in place to know when X changes so we can update
> icons, and so that same tracking would still be needed.

Hmm, I'm not sure at all, but if the data can still be computed off of the computed style of some SVG node, the same technique as comment 14 can be used to obtain it without a synchronous flush.
(In reply to :Ehsan Akhgari from comment #14)
> I think perhaps a better way would be to register a weak reflow observer
> (using nsIDocShell::AddWeakReflowObserver) in the first browser window, and
> the first time the reflow is done run the inferFromText() code to get the
> colors and then cache and reuse them for the rest of the life time of the
> browser.  This should require no reflows and no hardcoding of colors.

As Dão said, we also need to wait for other things to have happened. Plus, there are at least 2 states that we need this for (active and non-active windows). We'd have to be very careful in the relevant frontend code to do this after the 'right' reflow(s).
(Sam will be looking at this soon.)

I'm inclined to focus on first implementing the caching in this bug (which seems straightforward, and fixes the bulk of the problem), and then spin off a followup bug to consider how we might additionally fix the cost of the initial color lookup.
Assignee: nobody → sfoster
Whiteboard: [qf:p1]
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review120746

This is my first pass at this, just storing the computed luminance values in a map keyed with the toolbar.id. The cache gets invalidated in the lw-theme update observer. 
Notably this doesnt handle the case where a theme defines different colors for active vs. inactive states. I've been testing this in Ubuntu and have not seen this with the handful of themes I've tried - I don't know how common it is. But I can certainly cache values for both active and inactive states if necessary. 
I had some logging in there which showed this is working well. Not sure if it needs tests?
Attachment #8845676 - Flags: review?(sfoster)
Attachment #8845676 - Flags: review?(sfoster) → review?(dao+bmo)
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review120758
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review120928

::: browser/base/content/browser.js:8213
(Diff revision 1)
> +      this._toolbarLuminances = new Map());
> +    let luminances = new Map();
>      for (let toolbar of document.querySelectorAll(toolbarSelector)) {
> +      // toolbars *should* all have ids, but guard anyway to avoid blowing up
> +      let luminance = toolbar.id && cachedLuminances.get(toolbar.id);
> +      if (isNaN(luminance)) {

This doesn't seem correct. For instance, we can't use the same cache for when the window is active and when it's inactive, because toolbar colors can change between that. Basically, we need a separate cache for every place calling inferFromText.
Attachment #8845676 - Flags: review?(dao+bmo) → review-
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review120928

> This doesn't seem correct. For instance, we can't use the same cache for when the window is active and when it's inactive, because toolbar colors can change between that. Basically, we need a separate cache for every place calling inferFromText.

I've updated the patch to cache the active and inactive values separately. Also, there's now a reCalculate method which we can use instead of inferFromText when a change should invalidate that cache. That's used when the lw-theme update fires, when a toolbar visibility changes. And, if we can get OS theme change notification at some point, we'll use it then too. I've not tried to be clever with cache invalidation - any change wipes all of it which seems safest at this point. 

I track the active window state in ToolbarIconColor using the activate/deactivate event listener. That seems reliable. Checking Services.focus.activeWindow I guess is something else entirely because I never saw that go false - toggling between multiple firefox windows, or other application windows. 

Unless I'm missing something, that seems to cover it, but maybe you had something else in mind?
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review120746

Good thing to bring up tests.

We want to avoid adding accidental reflow here in the future. What I'd suggest is writing a test that uses a reflow observer to ensure that switching focus between windows doesn't result in any reflows being logged.

There's an example of this kind of test [here](browser/base/content/test/general/browser_tabopen_reflows.js).
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review121992

::: browser/base/content/browser.js:5351
(Diff revision 2)
>    toolbar.dispatchEvent(event);
>  
>    PlacesToolbarHelper.init();
>    BookmarkingUI.onToolbarVisibilityChange();
> -  if (isVisible)
> -    ToolbarIconColor.inferFromText();
> +  if (isVisible) {
> +    ToolbarIconColor.reCalculate();

I don't think we should be proactively cautious here -- if the cache works as expected, there should be no problem using it here. No reason to wipe the cache as far as I can tell.

::: browser/base/content/browser.js:8154
(Diff revision 2)
>  
>      // If the window isn't active now, we assume that it has never been active
>      // before and will soon become active such that inferFromText will be
>      // called from the initial activate event.
> -    if (Services.focus.activeWindow == window)
> -      this.inferFromText();
> +    if (Services.focus.activeWindow == window) {
> +      this.reCalculate();

There should be nothing cached at this point so this change doesn't make sense.

::: browser/base/content/browser.js:8171
(Diff revision 2)
>  
>    handleEvent(event) {
>      switch (event.type) {
>        case "activate":
> +        this._isActiveWindow = true;
> +        this.inferFromText();

How about we let inferFromText take an argument for callers to specify the reason why they're calling it (i.e. what changed), and used that info to build the cache key?
Attachment #8845676 - Flags: review?(dao+bmo) → review-
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review122196

::: browser/base/content/browser.js:8154
(Diff revision 2)
>  
>      // If the window isn't active now, we assume that it has never been active
>      // before and will soon become active such that inferFromText will be
>      // called from the initial activate event.
> -    if (Services.focus.activeWindow == window)
> -      this.inferFromText();
> +    if (Services.focus.activeWindow == window) {
> +      this.reCalculate();

This was to ensure init() is idempotent and always resets the cache. That said, its probably not necessary as uninit clears out this._toolbarLuminances so I'll change this back

::: browser/base/content/browser.js:8171
(Diff revision 2)
>  
>    handleEvent(event) {
>      switch (event.type) {
>        case "activate":
> +        this._isActiveWindow = true;
> +        this.inferFromText();

I'm trying to figure out what you have in mind here. If youj mean to pass in a reason like inferFromText("activate|deactivate") (or "active|inactive") so we dont have to stash the `_isActiveWindow` property? I think we still need this state in cases where we call into inferFromText and don't have ready access to the window state. Or we could do something like:
```
inferFromText({ 
  active: true|false,
  fullscreen: true|false,
  ltTheme: true|false
})
```
or just: 
```
inferFromText("activate|deactivate|fullscreen|lightweight-theme-styling-update")
```
I.e. just pass in the event/notification name. That lets inferFromText figure out the smart thing to do, and doesnt place the onus on the calling code to know when to clear the cache or not. That sounds reasonable to me. It does mean inferFromText needs knowledge of all the events/places it can be called from. If that is ok, I'll make it so.
See my responses in Comment #30. Meantime I'm working on the test that :mconley mentioned.
Flags: needinfo?(dao+bmo)
(In reply to Sam Foster [:sfoster] from comment #30)
> ::: browser/base/content/browser.js:8154
> (Diff revision 2)
> >  
> >      // If the window isn't active now, we assume that it has never been active
> >      // before and will soon become active such that inferFromText will be
> >      // called from the initial activate event.
> > -    if (Services.focus.activeWindow == window)
> > -      this.inferFromText();
> > +    if (Services.focus.activeWindow == window) {
> > +      this.reCalculate();
> 
> This was to ensure init() is idempotent and always resets the cache. That
> said, its probably not necessary as uninit clears out
> this._toolbarLuminances so I'll change this back

We don't need either of these things. init should be called exactly once when the window opens, and uninit is supposed to be called once when the window closes, that's it.

> ::: browser/base/content/browser.js:8171
> (Diff revision 2)
> >  
> >    handleEvent(event) {
> >      switch (event.type) {
> >        case "activate":
> > +        this._isActiveWindow = true;
> > +        this.inferFromText();
> 
> I'm trying to figure out what you have in mind here. If youj mean to pass in
> a reason like inferFromText("activate|deactivate") (or "active|inactive") so
> we dont have to stash the `_isActiveWindow` property? I think we still need
> this state in cases where we call into inferFromText and don't have ready
> access to the window state.

Yes, inferFromText will still need to keep track of the window state, just in a more generic way than _isActiveWindow. For instance, there could be an object with active/fullscreen/etc. properties and we could use JSON.stringify to get a string for the cache key.
Flags: needinfo?(dao+bmo)
> > This was to ensure init() is idempotent and always resets the cache. That
> > said, its probably not necessary as uninit clears out
> > this._toolbarLuminances so I'll change this back
> 
> We don't need either of these things. init should be called exactly once
> when the window opens, and uninit is supposed to be called once when the
> window closes, that's it.

Fair enough. I'm used to people wanting to call init/uninit in tests where the window may get reused, so init/uninit need to be symmetrical - with no side-effects from init that aren't undone in uninit. If that's not a pattern that we need to worry about, I wont worry about it either. 

> > I'm trying to figure out what you have in mind here. If youj mean to pass in
> > a reason like inferFromText("activate|deactivate") (or "active|inactive") so
> > we dont have to stash the `_isActiveWindow` property? I think we still need
> > this state in cases where we call into inferFromText and don't have ready
> > access to the window state.
> 
> Yes, inferFromText will still need to keep track of the window state, just
> in a more generic way than _isActiveWindow. For instance, there could be an
> object with active/fullscreen/etc. properties and we could use
> JSON.stringify to get a string for the cache key.

It's likely I'm missing some important piece of the puzzle here or just thinking about this problem wrong, because as far as I can tell active/inactive is the only state we need to worry about. Going into fullscreen, none of these toolbars will be visible so we don't care what brighttext is. Coming out of fullscreen, we just need to know if we're active or not and should be able to use the corresponding cached value. Are there other considerations I'm missing? I know we are across a challenging timezone gap (I'm US/pacific time), but maybe we can chat on irc/vidyo about this? I'll look for you in #fx-team. I'm in no particular rush here, so if there's a time that works for you (or doesnt) let me know.
Flags: needinfo?(dao+bmo)
The fullscreen stuff is for manual fullscreen (F11) rather than DOM fullscreen. The tab and navigation toolbars are present there, and they can change colors.
Flags: needinfo?(dao+bmo)
Attachment #8848557 - Attachment is obsolete: true
Attachment #8848558 - Attachment is obsolete: true
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/118826/#review123492

I started sketching out the mochitest to verify we don't get reflows when reactivating windows. The new Error().stack is just giving me a single line though, which makes me think I'm misunderstanding where/how this  gets called?
Comment on attachment 8845676 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

The reflow observer is called by native code, 'new Error().stack' only gives you the JS callstack.
Attachment #8845676 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #40)
> Comment on attachment 8845676 [details]
> Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.
> 
> The reflow observer is called by native code, 'new Error().stack' only gives
> you the JS callstack.

Yeah, its expected that some of the stacks will be empty having come from native code. But, looking at a similar test in browser_windowopen_reflows.js, I would expect at least some of the reflows to result in readable stacks. And, that one of them would be from ToolbarIconColor.inferFromText, where we land after handling the "activate" and "deactivate" events. Without this patch getComputedStyle is called onconditionally which should result in a reflow. But that seems not to be the case. With or without my patch I don't see reflows froming from that spot. And actually, if I had thought about this, I would have expected ToolbarIconColor.inferFromText to be listed in the expected reflows paths in the browser_windowopen_reflows.js and similar tests. It is not. 

I know there's not a 1:1 with getComputedStyle resulting in a reflow. So perhaps the textbox.xml|select and focusAndSelectUrlBar path is being fingered as ultimately responsible for reflow, but inferFromText's getComputedStyle called earlier would also have triggered it? That doesn't agree with :ehsan's diagnosis though...

The original bug description was "The problem is inferFromText doing a synchronous flush" and the proposed (simple/stop-gap) solution which I'm trying to implement here: "the problem in this bug could be fixed by caching the results for the active/inactive state." (from comment #7) The patch should address this, and I'm just trying to write a test to confirm that is the case, and that it stays that way. Now I find either I don't know what the minimal set of steps are to reproduce the issue in order to write this test, or that I'm not sure if the symptoms  - and therefore the fix - are actually measurable. I'm obviously wrong somewhere in a least a couple of ways, so I'm open to suggestions :)
Flags: needinfo?(dao+bmo)
(In reply to Sam Foster [:sfoster] from comment #41)
> (In reply to Dão Gottwald [::dao] from comment #40)
> > Comment on attachment 8845676 [details]
> > Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.
> > 
> > The reflow observer is called by native code, 'new Error().stack' only gives
> > you the JS callstack.
> 
> Yeah, its expected that some of the stacks will be empty having come from
> native code. But, looking at a similar test in
> browser_windowopen_reflows.js, I would expect at least some of the reflows
> to result in readable stacks. And, that one of them would be from
> ToolbarIconColor.inferFromText, where we land after handling the "activate"
> and "deactivate" events. Without this patch getComputedStyle is called
> onconditionally which should result in a reflow. But that seems not to be
> the case. With or without my patch I don't see reflows froming from that
> spot. And actually, if I had thought about this, I would have expected
> ToolbarIconColor.inferFromText to be listed in the expected reflows paths in
> the browser_windowopen_reflows.js and similar tests. It is not.

Oh, I thought browser_windowopen_reflows.js had some different means to get the stack behind the reflow. I hadn't realized 'new Error().stack' is actually expected to work here. In that case, I can't make sense of this either. Ehsan?
Flags: needinfo?(dao+bmo) → needinfo?(ehsan)
Gijs, per bug 1348918, I'll update this patch to move this test somewhere less "general", but its not clear to me where that better place is?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Sam Foster [:sfoster] from comment #43)
> Gijs, per bug 1348918, I'll update this patch to move this test somewhere
> less "general", but its not clear to me where that better place is?

I guess you could move it together with the existing window/tabopen reflow tests into a new 'reflow' subdirectory of browser/base/content/test/general/.

It's not ideal to have a dir with very few tests in it, but they are pretty special tests in their way, given they only care about reflows and don't actually test anything functionality-wise.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #44)
> (In reply to Sam Foster [:sfoster] from comment #43)
> > Gijs, per bug 1348918, I'll update this patch to move this test somewhere
> > less "general", but its not clear to me where that better place is?
> 
> I guess you could move it together with the existing window/tabopen reflow
> tests into a new 'reflow' subdirectory of browser/base/content/test/general/.

Errr, I meant new subdirectory of browser/base/content/test/ . You'll need a new browser.ini file with just these tests (and any supporting files) in, and then add that browser.ini file to browser/base/moz.build .
(In reply to Dão Gottwald [::dao] from comment #42)
> (In reply to Sam Foster [:sfoster] from comment #41)
> > (In reply to Dão Gottwald [::dao] from comment #40)
> > > Comment on attachment 8845676 [details]
> > > Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.
> > > 
> > > The reflow observer is called by native code, 'new Error().stack' only gives
> > > you the JS callstack.
> > 
> > Yeah, its expected that some of the stacks will be empty having come from
> > native code. But, looking at a similar test in
> > browser_windowopen_reflows.js, I would expect at least some of the reflows
> > to result in readable stacks. And, that one of them would be from
> > ToolbarIconColor.inferFromText, where we land after handling the "activate"
> > and "deactivate" events. Without this patch getComputedStyle is called
> > onconditionally which should result in a reflow. But that seems not to be
> > the case. With or without my patch I don't see reflows froming from that
> > spot. And actually, if I had thought about this, I would have expected
> > ToolbarIconColor.inferFromText to be listed in the expected reflows paths in
> > the browser_windowopen_reflows.js and similar tests. It is not.
> 
> Oh, I thought browser_windowopen_reflows.js had some different means to get
> the stack behind the reflow. I hadn't realized 'new Error().stack' is
> actually expected to work here. In that case, I can't make sense of this
> either. Ehsan?

Are you asking how you can reproduce this? (I'm not 100% sure if I understand the question correctly, you may also be asking about how you can retrieve the native stack from JS...)

I hit this bug when I had several windows open as I switched between them.  Note that reflow is a lazy operation, so a single getComputedStyle() call may not trigger any work under a reflow call if there is no pending layout changes, so in the test you may want to ensure that the layout is dirty (for example by modifying the browser.xul DOM) before triggering the window activation/deactivation paths to guarantee that getComputedStyle will definitely trigger a reflow.
Flags: needinfo?(ehsan)
 > I hit this bug when I had several windows open as I switched between them. 
> Note that reflow is a lazy operation, so a single getComputedStyle() call
> may not trigger any work under a reflow call if there is no pending layout
> changes, so in the test you may want to ensure that the layout is dirty (for
> example by modifying the browser.xul DOM) before triggering the window
> activation/deactivation paths to guarantee that getComputedStyle will
> definitely trigger a reflow.

I backed up and I'm looking for steps to reproduce this bug as reported. I've added logging to ToolbarIconColor.inferFromText, and I've added a reflow observer (docShell.addWeakReflowObserver(this)) such that ToolbarIconColor logs out the stack trace when it sees a reflow. From this, I see inferFromText and its getComputedStyle calls getting called quite often as I create and switch between windows. But, I can only see a reflow that points back to inferFromText by going in/out of fullscreen (f11). The activate/deactivate events do not trigger reflow for me so far. Showing/hiding toolbars via the Customize menu doesnt trigger reflow. Actually, enabling a lw theme causes inferFromText to be called, but again no reflow. 

:mconley, I've tried adding & removing tabs and then switching between windows, toggling menus, bookmarking - and I still don't get a reflow. Can you suggest other STR? 

Clearly we're calling getComputedStyle a lot more than we need to here, so the cache idea remains valid I think. I'm not sure there's any value in a reflow mochitest until I can pin down reliable STR though?
Flags: needinfo?(mconley)
Hm. We might not actually be reflowing here. I may have led you down the garden path a little. :/

It looks like what we're doing instead of a synchronous style flush, and not a reflow. So a reflow observer wouldn't, I believe, be useful here, unless that resulting styles after the calculated required us to actually reflow the page.

Hey ehsan, do you know if we have anything like the reflow observer for things like style flushes?
Flags: needinfo?(mconley) → needinfo?(ehsan)
I took a quick look around the tree, and I can't find anything that we can use from script to detect style flushes.

I suggest we punt on the test. sfoster, is this patch ready for another round of review, modulo test removal?
Flags: needinfo?(ehsan) → needinfo?(sfoster)
Filed bug 1352058 to create a way for observing style flushes.
In bug 1352058 comment 2, dbaron pointed out some interesting properties inside the nsIDOMWindowUtils interface that we could use on the browser windows that we care about:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/interfaces/base/nsIDOMWindowUtils.idl#1874-1889

So we could write a test that records the number of elementsRestyled for the browser window before activation, and then elementsRestyled _after_ activation, and make sure that the number does not change (or only changes the expected amount for restyles that are expected / unavoidable).

Does that help?
Flags: needinfo?(sfoster)
Whoops, didn't mean to clear that needinfo.
Flags: needinfo?(sfoster)
Whiteboard: [qf:p1] → [photon] [qf:p1]
Attachment #8845676 - Attachment is obsolete: true
Attachment #8845676 - Flags: review?(dao+bmo)
Attachment #8853189 - Attachment is obsolete: true
Attachment #8853189 - Flags: review?(dao+bmo)
Dao: sorry for the noise as I flail at getting the right changes push for review. Attachment 8853188 [details] I think incorporates all your suggestions regarding the per-state cached values, and also the logic around which changes should flush out the cache or not. I've not yet addressed Mike's suggestions about the test, I'd like to add that before trying to land this, but this is in a good state for your feedback. 

I've put the cache behind a getter because, in the course of testing I was reminded that each browser/window gets its own copy of ToolbarIconColor, so each has to populate its own cache. This is still an improvement over the current state, but clearly it would be better if they could share the cache as toolbars with the same id, in the same state shouldnt need to re-compute the color values in each window. It sounds like to do that I would probably need to create a thin .jsm, and I'm not sure if the overhead associated with that starts to eat into the gains it brings. Do you have thoughts on this? Could be a follow-up, or I can add it in here. 

:mconley, thanks for that, it looks like layout/style/test/test_media_queries_dynamic.html is doing something similar. I'll give it a shot.
Flags: needinfo?(sfoster) → needinfo?(dao+bmo)
(In reply to Sam Foster [:sfoster] from comment #58)
> I've put the cache behind a getter because, in the course of testing I was
> reminded that each browser/window gets its own copy of ToolbarIconColor, so
> each has to populate its own cache.

I don't see how the getter makes a difference here. How is this different from, say, _toolbarLuminanceCache: new Map(), except that the getter creates the Map on first use rather than eagerly?

> This is still an improvement over the
> current state, but clearly it would be better if they could share the cache
> as toolbars with the same id, in the same state shouldnt need to re-compute
> the color values in each window. It sounds like to do that I would probably
> need to create a thin .jsm, and I'm not sure if the overhead associated with
> that starts to eat into the gains it brings. Do you have thoughts on this?
> Could be a follow-up, or I can add it in here.

Good point. However, I think down the road with SVG icons we can completely get rid of ToolbarIconColor, so let's not overthink this and get something landed asap.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #59)
> (In reply to Sam Foster [:sfoster] from comment #58)
> > I've put the cache behind a getter because, in the course of testing I was
> > reminded that each browser/window gets its own copy of ToolbarIconColor, so
> > each has to populate its own cache.
> 
> I don't see how the getter makes a difference here. How is this different
> from, say, _toolbarLuminanceCache: new Map(), except that the getter creates
> the Map on first use rather than eagerly?

You're right. The getter was planning ahead for possibly getting this cache from a shared .jsm, so this code could treat whatever that module exposed as a simple property. If we don't do that I'll just access the Map directly. 

> Good point. However, I think down the road with SVG icons we can completely
> get rid of ToolbarIconColor, so let's not overthink this and get something
> landed asap.

Ah, good to know.
I gave the utils.elementsRestyled (see bug 1352058) a go in this test. I'm logging out the elementsRestyled for each window, at each step, and I'm seeing some variation there as I create and switch focus between 2 new windows that should be basically identical: 

```
GECKO(32582) | console.log: initial win1, elementsRestyled is:  794
GECKO(32582) | console.log: initial win2, elementsRestyled is:  786

GECKO(32582) | console.log: focus win1, elementsRestyled is:  1357
GECKO(32582) | console.log: focus win2, elementsRestyled is:  1323
```

Is this variability something I can control for in how I create those windows? I'm not that familiar with all the methods and helpers for this. If some variability is expected and the restyle count isn't deterministic for this case, its going to be hard to make any assertions about. Add to that the actual caching effect in ToolbarIconColor.inferFromText which we are trying to test - so I can't for example toggle between the windows a couple times to try and get a stable restyle count.

Given what :dao said in comment #59 that SVG icons may allow us to remove ToolbarIconColor altogether, its possible we're into diminishing returns for the value of this test?
Attachment #8853518 - Attachment is patch: true
Attachment #8853518 - Attachment mime type: application/javascript → text/plain
Attachment #8853518 - Attachment is patch: false
(In reply to Sam Foster [:sfoster] from comment #61)
> 
> Given what :dao said in comment #59 that SVG icons may allow us to remove
> ToolbarIconColor altogether, its possible we're into diminishing returns for
> the value of this test?

I wouldn't say that, no. I hope dao is right, and we can remove ToolbarIconColor, and then we don't need to sync style flush here ever again. In fact, we should _ensure_ that we don't cause a sync style flush here in this use case ever again - and that's what the test is for. I guess I'm trying to make sure that we don't accidentally introduce a new one a few months down the line and not notice.

Would you mind posting your test as a patch? It'll be easier for me to review it that way.

I think there are a few alterations we could make to it to make it more consistent - though it's possible a fuzz-factor might be necessary. We'll see. When the patch is up, I'll give it a full-blown review and we'll see what we can do. :)

Thanks for working on this, sfoster.
Attachment #8853188 - Attachment is obsolete: true
Attachment #8853188 - Flags: review?(dao+bmo)
Comment on attachment 8853538 [details]
Exploratory test using utils.elementsStyled.

https://reviewboard.mozilla.org/r/125586/#review128672

Thanks for the test, sfoster! Much appreciated. :) Got some feedback below. Let me know if you have questions!

::: browser/base/content/test/general/browser_toolbariconcolor_restyles.js:2
(Diff revision 1)
> +"use strict";
> +add_task(function* test_toolbar_element_restyles_on_activation() {

Mind adding a quick comment up here describing what this test does?

::: browser/base/content/test/general/browser_toolbariconcolor_restyles.js:3
(Diff revision 1)
> +"use strict";
> +add_task(function* test_toolbar_element_restyles_on_activation() {
> +  SimpleTest.waitForExplicitFinish();

So the good news is, with add_task, you don't need to tell SimpleTest that we're going to finish asynchronously - it's implied once you start using add_task.

::: browser/base/content/test/general/browser_toolbariconcolor_restyles.js:6
(Diff revision 1)
> +    let win = yield BrowserTestUtils.openNewBrowserWindow();
> +    yield SimpleTest.promiseFocus(win);
> +    return win;

One of the problems here is that we're yielding back to the event loop here, which means that we're not actually testing for synchronous style calcluations. Any number of style calculations could happen between the point where we yield and the point that we're re-entered, due to any number of things.

What we really care about here are style calculations that occur synchronously when focus is shifted.

So if possible, we should try to do the following synchronously:

1) Get nsIDOMWindowUtils on win1 and win2, as you've done
2) Record a snapshot of the elementsRestyled count for both windows
3) Synchronously shift focus from one window to the other, _without_ going back to the event loop
4) Check that the elementsRestyled count has not increased
5) Close the windows using:

```js
yield BrowserTestUtils.closeWindow(win1);
yield BrowserTestUtils.closeWindow(win2);
```

::: browser/base/content/test/general/browser_toolbariconcolor_restyles.js:11
(Diff revision 1)
> +    let win = yield BrowserTestUtils.openNewBrowserWindow();
> +    yield SimpleTest.promiseFocus(win);
> +    return win;
> +  }
> +
> +  function* runTest() {

We also don't need this nested function - we can run all of these things from within the add_task function.
Attachment #8853538 - Flags: review?(mconley) → review-
Comment on attachment 8853537 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/125584/#review129388

::: browser/base/content/browser.js:8302
(Diff revision 1)
> +      case "activate": // falls through
> +      case "deactivate":
> +        this._windowState.active = (reason === "activate");
> +        break;
> +      case "fullscreen":
> +        this._windowState.fullscreen = reasonValue

nit: missing semicolon

::: browser/base/content/browser.js:8305
(Diff revision 1)
> +        break;
> +      case "fullscreen":
> +        this._windowState.fullscreen = reasonValue
> +        break;
> +      case "lightweight-theme-styling-update":
> +          // theme change, we'll need to recalculate all color values

nit: indentation is off

::: browser/base/content/browser.js:8309
(Diff revision 1)
> +      case "lightweight-theme-styling-update":
> +          // theme change, we'll need to recalculate all color values
> +        this._toolbarLuminanceCache.clear();
> +        break;
> +      case "toolbarvisibilitychange":
> +          // toolbar changes dont require reset of the caches color values

ditto
Attachment #8853537 - Flags: review?(dao+bmo) → review+
See Also: → 1354194
No longer blocks: UIJank
Attachment #8853537 - Attachment is obsolete: true
Attachment #8853538 - Attachment is obsolete: true
:Dao, sorry I lost your r+ trying to get this rebased and the test on there. The new patch includes a test for the restyles, and addresses the nits you pointed out in Attachment #8853537 [details]. :mconley has said he'll review the test so I think I just need your stamp again. 

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d876951efc1a010ff7a62711335d62339d588ac
Comment on attachment 8856609 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/128562/#review131292

::: browser/base/moz.build:38
(Diff revision 3)
>      'content/test/tabPrompts/browser.ini',
>      'content/test/tabs/browser.ini',
>      'content/test/urlbar/browser.ini',
>      'content/test/webextensions/browser.ini',
>      'content/test/webrtc/browser.ini',
> +    'content/test/windows/browser.ini',

"windows" to me doesn't seem like meaningful enough name here.
Attachment #8856609 - Flags: review?(dao+bmo) → review+
Comment on attachment 8856609 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/128562/#review131292

> "windows" to me doesn't seem like meaningful enough name here.

Is it any less meaningful than "tabs"? Not being cheeky - I'm just curious to know what you think the right folder name would be for a test like this.
Comment on attachment 8856609 [details]
Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.

https://reviewboard.mozilla.org/r/128562/#review131554

Test looks good to me! Thanks sfoster.
Attachment #8856609 - Flags: review?(mconley) → review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a98180ffc2f
Cache luminance values for each toolbar in ToolbarIconColor. r=dao,mconley
Hey sfoster, did you come to a resolution on the test folder name with dao?
Flags: needinfo?(sfoster)
I suggested we rename in a follow-up when we have more tests and can make a more informed choice. Did I misunderstand and he wanted to defer landing until that was resolved? Eek.
Flags: needinfo?(sfoster)
(In reply to Sam Foster [:sfoster] from comment #77)
> I suggested we rename in a follow-up when we have more tests and can make a
> more informed choice. Did I misunderstand and he wanted to defer landing
> until that was resolved? Eek.

Filing a new bug and fixing in a follow-up is fine with me. Let's just not bikeshed on a folder name for too long. :)
(In reply to Mike Conley (:mconley) from comment #72)
> Comment on attachment 8856609 [details]
> Bug 1334642 - Cache luminance values for each toolbar in ToolbarIconColor.
> 
> https://reviewboard.mozilla.org/r/128562/#review131292
> 
> > "windows" to me doesn't seem like meaningful enough name here.
> 
> Is it any less meaningful than "tabs"?

Yes! Those tests test aspects of our tabbed browser implementation (tabbrowser.xml).

> Not being cheeky - I'm just curious
> to know what you think the right folder name would be for a test like this.

How about style-and-layout-flushes/? Or something more generic like performance/? It depends on what other tests we'll want to add there. And if we don't want to add similar tests, we shouldn't have a folder for this in the first place.
Whiteboard: [photon] [qf:p1] → [qf:p1]
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: P3 → P1
Whiteboard: [qf:p1] → [photon-performance] [qf:p1]
Hey sfoster, maybe let's land this fix again on its own, and file a bug with the test separately so it can be worked on independently. Sound good?
Flags: needinfo?(sfoster)
(In reply to Mike Conley (:mconley) from comment #82)
> Hey sfoster, maybe let's land this fix again on its own, and file a bug with
> the test separately so it can be worked on independently. Sound good?

I can do that. FWIW in your try push from Comment #81 and the re-triggers on mine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fe9887e2542efd45cf180e386d51ec8abd3dcc0&selectedJob=91751108 - we've not been able to reproduce that initial failure. I'm working on a win8 environment to try and reproduce it.
Flags: needinfo?(sfoster)
(In reply to Sam Foster [:sfoster] from comment #83)
> I can do that. FWIW in your try push from Comment #81 and the re-triggers on
> mine:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2fe9887e2542efd45cf180e386d51ec8abd3dcc0&selectedJob=9
> 1751108 - we've not been able to reproduce that initial failure. I'm working
> on a win8 environment to try and reproduce it.

Note that in the patch I pushed to try, I modified your test slightly to ensure that the second window had fully focused (using a a yield on a Promise wrapping SimpleTest.waitForFocus) before taking the measurements and doing the sync .focus() calls. Perhaps that's related.
Blocks: 1356684
I've re-opened the review request and modified the test to skip windows 8 for now at :jaws' suggestion. I had already filed bug 1356684 as a follow-up for this test, so I can adopt that for re-enabling this on win8.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=116aa856bfac3be59735d9b5f0177ffba22351a5
If that ends up ok I'll attempt to re-land.
(In reply to Sam Foster [:sfoster] from comment #88)
> Non-artifact try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb94bd6047e6c7d98d44d0475f308d9d75e56def

Looks better :mconley? I don't have the context needed to know if I should be worrying about those remaining oranges. AFAICT they look unrelated and this should be ready to land.
Flags: needinfo?(mconley)
(In reply to Sam Foster [:sfoster] from comment #89)
> (In reply to Sam Foster [:sfoster] from comment #88)
> > Non-artifact try:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb94bd6047e6c7d98d44d0475f308d9d75e56def
> 
> Looks better :mconley? I don't have the context needed to know if I should
> be worrying about those remaining oranges. AFAICT they look unrelated and
> this should be ready to land.

Looks good to me!
Flags: needinfo?(mconley)
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
See Also: 1354194 → 
(In reply to Sam Foster [:sfoster] from comment #96)
> So, unless I'm missing something obvious, or there are better ideas, I'm
> back to landing this sans-test and working on the test in bug 1356684.

Yep, that sounds like a plan.
Flags: needinfo?(mconley)
Mea culpa, the patch on that last round of tests got screwed up when I was rebasing and was missing the browser.js changes. So the test was correctly reporting the error (yay?). I.e. ignore comment #91 thru comment #96. I've fixed up the patch and included mike's proposed changes to the test to wait for window focus before getting the initial snapshot of elementsStyled. Re-pushed to try: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0e65171a7d267ac70d6df21e9e58b5c53bf920d&selectedJob=92803459
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8c1952346f3
Cache luminance values for each toolbar in ToolbarIconColor. r=dao,mconley
https://hg.mozilla.org/mozilla-central/rev/c8c1952346f3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.4 - May 1
Blocks: 1358899
Depends on: 1358356
QA note: this patch changes the code that calculates the colors to use in the toolbar when activating/deactivating a window. I think it would make sense to test that color changes there still work as expected when activating/deactivating windows, and interactions with theme changes (potentially both Firefox lightweight themes and OS themes).
Whiteboard: [photon-performance] [qf:p1] → [photon-performance] [qf:p1][qa-commented]
Blocks: 1383367
Environment: Windows 7 x32, Windows 10 x64, Windows 8.1 (with touch), Ubuntu 16.04 x64, OSX 10.12: 57.0a1 20170908100218

Tested the activate/deactivate window including:

-High contrast OS themes (Windows/Ubuntu) & Firefox Light/Dark themes - combinations of the two.
-32b/64b FF
-Normal/Private window
-Tabs/toolbars
-non e10s

I also included light smoke testing on 55.0.3 20170824053622 Ubuntu 16.04 and Windows10 x64, but not insisted on it, since it has already been in release for a while now. 

As in no regressions were identified at this point, marking this as verified for FF55 and FF57.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox57: --- → verified
Flags: qe-verify+
Performance Impact: --- → P1
Whiteboard: [photon-performance] [qf:p1][qa-commented] → [photon-performance] [qa-commented]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: