Closed Bug 1370294 Opened 7 years ago Closed 7 years ago

Lazily append the dateTimePopupFrame iframe in browser.xul

Categories

(Toolkit :: XUL Widgets, defect)

Product:
Toolkit
Component:
XUL Widgets
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1370294 - Lazily create the iframe for the date time picker;
(deleted), text/x-review-board-request
mconley
: review+
Details
There's currently an iframe in the browser.xul markup to load the date time picker: https://github.com/mozilla/gecko-dev/blob/d441cb24482c2e5448accaf07379445059937080/browser/base/content/browser.xul#L172. It's then accessed via querySelector in the binding: https://github.com/mozilla/gecko-dev/blob/1ff4a3995fd18cb7cc74b7f00911d2c8ce974ba6/toolkit/content/widgets/datetimepopup.xml#L19. The field could be converted into a property that first checks to see if the frame exists and if not, appends it to the panel.
I'm not seeing a significant improvement on talos with the change applied [0]. But https://jsfiddle.net/76vqay5e/ suggests that creating an iframe with no src is ~2ms on my hardware, maybe it's not enough to be noticed by tpaint. [0]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e038fad1605167af4ed8da0e89e1a0a07b1297d2&newProject=try&newRevision=83e26b8fec87e647e7ada57f04c900d822f55783&framework=1&showOnlyImportant=0
Blocks: 1283381
See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=1283381 →
No longer blocks: 1283384
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8874503 [details] Bug 1370294 - Lazily create the iframe for the date time picker; https://reviewboard.mozilla.org/r/145874/#review150326 Hm. The <xul:panel> has hidden="true" set on it by default until it's eventually opened, and I imagine it's only then that we'll create the frameloader / set up the <iframe>... If we're not sure if this has any performance value, I'm hesitant to r+ it. I'll happily do so if it can be shown this has any amount of impact on tpaint, ts_paint, (or real-world user impact on window / browser open).
Attachment #8874503 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) from comment #4) > Comment on attachment 8874503 [details] > Bug 1370294 - Lazily create the iframe for the date time picker; > > https://reviewboard.mozilla.org/r/145874/#review150326 > > Hm. The <xul:panel> has hidden="true" set on it by default until it's > eventually opened, and I imagine it's only then that we'll create the > frameloader / set up the <iframe>... That's what I assumed as well, but I noticed that an <html>, <head>, and <body> were created for the frame in a new window when I was traversing the DOM with a deeptreewalker in a mochitest at [0]. You can also see those elements if you open the Browser Toolbox and search for #dateTimePopupFrame. Is it possible that those elements are somehow created without the frameloader being cerated? Here's all the elements created within the panel: INFO - GECKO(1104) | console.log: Found XBL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel INFO - GECKO(1104) | console.log: Found XUL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel INFO - GECKO(1104) | console.log: Found XUL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) INFO - GECKO(1104) | console.log: Found XUL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(0) INFO - GECKO(1104) | console.log: Found XBL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(0) image:nth-child(0) INFO - GECKO(1104) | console.log: Found XUL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(0) image:nth-child(0) INFO - GECKO(1104) | console.log: Found XUL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(1) INFO - GECKO(1104) | console.log: Found XBL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(1) iframe#dateTimePopupFrame INFO - GECKO(1104) | console.log: Found XUL element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(1) iframe#dateTimePopupFrame INFO - GECKO(1104) | console.log: Found HTML element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(1) iframe#dateTimePopupFrame html:nth-child(0) INFO - GECKO(1104) | console.log: Found HTML element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(1) iframe#dateTimePopupFrame html:nth-child(0) head:nth-child(0) INFO - GECKO(1104) | console.log: Found HTML element: window#main-window popupset#mainPopupSet panel#DateTimePickerPanel vbox:nth-child(0) box:nth-child(1) iframe#dateTimePopupFrame html:nth-child(0) body:nth-child(1) [0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f240598809552379792fa3d65d91a712884d1978&selectedJob=104199911 > If we're not sure if this has any performance value, I'm hesitant to r+ it. > I'll happily do so if it can be shown this has any amount of impact on > tpaint, ts_paint, (or real-world user impact on window / browser open). I've confirmed with the patch applied that we create one less <html>, <head>, and <body> when opening a window. But maybe that alone isn't enough to justify the change - happy to wontfix if so.
Comment on attachment 8874503 [details] Bug 1370294 - Lazily create the iframe for the date time picker; https://reviewboard.mozilla.org/r/145874/#review150346 > I've confirmed with the patch applied that we create one less \<html>, > \<head>, and \<body> when opening a window. But maybe that alone > isn't enough to justify the change - happy to wontfix if so. That's enough evidence for me that this is helping us avoid work on start-up. Thanks!
Attachment #8874503 - Flags: review- → review+
And just for my own reference later, this is one of 3 html frames loaded at startup: 1) The content browser: `tabbrowser#content browser[anonid=initialBrowser]` 2) `iframe#dateTimePopupFrame` 3) `browser#sidebar`
Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8117f05e2a20 Lazily create the iframe for the date time picker;r=mconley
https://hg.mozilla.org/mozilla-central/rev/8117f05e2a20
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: