Last Comment Bug 773393 - Move filepicker and activity-result code out of GeckoApp so it survives activity destruction
: Move filepicker and activity-result code out of GeckoApp so it survives activ...
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 16
Assigned To: away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
: general
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks: 769269
  Show dependency treegraph
 
Reported: 2012-07-12 12:25 PDT by away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-07-18 21:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - Move GeckoApp inner classes out into their own files (24.16 KB, patch)
2012-07-12 12:25 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Part 2 - Create an ActivityHandlerHelper class (25.84 KB, patch)
2012-07-12 12:26 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Part 3 - Move ActivityHandlerHelper and PromptService to be owned by GeckoAppShell so it survives activity destruction (7.96 KB, patch)
2012-07-12 12:27 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
(1/3) Move GeckoApp inner classes out into their own files (25.59 KB, patch)
2012-07-13 08:24 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review
(2/3) Create an ActivityHandlerHelper class (24.30 KB, patch)
2012-07-13 08:24 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review
(3/3) Move ActivityHandlerHelper over to GeckoAppShell so it survives activity destruction (5.17 KB, patch)
2012-07-13 08:25 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review

Description away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 12:25:58 PDT
Created attachment 641559 [details] [diff] [review]
Part 1 - Move GeckoApp inner classes out into their own files

I started doing this but now my brain is rotting so I don't know if what I'm doing is helping at all. Putting my patches here for now.
Comment 1 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 12:26:35 PDT
Created attachment 641560 [details] [diff] [review]
Part 2 - Create an ActivityHandlerHelper class
Comment 2 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-12 12:27:26 PDT
Created attachment 641561 [details] [diff] [review]
Part 3 - Move ActivityHandlerHelper and PromptService to be owned by GeckoAppShell so it survives activity destruction
Comment 3 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-13 08:24:02 PDT
Created attachment 641909 [details] [diff] [review]
(1/3) Move GeckoApp inner classes out into their own files
Comment 4 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-13 08:24:37 PDT
Created attachment 641910 [details] [diff] [review]
(2/3) Create an ActivityHandlerHelper class
Comment 5 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-13 08:25:12 PDT
Created attachment 641911 [details] [diff] [review]
(3/3) Move ActivityHandlerHelper over to GeckoAppShell so it survives activity destruction
Comment 6 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-07-13 08:35:23 PDT
In part 1, when I moved the classes out, there were some minor changes to the code. I'm listing them here for ease of reviewing:

- AwesomebarResultHandler does GeckoApp.mAppContext.loadRequest(...) instead of loadRequest(...)
- Some of the extracted classes take a SynchronousQueue<String> in the constructor and store it locally since they don't have access to the one in GeckoApp directly anymore
- I moved GeckoApp.mImageFilePath to be CameraImageResultHandler.sImageName and updated usage of it accordingly.

In part 2 when I moved the functions out I modified:

- The showFilePicker functions take an "Activity parentActivity" parameter since I needed something on which to call startActivityForResult.
- A lot of the extracted functions now take a Context parameter since there are a couple of functions that need to be called on it (getString in getFilePickerTitle, getPackageManager in addIntentActivitiesToList).
- Renamed AddFilePickingActivities to addFilePickingActivities (case fixup)
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-13 10:15:39 PDT
Comment on attachment 641909 [details] [diff] [review]
(1/3) Move GeckoApp inner classes out into their own files

Looks a good, clean port. Nice work.
Comment 10 Brian Nicholson (:bnicholson) 2012-07-18 21:11:32 PDT
Landed in http://hg.mozilla.org/releases/mozilla-beta/rev/b2487714085b

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