Closed
Bug 878747
Opened 11 years ago
Closed 11 years ago
browser.stop() call in addTab() is expensive and causes reflows
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 24
People
(Reporter: ttaubert, Assigned: ttaubert)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
While profiling gBrowser.addTab() calls b.stop() turned out to be quite expensive and does also cause an uninterruptible reflow:
http://hg.mozilla.org/mozilla-central/file/876d89111acb/browser/base/content/tabbrowser.xml#l1396
I think it would be great if we could just prevent the frame from loading 'about:blank' in the first place if that's not the URL we want. We would then save time loading, stopping and reflowing.
Assignee | ||
Comment 1•11 years ago
|
||
This patch adds the 'noloadsrc' attribute for XUL frames that prevents LoadSrc() from being called when it's set before the element is injected into the DOM.
Attachment #757443 -
Flags: review?(bugs)
Assignee | ||
Comment 2•11 years ago
|
||
This patch uses the new 'noloadsrc' attribute to prevent the newly created <browser> from loading 'about:blank' if that's not the URL we want.
Attachment #757445 -
Flags: review?(dao)
Updated•11 years ago
|
Attachment #757443 -
Flags: review?(bugs)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #757443 -
Attachment is obsolete: true
Attachment #757589 -
Flags: review?(bugs)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #757445 -
Attachment is obsolete: true
Attachment #757445 -
Flags: review?(dao)
Attachment #757590 -
Flags: review?(dao)
Comment 5•11 years ago
|
||
Comment on attachment 757589 [details] [diff] [review]
part 1 - implement 'nodefaultsrc' attribute for XUL frames
if (expr) {
stmt;
}
and eCaseMatters
(XUL is case-sensitive)
Attachment #757589 -
Flags: review?(bugs) → review+
Comment 6•11 years ago
|
||
Comment on attachment 757590 [details] [diff] [review]
part 2 - use the new 'nodefaultsrc' attribute for browsers
>+ // If the URI to load is not 'about:blank' then let's prevent the
>+ // initial load of a blank document. We're going to kick off the
>+ // actual page load after we know that no preloaded newtab page
>+ // has been swapped in.
>+ if (!uriIsAboutBlank) {
>+ b.setAttribute("nodefaultsrc", "true");
>+ }
The preload stuff isn't really of interest here. I'd just have the comment say: "Prevent the superfluous initial load of a blank document if we're going to load something other than about:blank."
>+ // TabView causes reflows. Need to find out why.
>+ "iQClass_height@chrome://browser/content/tabview.js|" +
>+ "GroupItem_getContentBounds@chrome://browser/content/tabview.js|" +
>+ "GroupItem_shouldStack@chrome://browser/content/tabview.js|" +
>+ "GroupItem_arrange@chrome://browser/content/tabview.js|" +
>+ "GroupItem_add@chrome://browser/content/tabview.js|" +
>+ "GroupItems_newTab@chrome://browser/content/tabview.js|" +
>+ "TabItem__reconnect@chrome://browser/content/tabview.js|" +
>+ "TabItem@chrome://browser/content/tabview.js|" +
>+ "TabItems_link@chrome://browser/content/tabview.js|" +
>+ "@chrome://browser/content/tabview.js|" +
>+ "addTab@chrome://browser/content/tabbrowser.xml|"
What does this have to do with this bug?
Attachment #757590 -
Flags: review?(dao) → review+
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #6)
> The preload stuff isn't really of interest here. I'd just have the comment
> say: "Prevent the superfluous initial load of a blank document if we're
> going to load something other than about:blank."
Ok, will correct.
> What does this have to do with this bug?
Yeah... I forgot to remove that before uploading the patch. It's related to (or actually the reason for) bug 879745. Panorama is active in the background and causes an uninterruptible reflow because iframes can do that :/
Assignee | ||
Comment 8•11 years ago
|
||
Instead of landing bug 879745 and losing actual test coverage for Panorama, I think we should just kill the TabView iframe before testing reflows when opening new tabs. That change is a little more self-contained and we can just remove it when Panorama goes away. We can thus test tabopen reflows that the majority of people not using Panorama will see.
FTR, I looked into why Panorama is causing reflows and without refactoring Panorama to not update its UI while it's hidden, there is not much we can do about this, unfortunately.
Attachment #759027 -
Flags: review?(dao)
Assignee | ||
Comment 9•11 years ago
|
||
We should include the one fix for tabitems.js that removes the "._tabViewTabItem" from tabs.
Attachment #759027 -
Attachment is obsolete: true
Attachment #759027 -
Flags: review?(dao)
Attachment #759028 -
Flags: review?(dao)
Comment 10•11 years ago
|
||
Comment on attachment 759028 [details] [diff] [review]
part 3 - kill Panorama's iframe before testing tabopen reflows, v2
I'd rather just add it to the list of known reflows.
Attachment #759028 -
Flags: review?(dao) → review-
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #10)
> I'd rather just add it to the list of known reflows.
The problem with adding it to the list of known reflows is that it actually always reflows. So if we're trying to get rid of more reflows in the future we can test that locally but if we run tests on our test servers there will always be the TabView reflow overriding all other achievements :/
Comment 12•11 years ago
|
||
As I understand from Tim, panorams causes reflows on new tab only if panorama was used during the current browser session, and it happens that the panorama test is executed before the reflows test.
Maybe we could just make sure they run in opposite order?
Comment 13•11 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > I'd rather just add it to the list of known reflows.
>
> The problem with adding it to the list of known reflows is that it actually
> always reflows. So if we're trying to get rid of more reflows in the future
> we can test that locally but if we run tests on our test servers there will
> always be the TabView reflow overriding all other achievements :/
Sorry, I don't understand what this means.
Assignee | ||
Comment 14•11 years ago
|
||
The reflow test does only fail if Panorama has been loaded before. So if the reflow test would be run before the tabview test suite, we'd be fine. That doesn't sound like a great solution, though.
I think I'll go with adding it to the list of known reflows to get this landed. We can figure out how to deal with tabview fallout later.
Assignee | ||
Updated•11 years ago
|
Attachment #759028 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/fa60aab7fa57
https://hg.mozilla.org/integration/fx-team/rev/73483f4906a7
Pushed including a small test fix for browser_bug678392.js that turned OS X orange.
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fa60aab7fa57
https://hg.mozilla.org/mozilla-central/rev/73483f4906a7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 17•11 years ago
|
||
Not sure to understand what needs to be documented here.
Tetsuharu, could you be a bit more specific?
Flags: needinfo?(saneyuki.s.snyk)
Assignee | ||
Comment 18•11 years ago
|
||
We should document the new 'nodefaultsrc' attribute for <xul:browser> elements.
Comment 19•11 years ago
|
||
(In reply to Jeremie Patonnier :Jeremie from comment #17)
> Not sure to understand what needs to be documented here.
> Tetsuharu, could you be a bit more specific?
I think ttaubert's comments too.
Flags: needinfo?(saneyuki.s.snyk)
You need to log in
before you can comment on or make changes to this bug.
Description
•