Open Bug 1382588 Opened 7 years ago Updated 2 years ago

Verify that the Photon tab implementation gets rid of all the images in the tab strip that are loaded at startup

Categories

(Firefox :: Theme, enhancement, P3)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

People

(Reporter: johannh, Unassigned)

References

Details

Attachments

(1 file)

Bug 1382588 - Hide scroll buttons and overflow indicators using display:none rather than visibility:collapse to avoid loading their image resources when they're not needed.
59 bytes, text/x-review-board-request
Details
A ton of images loaded at startup (https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/test/performance/browser_startup_images.js#57,75,80,85,90,120,125,130,135,140,146,158,182,187,193,210,214,219,229,234) are due to the current tab strip implementation. It's probably useless to open a bug for every one of them, since there will be major changes happening in the tab strip soon and I assume many of those images will be removed eventually. I'd like to use this bug to track that the changes we're making actually get rid of all these images loaded at startup.
Flags: qe-verify-
Whiteboard: [photon-visual][triage]
Looks like there are still issues on Linux.
I think this was skipped on Windows 7 yet again (and it's not running on Windows 10 at all). The first patch failed locally on my Windows 10 machine, so we might need to change something. First, I'd really like to find out why a whole chunk of tests are skipped on try but run on integration, though. https://treeherder.mozilla.org/logviewer.html#?job_id=122528581&repo=try&lineNumber=1709 Joel, any idea? Also, how can we make this test run on Windows 10 on try?
Flags: needinfo?(jmaher)
I am not clear on which tests are not run on try- is there a manifest with the tests I can see or possibly a job type? It looks like I was linked to mochitest-browser-chrome chunk 1.
Flags: needinfo?(jmaher)
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #7) > I am not clear on which tests are not run on try- is there a manifest with > the tests I can see or possibly a job type? It looks like I was linked to > mochitest-browser-chrome chunk 1. Sorry, I linked the line number but it seems like line number linking is broken for logviewer :) I'm referring to browser/base/content/test/performance/browser_startup_images.js (https://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/browser/base/content/test/performance/browser.ini#9) The line number in the log is 1709 Thank you!
ok, I see this now: TEST-SKIP | browser/base/content/test/performance/browser_startup_images.js | took 0ms and oddly this has |skip-if = !debug|: https://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/browser/base/content/test/performance/browser.ini#10 so it should be running on that debug build. I do see that browser_appmenu_reflows.js is running and that is |skip-if = debug|, so I suspect this is being run as non debug. that is defined in mozinfo.json which lives in common.tests.zip: https://queue.taskcluster.net/v1/task/A3zrOKppReuml2nh9Aawag/artifacts/public/build/target.common.tests.zip inside of mozinfo.json, I see |debug: false| this is the problem! I see a reference to this in bug 1388012, but it appears that something else resolved this between when the bug was filed and when it was investigated- is it possible the root revision on this try push is out of date? It looks to be August 10th m-c rev:253a8560dc34, would it be possible to push to try again? Lastly to get browser chrome running on windows 10 we need to resolve bug 1388512 and bug 1388722, then we can enable bc tests on win10 :)
Thanks for your help! I'll look into bug 1388512 today ;)
Joel, this is still getting skipped (I rebased on today's central): https://treeherder.mozilla.org/logviewer.html#?job_id=123549421&repo=try&lineNumber=1771 (Line 1771) Sorry for nagging you about this. Let me know if there's a more appropriate place/person to complain to :)
Flags: needinfo?(jmaher)
I see this is still a problem :( I downloaded the target.common.tests.zip from your try push and then from the tip of mozilla central: https://public-artifacts.taskcluster.net/VoNlFf2eQvml9YTdfKqusA/0/public/logs/live_backing.log what I see is that mozinfo.json for try has |debug: false| and on mozilla-central |debug: true|. Is there a chance you are pushing with an odd build config? Possibly this is related to pushing to try. :ted, do you know of anything that would set mozinfo.debug=False when pushing to try? Possibly it is using the mozinfo.json from an opt build, or this is some difference in configs?
Flags: needinfo?(jmaher) → needinfo?(ted)
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #13) > I see this is still a problem :( > > I downloaded the target.common.tests.zip from your try push and then from > the tip of mozilla central: > https://public-artifacts.taskcluster.net/VoNlFf2eQvml9YTdfKqusA/0/public/ > logs/live_backing.log > > what I see is that mozinfo.json for try has |debug: false| and on > mozilla-central |debug: true|. > > Is there a chance you are pushing with an odd build config? Possibly this > is related to pushing to try. This happened to dao and me pushing via ./mach try and just now me doing a try build directly from mozreview in bug 1390874 (which will now likely get backed out because of that). We were using really simple try syntax. I don't think it's likely that we've been doing it wrong in all three cases. > :ted, do you know of anything that would set mozinfo.debug=False when > pushing to try? Possibly it is using the mozinfo.json from an opt build, or > this is some difference in configs? It's probably more productive to file a bug on this. I'll do that.
Depends on: 1391172
Let's move to bug 1391172.
Flags: needinfo?(ted)
Attachment #8896167 - Flags: review?(jhofmann) → review?(florian)
Sorry, I'm on PTO now. As I mentioned, I believe this still has issues but I don't have time to look into it any further. I'm forwarding to florian for review (though you might want to cancel it as long as we don't have a green try run for browser_startup_images on Windows 7).
Assignee: dao+bmo → nobody
Status: ASSIGNED → NEW
Attachment #8896167 - Flags: review?(florian)
My impression here is that the patch is probably correct, and given that we can't test reliably on try, someone should try locally on a Windows debug build. Am I missing something?
Flags: needinfo?(dao+bmo)
I'm not sure why browser_startup_images.js would see these images on any platform with the scrollbox.xml fix. My guess is that the tab strip overflows initially because the window is too small and then gets resized.
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #18) > I'm not sure why browser_startup_images.js would see these images on any > platform with the scrollbox.xml fix. My guess is that the tab strip > overflows initially because the window is too small and then gets resized. I've observed that on startup on Linux when the browser window is meant to be fullscreen, it's initially painted once at about 2/3 of the final size, and then resized (with a resize event that seems to come from native platform specific code). I've never observed anything similar on Mac, but if the same resize event happens before first paint, I'll never catch it in my screen recordings. But even with a smaller window, I don't see how the tab strip could overflow when we have a single tab.
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: