Closed Bug 980366 Opened 6 years ago Closed Last year

removing "windows touch" from the file menu if fxmetro not supported

Categories

(Firefox :: Menus, defect)

x86
Windows 8.1
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox28 --- affected
firefox29 --- affected
firefox30 --- affected

People

(Reporter: kjozwiak, Unassigned)

References

Details

(Whiteboard: [triage] p=0)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
This hides the menu item using the new checks we added in bug 969831.
Assignee: nobody → jmathies
Attachment #8386852 - Flags: review?(gavin.sharp)
Component: General → Menus
Product: Firefox for Metro → Firefox
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+
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?
(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.
> 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
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.
(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.
Attached patch patch v.2Splinter Review
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 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+
Assignee: jmathies → nobody
We never shipped the metro support, closing!
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.