Last Comment Bug 662261 - [Mac] When trying to choose a helper app, the app bundle expands
: [Mac] When trying to choose a helper app, the app bundle expands
Status: VERIFIED FIXED
: regression, useless-UI, ux-implementation-level
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- major (vote)
: mozilla7
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 643576
  Show dependency treegraph
 
Reported: 2011-06-06 06:37 PDT by Jesse Ruderman
Modified: 2011-08-11 04:40 PDT (History)
7 users (show)
mounir: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
fixed


Attachments
Work around the preference (979 bytes, patch)
2011-06-06 10:31 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Remove the pref and fallback to 0 (979 bytes, patch)
2011-06-06 10:59 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Remove the pref and fallback to 0 (2.77 KB, patch)
2011-06-06 11:01 PDT, Mounir Lamouri (:mounir)
jaas: review+
christian: approval‑mozilla‑aurora+
mounir: checkin+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-06-06 06:37:53 PDT

    
Comment 1 Jesse Ruderman 2011-06-06 06:39:20 PDT
Steps to reproduce:
1. http://www.squarefree.com/bug662261/x.test
2. Click "Choose..."
3. Click TextEdit.app

Expected: the "Open" button should now be enabled

Result: the app bundle has been expanded, showing a "Contents" subfolder.


Regression: Bug is present on trunk (7) and aurora (6), but not beta (5).

Impact: Combined with bug 589176, this makes the "Open with helper app" feature entirely useless for people who don't know how to dig into an .app bundle and find the right executable. So somewhere between ux-implementation-level and useless-UI.

(And maybe the initial folder also stopped defaulting to /Applications/ like it should?)
Comment 2 Boris Zbarsky [:bz] (TPAC) 2011-06-06 09:17:35 PDT
So this is really weird....  I can reproduce in my normal browsing profile, but not in a clean profile using the same nightly.

Jesse, do you see this in a clean profile too?
Comment 3 Boris Zbarsky [:bz] (TPAC) 2011-06-06 09:27:34 PDT
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a45b5cf1d625&tochange=2a0fbd6eedbd

Looks like a regression from bug 656260.
Comment 4 Boris Zbarsky [:bz] (TPAC) 2011-06-06 09:38:13 PDT
The pref that causes the problem for me is "filepicker.lastTypeIndex", which is set to "1".

This is set because at some point I saved a web page and chose "HTML only".

And then nsFilePicker::GetFilterList() is just broken:

548   if (mFilters.Length() <= (PRUint32)mSelectedTypeIndex) {
549     return nil;
550   }

so if mFilters.Length() == 1 in my case (as it is for the app picker, which just has the filterApps filter), GetFilterList will incorrectly return nil instead of the actual single filter desired.  And then |filters| is null in the code bug 656260 added, and things break.
Comment 5 Mounir Lamouri (:mounir) 2011-06-06 10:27:36 PDT
This bug actually appeared with bug 643576 but was hidden because of a very old bug fixed by bug 656260.

I'm going to attach a patch that fixes the issue but I would prefer to just remove "filepicker.lastTypeIndex" preference which is only used on MacOS and doesn't seem useful: you might want to select the first item of a filter list sometimes and the second some other times and the preference will try to default to the wrong one each time...
Josh, would you be okay if I remove this 'feature'?
Comment 6 Mounir Lamouri (:mounir) 2011-06-06 10:31:09 PDT
Created attachment 537584 [details] [diff] [review]
Work around the preference
Comment 7 Boris Zbarsky [:bz] (TPAC) 2011-06-06 10:31:50 PDT
So what does this preference do?  Persist the last-selected filter index?

For the specific case of "html only" vs "complete", do we remember the chosen value elsewhere?
Comment 8 Mounir Lamouri (:mounir) 2011-06-06 10:47:57 PDT
(In reply to comment #7)
> So what does this preference do?  Persist the last-selected filter index?
> 
> For the specific case of "html only" vs "complete", do we remember the
> chosen value elsewhere?

Yes, but I think it's done by toolkit/content/contentAreaUtils.js

Except if this code doesn't work for MacOS X, it looks like we do that twice.
Comment 9 Mounir Lamouri (:mounir) 2011-06-06 10:59:14 PDT
Created attachment 537590 [details] [diff] [review]
Remove the pref and fallback to 0

I prefer this patch that removes the cause of the failure and also falls back to 0 (and warns) when the index is out of range. I believe it's a saner fallback than fallback to no filter.
Comment 10 Mounir Lamouri (:mounir) 2011-06-06 11:01:00 PDT
Created attachment 537591 [details] [diff] [review]
Remove the pref and fallback to 0

Sorry, that was the wrong file.
Comment 11 Josh Aas 2011-06-13 23:43:00 PDT
Comment on attachment 537584 [details] [diff] [review]
Work around the preference

Canceling review here because it seems like you prefer the other patch and I do too.
Comment 12 Mounir Lamouri (:mounir) 2011-06-15 08:43:49 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/97b2a9eeab9c
Comment 13 christian 2011-06-16 15:01:56 PDT
Comment on attachment 537591 [details] [diff] [review]
Remove the pref and fallback to 0

Approved for mozilla-aurora
Comment 14 Mounir Lamouri (:mounir) 2011-06-17 09:52:23 PDT
Pushed to aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/aad654481c86
Comment 15 Vlad [QA] 2011-08-11 04:40:17 PDT
Setting resolution  to Verified Fixed on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0 beta 5

The open button is enabled.

Note You need to log in before you can comment on or make changes to this bug.