972208 - Support shipping default quickshare providers in distributions.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I need to look at your tests for this stuff, but this works locally. Distributers can add a history.xml file to their distribution that contains something like:

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

With some of the newer quickshare contexts stuff, we will actually have separate files for different link/mime types. i.e. history-image.xml, history-tel.xml, etc. This will look for them. Its not smart enough to look for history-image.xml and then look for history.xml if it can't find that. I'm not sure if we want that or not.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8382820 [details] [diff] [review]
Patch v1

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

I need to spend more time looking at this, but here are a few comments I have so far.

::: mobile/android/base/Distribution.java
@@ +298,5 @@
>          return descFile;
>      }
>  
> +    public File getFile(String filename) {
> +        return getDistributionFile(filename);

Why not just make getDistributionFile public?

@@ +330,5 @@
> +        }
> +        return null;
> +    }
> +
> +    private String getFileContents(String filename) {

What is the purpose of this change? It doesn't seem to have anything to do with quickshare distributions. If this is general refactoring, it should go in a different bug.

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +1059,4 @@
>          } catch (FileNotFoundException fnfe) {
> +            try {
> +                Distribution dist = new Distribution(mContext);
> +                File distFile = dist.getFile(mHistoryFileName);

I think it would be better to just add a new static method to Distribution to get the quickshare providers, but maybe that doesn't work because we'd still need to pass in this file name? I'm not familiar with ActivityChooserModel, so I'd have to look through this more carefully to understand what's going on here.

Comment 3

[Wesley Johnston (:wesj)]

Heh. That code is probably a bit of an artifact of me trying to put in a getQuickShare method. In the end, I decided that I really only wanted a File/FileInputStream and anything else was going to require Distribution to know more about quickshare than I wanted. But I left in the logic consolidating some of that code because removing the duplicated code seemed like a good idea.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8382820 [details] [diff] [review]
Patch v1

r- for the unnecessary changes. Also, I think we should look into making a distribution/quickshare directory for these files, since history.xml is very generic sounding, and someone peeking in the distribution directory would probably get confused by that.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Removed the fluff and added a quickshare directory.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8385676 [details] [diff] [review]
Patch v2

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

r+ with return statement issue addressed.

Also, I did not test this myself, I trust that you have been testing this :) 

For tests, we have a .zip file that pretends to be an APK with distribution contents: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/assets/mock-package.zip

You can update that .zip to contain a quickshare directory, and then you can add a small method to testDistribution that somehow checks to make sure the quickshare items ended up where we want them. It would be nice if you could just query directly into ActivityChooserModel for this.

::: mobile/android/base/widget/ActivityChooserModel.java
@@ -1047,3 @@
>          } catch (FileNotFoundException fnfe) {
> -            if (DEBUG) {
> -                Log.i(LOG_TAG, "Could not open historical records file: " + mHistoryFileName);

I think it would be useful to keep this log statement in if we still couldn't find any file to use.

@@ +1059,4 @@
>          } catch (FileNotFoundException fnfe) {
> +            try {
> +                Distribution dist = new Distribution(mContext);
> +                File distFile = dist.getDistributionFile("quickshare/" + mHistoryFileName);

I'm happy this getDistributionFile method abstracts away dealing with system vs. APK distributions.

@@ -1050,2 @@
>              }
> -            return;

I don't think you meant to get rid of this return statement. You could move the one from the new inner catch statement out to here.

Comment 7

[:Margaret Leibovic]

We should wrap this up and land it.

Comment 8

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/1eddd4bab329

I need to document this: https://wiki.mozilla.org/Mobile/Distribution_Files Also, need to wrap up the tests. But I landed it for now.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1eddd4bab329

Comment 10

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #8)
> https://hg.mozilla.org/integration/fx-team/rev/1eddd4bab329
> 
> I need to document this: https://wiki.mozilla.org/Mobile/Distribution_Files
> Also, need to wrap up the tests. But I landed it for now.

Friendly reminder to add this to the documentation :)

Comment 11

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #10)

> Friendly reminder to add this to the documentation :)

I'll tackle the docs as part of Bug 1014242.