1014242 - Distribution handling is triggered by ActivityChooserModel during initial startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

This needs to wait for the distribution to be loaded.

05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.distribution.Distribution.getDistributionFile(Distribution.java:209)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.widget.ActivityChooserModel.readHistoricalDataImpl(ActivityChooserModel.java:1064)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.widget.ActivityChooserModel.ensureConsistentState(ActivityChooserModel.java:713)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.widget.ActivityChooserModel.getDistinctActivityCountInHistory(ActivityChooserModel.java:683)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.widget.GeckoActionProvider.onCreateActionView(GeckoActionProvider.java:94)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.widget.GeckoActionProvider.getView(GeckoActionProvider.java:115)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.menu.GeckoMenuItem.getActionView(GeckoMenuItem.java:104)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.menu.GeckoMenu.onShowAsActionChanged(GeckoMenu.java:503)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.menu.GeckoMenuItem.setActionProvider(GeckoMenuItem.java:228)
05-21 14:31:43.818 I/GeckoDistribution(15357): 	at org.mozilla.gecko.BrowserApp.onCreateOptionsMenu(BrowserApp.java:2227)

Comment 1

[Richard Newman [:rnewman]]

Attachment: Distribution handling is triggered by ActivityChooserModel during initial startup. v1

Not yet cleaned up.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Delay loading of distribution quickshare lists.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8426715 [details] [diff] [review]
Delay loading of distribution quickshare lists.

Review of attachment 8426715 [details] [diff] [review]:
-----------------------------------------------------------------

ActivityChooserModel is stolen mostly from Android, but at this point I'm happy to break that entirely if we need to.

The biggest question I have here, this code is mostly called from a function called ensureConsistentState designed to make sure that the ActivityChooserModel's internal state matches the stored history file. With this patch that no longer is true I think? i.e. calling ensureConsistentState could kick off an async task so that our internal state won't match whats eventually in the fileWhat are the implications there? I'm still thinking my way through, but I'm going to r- in the meantime. If you've thought about it and feel good about this, leave a note :)

(Aside, this comment doesn't seem to be true? http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/distribution/Distribution.java#468. i.e. the runnable isn't "discarded" if the dist is already set up?)

Comment 4

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #3)

> The biggest question I have here, this code is mostly called from a function
> called ensureConsistentState designed to make sure that the
> ActivityChooserModel's internal state matches the stored history file. With
> this patch that no longer is true I think?

I've thought about it some. :)

Essentially this change means "state will become consistent shortly", rather than "state will become consistent when this method returns".

Note that the behavior is *only* different if there's no file to load -- if the history file exists, we'll do the same as always, loading it synchronously.

There's no meaningful way for us to asynchronously load a distribution and fulfill the old contract of ensureConsistentState (unless we want to block the relevant thread for a few seconds, of course), but that'll kill our background throughput.

Yes, that means the activity chooser or menu might not reflect distribution-sourced values on first run.

The alternative is that we chain that load during startup, making sure it's done before BrowserApp.onCreateOptionsMenu. But that's already very early in startup.

So long as the ActivityChooserModel is safe to update after initial startup, things should just work.


> (Aside, this comment doesn't seem to be true?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> distribution/Distribution.java#468. i.e. the runnable isn't "discarded" if
> the dist is already set up?)

Good catch! Comment is obsolete.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #4)

> Good catch! Comment is obsolete.

Comment fixed in a follow-up.

Wes, thoughts on how to move forward with this?

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8426715 [details] [diff] [review]
Delay loading of distribution quickshare lists.

Review of attachment 8426715 [details] [diff] [review]:
-----------------------------------------------------------------

OK. Yeah, I think we're screwed here a bit. But given we're trying to update these at runtime, having them incorrect initially seems like par for the course.

Comment 7

[Richard Newman [:rnewman]]

I tested this by adding distribution/quickshare/history.xml to the downloaded zip.

<historical-records>
  <historical-record activity="com.android.mms/com.android.mms.ui.ConversationComposer"
                     time="0"
                     weight="1"/>
</historical-records>


After killing and restarting, I get the Bluetooth icon as a default share intent. I think that's now expected behavior.

Comment 8

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/17f004909902

Filed Bug 1021176 to try to improve the first-run experience.

Comment 9

[Richard Newman [:rnewman]]

Testing note: you'll see

  Running post-distribution task: quickshare

every time Firefox loads, *until you use quickshare*. We only flush the history file when you use it, and so otherwise it'll always reach into the distro.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/17f004909902