Closed
Bug 980366
Opened 10 years ago
Closed 6 years ago
removing "windows touch" from the file menu if fxmetro not supported
Categories
(Firefox :: Menus, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: kjozwiak, Unassigned)
References
Details
(Whiteboard: [triage] p=0)
Attachments
(1 file, 1 obsolete file)
4.53 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
We should remove the "Relaunch in Nightly for Windows 8 Touch" from the file menu if fxmetro is not supported on the target machine. Steps to reproduce the issue: 1) Install the latest Nightly 2) Disable acceleration via the options panel 3) Reset the 'gfx.direct3d.last_used_feature_level_idx' pref under about:start 4) Change HKEY_CURRENT_USER/SOFTWARE/Mozilla/Firefox/MetroD3DAvailable = 0 5) You'll notice that the "Windows Touch" portion under the Australis Option Panel is removed 5) Enable the "Menu Bar" and you'll notice that Relaunch in Nightly for Windows 8 Touch is still visible Current Behavior: - "Relaunch in Nightly for Windows 8 Touch" is being displayed even though fxmetro is not supported on the target machine. Expected Behavior: - If the computer doesn't support fxmetro, all instances of "Switch to Windows 8 Touch" should be removed.
Comment 1•10 years ago
|
||
This hides the menu item using the new checks we added in bug 969831.
Assignee: nobody → jmathies
Attachment #8386852 -
Flags: review?(gavin.sharp)
Updated•10 years ago
|
Component: General → Menus
Product: Firefox for Metro → Firefox
Comment 3•10 years ago
|
||
Comment on attachment 8386852 [details] [diff] [review] patch >diff -r 3c62bf9a86f4 browser/base/content/browser.js >+function updateSwitchToMetroVisibility() { >+#ifdef HAVE_SHELL_SERVICE >+#ifdef XP_WIN >+#ifdef MOZ_METRO >+ if (PrivateBrowsingUtils.isWindowPrivate(window) || >+ !Services.metro || !Services.metro.supported) { >+ document.getElementById("menu_switchToMetro").hidden = true; >+ } >+#endif It's impossible for Services.metro to be null, right? Either it will be there or it will throw, and it shouldn't throw #ifdef MOZ_METRO. So that suggests removing this null check. Bug 969831's patch has a few cases of this too, looks like... #ifdef HAVE_SHELL_SERVICE is also kind of a weird ifdef to have here - is it really necessary? (I'm assuming you've tested this in both relevant scenarios - I haven't)
Attachment #8386852 -
Flags: review?(gavin.sharp) → review+
Comment 4•10 years ago
|
||
The beta patch in bug 969831 is confusing. It looks like this patch is adding code to trunk that bug 928068 originally added only to beta?
Comment 5•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #4) > The beta patch in bug 969831 is confusing. It looks like this patch is > adding code to trunk that bug 928068 originally added only to beta? As far as I can tell the menu bar item never had a updateSwitchToMetroVisibility check associated with it on mc. But we had this check for the fx button entry on beta, which didn't cover the menu item. We missed the window for beta anyway so I guess we roll out with what we have there. I'm going to concentrate on getting things cleaned up on mc/aurora.
Updated•10 years ago
|
Comment 6•10 years ago
|
||
> It's impossible for Services.metro to be null, right? Either it will be > there or it will throw, and it shouldn't throw #ifdef MOZ_METRO. So that > suggests removing this null check. Bug 969831's patch has a few cases of > this too, looks like... Shouldn't it be null/undefined on non-windows platforms? http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Services.jsm#88
Comment 7•10 years ago
|
||
Well, sure, but your use of it is in a #ifdef XP_WIN. You can keep the null check or the ifdef, but both at the same time seems wrong.
Comment 8•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #7) > Well, sure, but your use of it is in a #ifdef XP_WIN. You can keep the null > check or the ifdef, but both at the same time seems wrong. Ok, I see your point. Will update the patch and test.
Comment 9•10 years ago
|
||
Updated per comments. Not sure why we had that ifdef HAVE_SHELL_SERVICE in here, that gets set based on the toolkit which includes windows - http://mxr.mozilla.org/mozilla-central/source/browser/base/moz.build#26 Since everything is wrapped in XP_WIN we should be ok without it.
Attachment #8386852 -
Attachment is obsolete: true
Attachment #8387510 -
Flags: review?(gavin.sharp)
Comment 10•10 years ago
|
||
Comment on attachment 8387510 [details] [diff] [review] patch v.2 >diff -r a58b12bc12b5 browser/base/content/browser-menubar.inc >-#ifdef HAVE_SHELL_SERVICE > #ifdef XP_WIN > #ifdef MOZ_METRO > <menuitem id="menu_switchToMetro" > label="&switchToMetroCmd2.label;" >diff -r a58b12bc12b5 browser/base/content/browser.js > function _checkDefaultAndSwitchToMetro() { >-#ifdef HAVE_SHELL_SERVICE > #ifdef XP_WIN > #ifdef MOZ_METRO > let shell = Components.classes["@mozilla.org/browser/shell-service;1"]. > getService(Components.interfaces.nsIShellService); > function SwitchToMetro() { >-#ifdef HAVE_SHELL_SERVICE > let shell = Components.classes["@mozilla.org/browser/shell-service;1"]. These ones actually make sense - checkDefaultAndSwitchToMetro and SwitchToMetro use the shell service. It's still redundant in practice since XP_WIN implies HAVE_SHELL_SERVICE (and I don't see that changing), as you say, but it's probably best to leave it there just for clarity's sake? Other changes look good.
Attachment #8387510 -
Flags: review?(gavin.sharp) → review+
Updated•10 years ago
|
Assignee: jmathies → nobody
Comment 11•6 years ago
|
||
We never shipped the metro support, closing!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•