Closed Bug 490189 Opened 11 years ago Closed 10 years ago

Fennec should offer to be the default browser on Windows Mobile

Categories

(Firefox for Android Graveyard :: General, defect)

All
Windows Mobile 6 Professional
defect
Not set

Tracking

(fennec1.0a4-wm+)

VERIFIED FIXED
Tracking Status
fennec 1.0a4-wm+ ---

People

(Reporter: blassey, Assigned: alexp)

References

Details

(Keywords: uiwanted)

Attachments

(1 file, 10 obsolete files)

I don't think we should have the model dialog like firefox.  The notification bar would work, or we could add something to the firstrun/start page
tracking-fennec: --- → 1.0b1-wm+
Component: General → Windows Mobile
QA Contact: general → mobile-windows
Assignee: nobody → alex.mozilla
A couple questions "To Whom It May Concern":
1. Do we need to store the original settings (for example Opera being the default browser on the Touch Pro), and an option to restore that?
2. What is the decision on the UI? Should it be a dialog or some kind of a notification bar?
Attached patch DefaultBrowser methods (obsolete) — Splinter Review
Proposed change to nsIPhoneSupport: methods isDefaultBrowser() and setDefaultBrowser() to be called from JS code.
Attachment #405181 - Flags: review?
(In reply to comment #1)
> A couple questions "To Whom It May Concern":
> 1. Do we need to store the original settings (for example Opera being the
> default browser on the Touch Pro), and an option to restore that?

I'd vote to not remember the previous default and just remove the registry key, allowing some other browser to grab it. But, we should see what Firefox does.

> 2. What is the decision on the UI? Should it be a dialog or some kind of a
> notification bar?

I'm CC'ing Madhava to think about this question. I'd vote to _not_ make this a notification bar since we try to limit those to only webpage issues. The default browser question is probably better handled as a pref, a "click me to set default" popup that disappears after a few seconds, or  a dialog - in that order
(In reply to comment #3)
> I'd vote to not remember the previous default and just remove the registry key,
> allowing some other browser to grab it. But, we should see what Firefox does.

I believe Firefox just sets the keys without storing the original value. But IE on Windows has the ability to set itself as the default browser. I don't think PocketIE can do this, that's why I've got this question. I just noticed while investigating this subject that some Windows Mobile users ask on forums how to restore PocketIE to be the default browser (for example when Opera is pre-installed on their devices). Apparently in some cases PocketIE works better for those people. So maybe we should consider this option.
(In reply to comment #4)
> (In reply to comment #3)
> I believe Firefox just sets the keys without storing the original value. But IE
> on Windows has the ability to set itself as the default browser. I don't think
> PocketIE can do this, that's why I've got this question. I just noticed while
> investigating this subject that some Windows Mobile users ask on forums how to
> restore PocketIE to be the default browser (for example when Opera is
> pre-installed on their devices). Apparently in some cases PocketIE works better
> for those people. So maybe we should consider this option.

Alex - this is a good reason to save the old value. I do worry that the old value could become stale if an update occurs. Maybe I am over-thinking it though
Call nsIPhoneSupport::SetDefaultBrowser() method from the preferences.
Attachment #405900 - Flags: review?(mark.finkle)
Attachment #405900 - Flags: ui-review?(madhava)
Attachment #405900 - Flags: review?(mark.finkle)
Attachment #405900 - Flags: review+
Comment on attachment 405900 [details] [diff] [review]
Default Browser preference setting

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  setDefaultBrowser: function setDefaultBrowser() {
>+    try {
>+      let phone = Cc["@mozilla.org/phone/support;1"].createInstance(Ci.nsIPhoneSupport);
>+      if (!phone.isDefaultBrowser(true))
>+      {
>+        phone.setDefaultBrowser(true);
>+      }

No {} needed around a one-liner
Use phone.isDefaultBrowser(false) - that is what Firefox uses in it's preferences

>diff --git a/locales/en-US/chrome/preferences.dtd b/locales/en-US/chrome/preferences.dtd

>+<!ENTITY defaultBrowser.button                     "Make default">

Use "Make Default"

I wonder if we should check to see if Fennec is already the default and if it is, hide this preference? Like we do with the prefs-language setting.

Asking Madhava for a review and feedback on the above question.

r+ with above changes

If Madhava wants the auto-hide feature, we can add it as a followup
When we become the default browser, should we be killing registry entries that start (for example) the Opera pre-loader, or not?
(In reply to comment #8)
> When we become the default browser, should we be killing registry entries that
> start (for example) the Opera pre-loader, or not?

Sounds like a good idea
Fixed.
Attachment #405900 - Attachment is obsolete: true
Attachment #405936 - Flags: ui-review?
Attachment #405936 - Flags: review+
Attachment #405900 - Flags: ui-review?(madhava)
Attachment #405936 - Flags: ui-review? → ui-review?(madhava)
Attachment #405181 - Attachment description: [Work in progress] DefaultBrowser methods → DefaultBrowser methods
Attachment #405181 - Flags: review? → review?(mark.finkle)
Pref is definitely better than a notification bar.  We have so few that this won't be hard to find.
Comment on attachment 405936 [details] [diff] [review]
Default Browser preference setting

>+              <setting title="&defaultBrowser.title;" type="control">
>+                <button id="prefs-default-browser" label="&defaultBrowser.button;" oncommand="BrowserUI.setDefaultBrowser();"/>
>+              </setting>

Add a description here

>diff --git a/locales/en-US/chrome/preferences.dtd b/locales/en-US/chrome/preferences.dtd

>+<!ENTITY defaultBrowser.title                      "Make &brandShortName; your default browser">
>+<!ENTITY defaultBrowser.button                     "Make Default">

defaultBrowser.title should be "Default Browser"

and add:

defaultBrowser.description "Make &brandShortName; your default browser"
Attachment #405936 - Flags: review+ → review-
Fixed.
Attachment #405936 - Attachment is obsolete: true
Attachment #406114 - Flags: review?(mark.finkle)
Attachment #405936 - Flags: ui-review?(madhava)
It would make sense to add a confirmation before actually making Fennec the default browser.

Another option is to change this setting to boolean, and when it is switched back to "No", restore the previous browser. It raises another issue though. For example, Opera was set as the default browser, then Fennec changed that, what should we do when the user un-selects this option? Restoring Opera is questionable, as it could be uninstalled since then. So it might be better to restore IE as the default, if we go this way.

Any comments?
(In reply to comment #14)

> Any comments?

I don't like the confirmation dialog. But I do like switching to "Reset" if we _are_ the default browser. Then the user can reset the previous default browser. As for which browser to reset, the actual previous browser would be the best choice. IE would be a safe fallback, if needed.

Can we get the path to the binary that was the previous default and then check for it before we reset? If it doesn't exist we could fallback to IE.

Brad & Doug - thoughts on this for WinMo?
Comment on attachment 406114 [details] [diff] [review]
Default Browser preference setting

Alex - This patch is exactly what we talked about, but I think we want to add the code to handle resetting the previous default.

Madhava, is that the case?

If that's not the case, we can use this patch.
Attachment #406114 - Flags: review?(mark.finkle) → review-
Duplicate of this bug: 507984
Comment on attachment 405181 [details] [diff] [review]
DefaultBrowser methods

A couple of comments...

>diff --git a/components/phone/nsPhoneSupport.cpp b/components/phone/nsPhoneSupport.cpp
>--- a/components/phone/nsPhoneSupport.cpp
>+++ b/components/phone/nsPhoneSupport.cpp
>+static SETTING gSettings[] = {
>+  { HKEY_LOCAL_MACHINE, DEFBROWSERREG, "0", APP_REG_NAME},
>+  { HKEY_LOCAL_MACHINE, DEFBROWSERREG, "1", "\"%APPPATH%\""},
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("http", SOP),  "", VAL_OPEN },
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("https", SOP), "", VAL_OPEN },
>+  // Optional
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("file", SOP),  "", VAL_OPEN },
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("ftp", SOP),   "", VAL_OPEN },
>+
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("htmlfile", SOP),  "", VAL_OPEN },
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("xhtmlfile", SOP), "", VAL_OPEN },
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("xmlfile", SOP),   "", VAL_OPEN },
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("xslfile", SOP),   "", VAL_OPEN },
>+  { HKEY_CLASSES_ROOT, MAKE_KEY_NAME1("urlfile", SOP),   "", VAL_OPEN },
url files are protocol shortcuts and Windows Mobile should handle these so it can redirect protocols such as mailto to the appropriate application.

I am personally against trying to restore a previous app as the default handler since trying to do so will create other bugs due to the issues noted previously when trying to accomplish this where we are responsible vs. the other app being responsible for providing this ability.
Perhaps we should at least be able to "restore" Pocket IE, since it it not uninstallable....
I think that would be a decent compromise. btw: the corresponding Firefox bug is bug 255225
(In reply to comment #18)
> url files are protocol shortcuts and Windows Mobile should handle these so it
> can redirect protocols such as mailto to the appropriate application.

So, what is the action item on this then?
Current implementation does handle url files together all other optional types when aClaimAllTypes=TRUE. Do you mean this one has to be mandatory along with http&https?
On the desktop you would never take over this key. The following is the handler for url files on the desktop and I would hope that Microsoft has a similar handler defined in this key for WinMo.
"C:\Windows\System32\rundll32.exe" "C:\Windows\System32\ieframe.dll",OpenURL %l

What is the shell open command value for .url or for the key it references in its default value
(In reply to comment #22)
> What is the shell open command value for .url or for the key it references in
> its default value

HKCR\urlfile\Shell\Open\Command="iexplore.exe -u%1" on WinMo - that's why I assumed we should take over this key as well.
PocketIE may handle it some special way though (looking at the -u switch). I'll try to skip this.
iirc iexplore.exe used to do it this way on the desktop as well. I'd just verify that it properly hands off http, https, etc. to Firefox and mailto, etc. to your default mail app to see if handling .url files needs to be set to something other than the default.
- Added restoreDefaultBrowser() function to restore previous default browser.
- Added handling for additional registry keys, which are overridden by Opera.

TODO:
UI has to be finalized. Current temporary implementation uses two buttons in preferences. Probably have to use <setting type="bool">, but it needs to call isDefaultBrowser() to get its value and setDefaultBrowser() to set it. This may depend on bug 523538.
Attachment #405181 - Attachment is obsolete: true
Attachment #406114 - Attachment is obsolete: true
Attachment #410415 - Flags: review?
Attachment #405181 - Flags: review?(mark.finkle)
Robert:  I think we'd like to consider making the restoration of the previous browser more (or less?) robust in a follow-on bug, if that's alright with you.

alexp:  You set an empty review-flag here.
(In reply to comment #26)
> Robert:  I think we'd like to consider making the restoration of the previous
> browser more (or less?) robust in a follow-on bug, if that's alright with you.
Of course it is.
(In reply to comment #26)
> alexp:  You set an empty review-flag here.

I did. I meant it would be nice if someone could have a look at this latest patch to confirm it's what we need. Doesn't it work like this?

Apart from the TODO note the patch is ready to use. But it will require some minor rework sooner or later at least from the UI point of view.
(In reply to comment #28)
> I did. I meant it would be nice if someone could have a look at this latest
> patch to confirm it's what we need. Doesn't it work like this?
nope, it doesn't work like that unfortunately
Attached patch DefaultBrowser feature (obsolete) — Splinter Review
Changed Default browser preference to a bool setting.
Attachment #410415 - Attachment is obsolete: true
Attachment #410594 - Flags: review?(mark.finkle)
Attachment #410415 - Flags: review?
Depends on: 523538
Summary: Fennec should offer to be the default browser → Fennec should offer to be the default browser on Windows Mobile
Comment on attachment 410594 [details] [diff] [review]
DefaultBrowser feature

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  setDefaultBrowser: function setDefaultBrowser() {

>+  restoreDefaultBrowser: function restoreDefaultBrowser() {

We never use these two methods. Remove them.

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+              <setting id="prefs-default-browser" type="bool" title="&defaultBrowser.title;" onchange="BrowserUI.updateDefaultBrowser(this.value);">
>+                &defaultBrowser.description;
>+              </setting>

Until this feature lands for Maemo, we should wrap this XUL with #ifdef WINCE ... #endif

>diff --git a/components/phone/nsPhoneSupport.cpp b/components/phone/nsPhoneSupport.cpp
>+private:
>+  PRBool    mCheckedThisSession;

1 space between the type and variable is ok

>+#define APP_REG_NAME "Fennec"
>+#define DEFBROWSERREG "Software\\Microsoft\\Shell\\Rai\\:DEFBROWSER"
>+#define DEFBROWSERREG_BACKUP "Software\\Microsoft\\Shell\\Rai"
>+#define VAL_OPEN "\"%APPPATH%\" \"%1\""
>+#define SOP "\\Shell\\Open\\Command"
>+#define BACKUP_VAL_NAME "Before" APP_REG_NAME

Where exactly does this "Before" regkey live in the registry? I don't like putting our keys in someone elses registry key. Also, I like "Backup" better than "Before"
 
>+OpenKeyForReading(HKEY aKeyRoot, const nsAString& aKeyName, HKEY* aKey)

>+  switch (res) {
>+  case ERROR_SUCCESS:
>+    break;

Remove this case?

>+OpenKeyForWriting(HKEY aStartKey, const nsAString& aKeyName, HKEY* aKey)

>+  switch (res) {
>+  case ERROR_SUCCESS:
>+    break;

Same

With Maemo being released ahead of WinMo, we can't have this option be visible in Maemo until real support exists for it.
Attachment #410594 - Flags: review?(mark.finkle) → review-
At this time, I'm not sure that we can do "default browser" on Maemo from within Fennec.  The steps needed to do this require root access, so they'd need to be done by the package manager.
Attached patch DefaultBrowser feature (obsolete) — Splinter Review
- Code review changes
- Use a separate key to backup registry values
Attachment #410594 - Attachment is obsolete: true
Attachment #412991 - Flags: review?(mark.finkle)
Drive by comments that should be addressed before moving forward:

>let phone = Cc["@mozilla.org/phone/support;1"].createInstance(Ci.nsIPhoneSupport);


I think this should be getService.  We don't really need to spin up new instances each time.

> boolean isDefaultBrowser(in boolean aStartupCheck);

I do not think it is a good idea to have this boolean, nor do I see any real reason for it.  Its a gross implementation detail.

> void setDefaultBrowser(in boolean aClaimAllTypes);

I am not really convinced that we need a |aClaimAllTypes| flag.  When would I know when to call one verses the other.  The current usages that we are adding don't use this flag.  I'd say drop it and if we wanted to support subsets of all of the types we can handle, we can do it later.


> +  PRBool mCheckedThisSession;

Because we are doing |createInstance|, this flag is meaningless


+#ifndef MAX_BUF
+#define MAX_BUF 1024
+#endif

Why is this needed?  Is MAX_BUF already defined?



 

Also, i think rstrong might be the best person to review the default settings.
(In reply to comment #34)
> >let phone = Cc["@mozilla.org/phone/support;1"].createInstance(Ci.nsIPhoneSupport);
> 
> 
> I think this should be getService.  We don't really need to spin up new
> instances each time.

Agreed

> > boolean isDefaultBrowser(in boolean aStartupCheck);
> 
> > void setDefaultBrowser(in boolean aClaimAllTypes);

I think Alex just pulled these from the Firefox nsShellService impl. But I agree that we should go minimal to start, so I think we could drop the unused parts.

> > +  PRBool mCheckedThisSession;
> 
> Because we are doing |createInstance|, this flag is meaningless

Also if we drop the aStartupCheck param
tracking-fennec: 1.0b1-wm+ → 1.0a4-wm+
Mark is right - the code was mostly copied from Firefox nsWindowsShellService. Originally I tried to avoid major changes and keep it more or less compatible with Windows implementation in case we'll want to switch to a common shell component, hence have a similar interface.
But as WinMo version is already quite different, and stays separate from desktop shell component, it definitely makes sense to simplify it more.
side note: I plan on investigating using js-ctypes and a js component for the Windows (possibly other platforms as well) shell service sometime in the future.
Attached patch DefaultBrowser feature (obsolete) — Splinter Review
More code review changes:
- Using getService instead of createInstance
- Removed function arguments, which are not used
Attachment #412991 - Attachment is obsolete: true
Attachment #412991 - Flags: review?(mark.finkle)
Attachment #413170 - Flags: review?(mark.finkle)
Attachment #413170 - Flags: review?(robert.bugzilla)
Comment on attachment 413170 [details] [diff] [review]
DefaultBrowser feature

Did a quick once over of nsPhoneSupport.cpp... looks good so far... I'll do a more thorough review tomorrow hopefully

>diff --git a/components/phone/nsPhoneSupport.cpp b/components/phone/nsPhoneSupport.cpp
>--- a/components/phone/nsPhoneSupport.cpp
>+++ b/components/phone/nsPhoneSupport.cpp
>...
>@@ -113,16 +114,304 @@ nsPhoneSupport::SwitchTask()
>...
>+// [HKEY_CLASSES_ROOT\urlfile\Shell\Open\Command]
>+// (Default)=iexplore.exe -u%1
This should be removed since it is no longer set

>...
>+static SETTING gSettings[] = {
>+  { HKEY_LOCAL_MACHINE, DEFBROWSERREG,         "0", APP_REG_NAME,    "Rai_DEFBROWSER_0" },
>+  { HKEY_LOCAL_MACHINE, DEFBROWSERREG,         "1", "\"%APPPATH%\"", "Rai_DEFBROWSER_1" },
>+  { HKEY_CLASSES_ROOT, "http" SHELL_OPEN,      "",  VAL_OPEN,        "http" },
>+  { HKEY_CLASSES_ROOT, "https" SHELL_OPEN,     "",  VAL_OPEN,        "https" },
>+  // Optional
We used to have a flag that specified the key was optional during the default check... you might want to do the same here since they are optional.
http://mxr.mozilla.org/mozilla1.8/source/browser/components/shell/src/nsWindowsShellService.cpp#205

>...
>+void
>+SetRegKey(HKEY aKeyRoot, const nsString& aKeyName, const nsString& aValueName, const nsString& aValue)
I've been meaning to clean up these old names for awhile now... it should actually be something like SetRegValue.

>...
>+DWORD
>+GetRegKey(HKEY aKeyRoot, const nsString& aKeyName, const nsString& aValueName, nsString& aValue)
I've been meaning to clean up these old names for awhile now... it should be something like GetRegValue.

>+NS_IMETHODIMP
>+nsPhoneSupport::IsDefaultBrowser(PRBool* aIsDefaultBrowser)
>+{
>+#ifdef WINCE
>+  *aIsDefaultBrowser = PR_TRUE;
>+
>+  SETTING* settings;
>+  SETTING* end = gSettings + gSettingsCount;
>+
>+  PRUnichar exePath[MAX_BUF];
>+  if (!::GetModuleFileNameW(0, exePath, MAX_BUF))
>+    return NS_ERROR_FAILURE;
>+
>+  nsAutoString appLongPath(exePath);
nit: all paths on winmo are long so just use appPath... same elsewhere

>+
>+  nsresult rv;
>+  PRUnichar currValue[MAX_BUF];
>+  for (settings = gSettings; settings < end; ++settings) {
>+    NS_ConvertUTF8toUTF16 dataLongPath(settings->valueData);
nit: all paths on winmo are long so just use dataPath... same elsewhere
(In reply to comment #39)
> We used to have a flag that specified the key was optional during the default
> check... you might want to do the same here since they are optional.

We decided to remove the option - just always change all the settings, so don't need this flag now.
Attached patch DefaultBrowser feature (obsolete) — Splinter Review
- Updated to the latest code
- Changed the function/variable names as per Robert's comments
Attachment #413170 - Attachment is obsolete: true
Attachment #414880 - Flags: review?(mark.finkle)
Attachment #413170 - Flags: review?(robert.bugzilla)
Attachment #413170 - Flags: review?(mark.finkle)
Attachment #414880 - Flags: review?(robert.bugzilla)
Comment on attachment 414880 [details] [diff] [review]
DefaultBrowser feature

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  updateDefaultBrowser: function updateDefaultBrowser(aSet) {

>+      if (aSet) {
>+        if (!phone.isDefaultBrowser())
>+          phone.setDefaultBrowser();
>+      }
>+      else {
>+        if (phone.isDefaultBrowser())
>+          phone.restoreDefaultBrowser();
>+      }

Is there any point in checking isDefaultBrowser rather than just calling set/restore unconditionally? If so, should set/restore do that themselves rather than relying on callers doing it?

>diff --git a/chrome/content/preferences.js b/chrome/content/preferences.js

>   _delayedInit: function ev__delayedInit() {

>+#ifdef WINCE
>+    let phone = Cc["@mozilla.org/phone/support;1"].getService(Ci.nsIPhoneSupport);
>+    document.getElementById("prefs-default-browser").value = phone.isDefaultBrowser(false);
>+#endif

I guess this depends on it not being possible to have the default browser change other than by being toggled in the UI... is that always true? What happens if the user launches IE and sets it as the default browser while Fennec is running? Not the biggest deal, I guess, but could move this to before the this._list check to have it run each time the panel is selected.

>diff --git a/locales/en-US/chrome/preferences.dtd b/locales/en-US/chrome/preferences.dtd

>+<!ENTITY defaultBrowser.title                      "Default Browser">
>+<!ENTITY defaultBrowser.description                "Make &brandShortName; your default browser">

Not sure how this is going to work. We're string frozen for Maemo 1.0, so I don't think we can land this now, at least not without causing some localizer confusion...

I haven't reviewed any of the nsIPhoneSupport changes (robstrong's a better reviewer for that anyways), but r=me on these chrome parts, with these comments addressed (and assuming you've tested it, because I haven't!).
Attachment #414880 - Flags: review+
I don't have a good suggestion either.

The large gap between maemo and winmo in terms of shipping might bite us again, and short of branching, I don't have anything to constructively add.
Attached patch DefaultBrowser feature (obsolete) — Splinter Review
Code review changes:
- Removed strings from preferences.dtd (will file a follow-up bug to put them back when unfrozen)
- Moved IsDefaultBrowser check from JS to C++
Attachment #414880 - Attachment is obsolete: true
Attachment #415239 - Flags: review?(mark.finkle)
Attachment #414880 - Flags: review?(robert.bugzilla)
Attachment #414880 - Flags: review?(mark.finkle)
[Minor change] Removed extra brackets.
Attachment #415239 - Attachment is obsolete: true
Attachment #415242 - Flags: review?(mark.finkle)
Attachment #415239 - Flags: review?(mark.finkle)
Attachment #415242 - Flags: review?(mark.finkle) → review+
Keywords: checkin-needed
pushed http://hg.mozilla.org/mobile-browser/rev/145b16089e3b
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified FIXED on build:

Mozilla/5.0 (Windows; U; Window3sCE 5.2; en-US; rv:1.9.2b5pre) Gecko/20091202 Namoroka/3.6b5pre Fennec/1.0a4pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Flags: in-litmus? → in-litmus+
Component: Windows Mobile → General
OS: All → Windows Mobile 6 Professional
QA Contact: mobile-windows → general
You need to log in before you can comment on or make changes to this bug.