Just reinstalled my Windows 7 desktop, which has the monitor in portrait mode. On first-run of Firefox, the window fills about 4/5 of the screen width and the entire screen height, which seems excessive.

The default size is set in browser.js's BrowserStartup()... http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1322 1322 // Set a sane starting width/height for all resolutions on new profiles. 1323 if (!document.documentElement.hasAttribute("width")) { 1324 let defaultWidth = 994; 1325 let defaultHeight; 1326 if (screen.availHeight <= 600) { 1327 document.documentElement.setAttribute("sizemode", "maximized"); 1328 defaultWidth = 610; 1329 defaultHeight = 450; 1330 } 1331 else { 1332 // Create a narrower window for large or wide-aspect displays, to suggest 1333 // side-by-side page view. 1334 if (screen.availWidth >= 1600) 1335 defaultWidth = (screen.availWidth / 2) - 20; 1336 defaultHeight = screen.availHeight - 10; 1337 #ifdef MOZ_WIDGET_GTK2 1338 // On X, we're not currently able to account for the size of the window 1339 // border. Use 28px as a guess (titlebar + bottom window border) 1340 defaultHeight -= 28; 1341 #endif 1342 } 1343 document.documentElement.setAttribute("width", defaultWidth); 1344 document.documentElement.setAttribute("height", defaultHeight); 1345 } My screen is 1200x1920 (wxh), so it only hits the case where the height is 10 pixels less than the screen. It would also be nice to adjust the default *position* so that the window doesn't overlap the first column of desktop icons on the left (Windows) or right (OS X), not sure if there are additional complications with doing that.

Would something like this help? >if (screen.availWidth >= 1600) > defaultWidth = (screen.availWidth / 2) - 20; > if (screen.availHeight > screen.availWidth) { > defaultHeight = (screen.availHeight / 2) - 20; > } > else { > defaultHeight = screen.availHeight - 10; > } If you can point me to the code where default position is determined, I can take a look at that too.

jphan expressed interest in working on this bug.

How would I go about reproducing this bug short of uninstalling my installation of Firefox? I tried deleting localstore.rdf but it only gave me the bug for a fraction of a second before it resized to what I'm assuming is the default window size for landscape mode for my resolution (1920x1200).

I'd also like to note that I did set my display to portrait mode before deleting "localstore.rdf". jphan9 and I are working on this.

You could try creating a new profile - start the Firefox executable with the command line argument -P and create a new profile from the dialog that appears before the browser starts up.

Thanks, that's exactly what I needed to do.

What file should I be looking at? I found browser.js in two places: C:\Users\Marcos\mozilla-central\obj-i686-pc-mingw32\dist\bin\chrome\browser\content\browser C:\Users\Marcos\mozilla-central\browser\base\content My guess is that it's the one in the second directory (mozilla-central\browser) according to what I've been understanding in the guides.

obj-i686-pc-mingw32 is your build directory, so any changes you make to browser.js inside of it will be overwritten next time you build.

Would it be okay to include some minor calculations that include multiplication to calculate how much of the screen the browser takes up? I'm not too sure, but it's my understanding that performing floating point multiplication costs are somewhat high for such a trivial calculation. For example, instead of defaultHeight = screen.availHeight - 10; I would do let defaultHeight = screen.availHeight * 1.3; in order to obtain a defaultHeight that takes up a percentage of the screen (as opposed to just obtaining a fixed size).

I changed the way default window sizes are calculated based on percentages of screen estate available. I also coupled the height and width sizes so that the window always keeps the same aspect ratio to prevent awkward behavior in extreme cases (i.e. triple monitor setups). However, I'm not sure what the code below is for (what is "X"?), lines 1362-1366 in original code: 1362 #ifdef MOZ_WIDGET_GTK2 1363 // On X, we're not currently able to account for the size of the window 1364 // border. Use 28px as a guess (titlebar + bottom window border) 1365 defaultHeight -= 28; 1366 #endif

Comment on attachment 619182 [details] [diff] [review] First changes on how the window sizes are calculated. I don't think my patch was very good; it's intent was just too far off of what was requested in the first place. jphan9 isn't working on this patch anymore either. It's best if this bug is reset to open to allow someone else the opportunity to work on it.

