Open Bug 1390928 Opened 2 years ago Updated Last year

sendStyleSheetInfo() shows up in profiles during page show

Categories

(Firefox :: Menus, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: ehsan, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p3][fxperf:p3])

Profile: https://perfht.ml/2i4ROkm
Code: https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/tab-content.js#471

As far as I can tell, this information in the parent process is currently only used in two places:

  * getBrowserStyleSheets() <https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/browser.js#6183>  This seems to be dead code.
  * Opening the Page Style menu: <https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/browser.js#6209>

I think janking each page show in order to keep opening a menu that (I'm going to assume) almost no user ever looks at open fast is a questionable choice.  We should do this the other way around.  That is, make opening the menu slow!

One idea would be to add a "Loading page styles..." or some such item to the menu and send a message to the content process, and populate the menu once the info arrives in the parent.
Whiteboard: [qf]
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
>   * getBrowserStyleSheets()
> <https://searchfox.org/mozilla-central/rev/
> 13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/browser.
> js#6183>  This seems to be dead code.
I believe that was put there for addons (see Bug 1141041 comment #14 and 17), which given that legacy deprecation is now live I'm guessing could be ripped out?

>   * Opening the Page Style menu:
> <https://searchfox.org/mozilla-central/rev/
> 13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/browser.
> js#6209>
> 
> I think janking each page show in order to keep opening a menu that (I'm
> going to assume) almost no user ever looks at open fast is a questionable
> choice.  We should do this the other way around.  That is, make opening the
> menu slow!
> 
> One idea would be to add a "Loading page styles..." or some such item to the
> menu and send a message to the content process, and populate the menu once
> the info arrives in the parent.
That was originally attempted but didn't work out:

(In reply to Mike Conley (:mconley) - (PTO starting Aug 11 - Back Aug 17) from Bug 1141041 comment #3)
> So I abandoned the cache idea entirely, and decided to try to asynchronously
> populate the Page Style menu after it had been opened. Essentially, when
> detecting the popupshowing event on the Page Style popup, I'd send a message
> down to content asking for the stylesheet info, and when the message came
> back (if the same browser was selected), we'd inject the right nodes into
> the popupmenu.
> 
> I've hit a bit of a snag with this last approach, because while I've got all
> of the infrastructure in place to populate the menu asynchronously, at least
> on OS X, the native global menu doesn't seem to become invalidated /
> updated. So now I'm down in the OS X menu glue trying to figure out if
> there's some what I can trigger an invalidation, but that'd be getting
> pretty hacky.
(In reply to Ian Moody [:Kwan] from comment #1)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #0)
> >   * getBrowserStyleSheets()
> > <https://searchfox.org/mozilla-central/rev/
> > 13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/browser.
> > js#6183>  This seems to be dead code.
> I believe that was put there for addons (see Bug 1141041 comment #14 and
> 17), which given that legacy deprecation is now live I'm guessing could be
> ripped out?

Good point, and yes, makes sense.

> >   * Opening the Page Style menu:
> > <https://searchfox.org/mozilla-central/rev/
> > 13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/browser.
> > js#6209>
> > 
> > I think janking each page show in order to keep opening a menu that (I'm
> > going to assume) almost no user ever looks at open fast is a questionable
> > choice.  We should do this the other way around.  That is, make opening the
> > menu slow!
> > 
> > One idea would be to add a "Loading page styles..." or some such item to the
> > menu and send a message to the content process, and populate the menu once
> > the info arrives in the parent.
> That was originally attempted but didn't work out:
> 
> (In reply to Mike Conley (:mconley) - (PTO starting Aug 11 - Back Aug 17)
> from Bug 1141041 comment #3)
> > So I abandoned the cache idea entirely, and decided to try to asynchronously
> > populate the Page Style menu after it had been opened. Essentially, when
> > detecting the popupshowing event on the Page Style popup, I'd send a message
> > down to content asking for the stylesheet info, and when the message came
> > back (if the same browser was selected), we'd inject the right nodes into
> > the popupmenu.
> > 
> > I've hit a bit of a snag with this last approach, because while I've got all
> > of the infrastructure in place to populate the menu asynchronously, at least
> > on OS X, the native global menu doesn't seem to become invalidated /
> > updated. So now I'm down in the OS X menu glue trying to figure out if
> > there's some what I can trigger an invalidation, but that'd be getting
> > pretty hacky.

Oh.  Thanks for pointing this out.

Markus, do you know if it is possible to update the menus dynamically on OSX?  From my old memory of how this used to worked, we looked at the menus registered on the hidden window on OSX to tell the OS which menus we have available, not sure if that's still the case, but perhaps that was the issue?
Flags: needinfo?(mstange)
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #2)
> Markus, do you know if it is possible to update the menus dynamically on
> OSX?

As far as I know it is not, no.

> From my old memory of how this used to worked, we looked at the menus
> registered on the hidden window on OSX to tell the OS which menus we have
> available, not sure if that's still the case, but perhaps that was the issue?

The hidden window is used to populate the menubar if all windows are closed. But it's not really relevant to the question about asynchronous menu population.
Flags: needinfo?(mstange)
If we can't figure out a way of lazily populating the menu with stylesheet information, we could also send up the stylesheet in an idle callback instead of immediately during pageshow:

https://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/browser/base/content/tab-content.js#471-479
Whiteboard: [qf] → [qf:p3]
Assignee: nobody → mconley
(In reply to Mike Conley (:mconley) (:⚙️) from comment #4)
> If we can't figure out a way of lazily populating the menu with stylesheet
> information, we could also send up the stylesheet in an idle callback
> instead of immediately during pageshow:

Yes, but then we'd need to figure out what to show in the menu in the parent process if the user opens it before the information has been sent up to the parent...
Priority: -- → P3
Keywords: perf
OSX might not allow us to update the menu while it is open, but could we at least do this for Windows? That would solve the performance issue for a very high percentage of our users. mconley, do you still have your patch lying around?
Flags: needinfo?(mconley)
Closing old needinfo requests...
Flags: needinfo?(mconley)
Ooof, yep, sorry, this never got answered. My bad.

I don't have the old patch lying around, but it wasn't too complicated - initially the menu was empty, and then we send a message down requesting the stylesheet information. Once it's received, the XUL menu was updated and items showed up.

I'm clearly not actively working on this though, and I'm sorry I've clogged it up for so long. De-assigning for now.
Assignee: mconley → nobody
See Also: → 1494587
I thinmk this is more of a frontend perf thing so going to radar it there.
Whiteboard: [qf:p3] → [qf:p3][fxperf]
Whiteboard: [qf:p3][fxperf] → [qf:p3][fxperf:p3]
You need to log in before you can comment on or make changes to this bug.