Closed Bug 1014242 Opened 5 years ago Closed 5 years ago

Distribution handling is triggered by ActivityChooserModel during initial startup

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: rnewman, Assigned: rnewman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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)
Depends on: 1014338
Attachment #8426620 - Attachment is obsolete: true
No longer depends on: 1013684
Attachment #8426715 - Flags: review?(wjohnston)
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?)
Attachment #8426715 - Flags: review?(wjohnston) → review-
(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.
(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?
Flags: needinfo?(wjohnston)
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.
Attachment #8426715 - Flags: review- → review+
Flags: needinfo?(wjohnston)
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.
Blocks: 1021176
https://hg.mozilla.org/integration/fx-team/rev/17f004909902

Filed Bug 1021176 to try to improve the first-run experience.
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.
https://hg.mozilla.org/mozilla-central/rev/17f004909902
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.