Ok, thanks for notifying us!

Is the "let defaultWidth = 994; let defaultHeight;" sane? can we use some expression related to availWidth and availHeight instead of a fixed value?

Comment on attachment 701336 [details] [diff] [review] Patch proposal Review of attachment 701336 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +1141,5 @@ > + // For portrait monitors > + // Select a shorter window to suggest top-and-bottom page view > + else if (screen.availWidth <= screen.availHeight) { > + defaultWidth = screen.availWidth * .9; > + defaultHeight = defaultWidth * .75; For top/bottom viewing, shouldn't defaultHeight be closer to 50%? With this it will be 67.5%. @@ +1152,5 @@ > + } > + // For normal monitors > + else { > + defaultWidth = screen.availWidth * .9; > + defaultHeight = screen.availHeight * .9; I think 90% and maximized are too close in relative sizes. If the code reaches here then it is a restored window, and we should make it more obvious. Something like 75% x 75% may be better here. @@ +1159,4 @@ > #ifdef MOZ_WIDGET_GTK2 > // On X, we're not currently able to account for the size of the window > // border. Use 28px as a guess (titlebar + bottom window border) > defaultHeight -= 28; I don't think we will always want to hit this code. As I'm reading the code right now, the defaultHeight will be changed even for maximized windows. I'm curious to hear what you think about this. If we do decide to keep this change, then you will have to un-indent this code so it is more obvious that it will be hit by all code paths.

Similar to previous patch, except incorporating the feedback from jaws on last proposal. Differences: --Now uses 75% x 75% for normal monitors, instead of previous 90% x 90%, per jaws' suggestion --The GTK window height reduction has been returned to the ELSE block, so that it does not apply for tiny monitors NOTE: --For 3x4 and 10x16 portrait monitors, the actual height would factor to about 50% and about 42%, respectively; not 67.5%, since monitors aren't square. No change was made here.

Comment on attachment 703531 [details] [diff] [review] Patch v2 from Brandon Waterloo Review of attachment 703531 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. Dolske, what do you think?

Comment on attachment 703531 [details] [diff] [review] Patch v2 from Brandon Waterloo Review of attachment 703531 [details] [diff] [review]: ----------------------------------------------------------------- Blech. I keep going back-and-forth on this. First-run UX is pretty important, and there are various edge cases in both the before/after versions of this code. But I think we can get a net improvement without trying to address all the existing issues. :) This patch is basically there, just have the one comment and a style nit... I hate to do style nits -- especially on a day when there are blog posts about best review practices! -- but I am going to anyway... :) The comment lines in between the if-else clauses make it really hard to read. I'd generally suggest either a single comment block at the top, moving the comment lines inside the blocks, or in just remove them entirely (in this case, the code is fairly simple and thus self-documenting). ::: browser/base/content/browser.js @@ +1149,5 @@ > + // For portrait monitors > + // Select a shorter window to suggest top-and-bottom page view > + if (screen.availWidth <= screen.availHeight) { > + defaultWidth = screen.availWidth * .9; > + defaultHeight = defaultWidth * .75; I don't think this makes sense, it creates a very squat (landscape ratio) window. The existing comment implies the landscape intent is side-by-side, although I think that's debatable. I think the stronger argument is that webpages tend to be designed for up-down scrolling, and not left-right; ergo a really wide browser just wastes space. But same net outcome. For portrait-mode usage, which is relatively rare, I think the likely user intent is to get more vertical space to reduce up-down scrolling (see previous), and not to have top-and-bottom views. The problem is that defaulting to the entire height (as we currently do) feels like too much of a good thing. :) Simply changing this line to: defaultHeight = screen.availHeight * .75; would accomplish the goal of this bug as-filed.

New patch including dolske's feedback. Changes default portrait height from 75% times the default width (so, .75 * .9 * available width) to just use simply 75% of available height. Moves comment lines to beginning of block for readability.

Comment on attachment 704354 [details] [diff] [review] New patch including dolske's comments Review of attachment 704354 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. I think this is ready for checkin, but I think it would be good to let Dolske take a look at it since he's expressed his opinion in this bug.

Comment on attachment 704354 [details] [diff] [review] New patch including dolske's comments Review of attachment 704354 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +1159,1 @@ > defaultWidth = (screen.availWidth / 2) - 20; We should consider adjusting this too. 1600px / 2 - 20 = 780px, which is too narrow for many web pages, which require at least 1024px wide viewports, e.g. the 960px-wide grid pattern plus gutter space. (google.com requires ~1000px; nytimes.com requires 987px.) I'd suggest replacing 1600 with 2048px. This shouldn't block the landing of this patch, of course.

This new patch includes dolske's previous comments (changing meaning of "widescreen" to mean >= 2048, not >= 1600, because most websites expect a horizontal resolution of ~1000). Additionally, rearranged some comments to make more sense and actually be ordered like the file.

Comment on attachment 712602 [details] [diff] [review] New patch with dolske's new comments Review of attachment 712602 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +1283,5 @@ > + defaultHeight = screen.availHeight * .75; > + } > + // Ensure no dimensions exceed available space > + defaultWidth = Math.min(defaultWidth, screen.availWidth); > + defaultHeight = Math.min(defaultHeight, screen.availHeight); In which cases are these Math.min lines needed? It looks mathematically impossible for this to ever occur. The rest of the patch looks fine.

Comment on attachment 712602 [details] [diff] [review] New patch with dolske's new comments Yeah, the Math.min() is no longer needed in this version of the patch. r+ for landing without that! Thanks! (And sorry for the review delay.)

This version of the patch removes the unnecessary Math.min() lines.

Pushed to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0528e2561bde Thanks for working on this Brandon! :)

Backed out because this caused browser_social_chatwindow.js to start failing permanently on the Mac slaves (e.g. https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=957ede94acc3). Not really your fault; we'll need to either fix or disable the busted parts of that test - this was just the most expedient/safest way to get inbound green again. https://hg.mozilla.org/integration/mozilla-inbound/rev/a20345bfe817

Looks like Win7 also changed size, and triggered failures in mochitest-1 (test_text_selection.html), mochitest-4 (test_plugin_mouse_coords.html), mochitest-5 (test_videocontrols.html), and probably browser-chrome (browser_newtab_focus.js), though it's harder to tell about browser-chrome because we only managed to find a slave and get around to running it once during the 12 rounds while your patch was in.

The browser_social_chatwindow.js test failure is a (relatively minor) bug in the chat window code that only happened to be seen once this new screen size was in place - and FWIW, I did manage to repro (and fix) the problem using this patch and a VM with an artificially restricted screen-size. I've opened bug 840832 for this and uploaded a patch. I've pushed the patch in this bug, plus the patch in that bug, to a try run at https://tbpl.mozilla.org/?tree=Try&rev=ffb3e2b9a30d, which will hopefully show no more failures in browser_social_chatwindow.js. I haven't marked this bug as depending on that new bug as that seems a little unfair, but obviously someone else can do that if desired.

(In reply to Mark Hammond (:markh) from comment #30) > I haven't marked > this bug as depending on that new bug as that seems a little unfair, but > obviously someone else can do that if desired. Thanks for investigating this. Since we can't push this bug's patch until the chat code and test are fixed, setting the dependency will at least help us track the status. No rush, no blame. Let's just do it right. :)

Looks like there's also extreme unhappiness in Win7 mochitest-other - the only one that's finished so far was https://tbpl.mozilla.org/php/getParsedLog.php?id=19686608&tree=Mozilla-Inbound, but there are a couple others which are still running after 5 or 5.5 hours, when it's generally a 30-45 minute suite, so something's hanging in a rather unpleasant way.

Repushed to tryserver to see what else is still broken: https://tbpl.mozilla.org/?tree=Try&rev=20697cb862ad

A more comprehensive try push to make sure that it is ready to land, https://tbpl.mozilla.org/?tree=Try&rev=a69bc2287749

The try push looks good, no failures seem related to the new window size. I've pushed this to inbound, and will monitor the results there. https://hg.mozilla.org/integration/mozilla-inbound/rev/65f79a842388