1145579 - Open tab queue file when fennec starts or resumes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

When Fennec loads or resumes, read the tab queue file and load tabs for each entry.

Comment 1

[Martyn Haigh (:mhaigh)]

Original scope was a bit too broad for this bug : this bug will track reading the tab queue file when fennec starts or resumes

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Part 1: optimise imports in BrowserApp

Just a bit of housekeeping first.  The imports in BrowserApp have been bugging me for a while, so lets clean them up before we start (we'll be editing BrowserApp later, so this isn't just a random act of tidying)

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Part 2: Store the number of queued tabs in a shared pref

A slight optimisation - this stores the number of queued tabs in a shared pref at the time of writing the file, which means that later when we come to work out if we want to process the tab queue, we don't need to open the file and process it to see if there's any data in there.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Part 3: Read the number of queued tabs from shared pref

Method to read and return the number of tabs queued from the shared pref added in the last patch

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Part 4: add method to determine if we should open any queued tabs

Method used to determine if we should try to process the tab queue based on if there are any tabs queued and if the tab queue is enabled.

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Part 5: add delete file method to gecko profile

Adding a method to delete a file via the gecko profile

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: Part 6: add method to load the tab queue file and cleanup

Here we add the method which will read and process the tab queue.  We also perform some cleanup after the data has been processed.

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Part 7: add calls to process the tab queue data when gecko has finished initialisation and onresume

Here we add hooks in to BrowserApp to process the tab queue when gecko has initialised and when it resumes.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581587 [details] [diff] [review]
Part 1: optimise imports in BrowserApp

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

::: mobile/android/base/BrowserApp.java
@@ +903,4 @@
>      public void onAttachedToWindow() {
>          // We can't show the first run experience until Gecko has finished initialization (bug 1077583).
>          checkFirstrun(this, new SafeIntent(getIntent()));
> +

nit: ws

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581597 [details] [diff] [review]
Part 2: Store the number of queued tabs in a  shared pref

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

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +82,5 @@
>          notificationManager.notify(TabQueueHelper.TAB_QUEUE_NOTIFICATION_ID, builder.build());
> +
> +        // Store the number of URLs queued so that we don't have to read and process the file to see if we have
> +        // any urls to open
> +        final StrictMode.ThreadPolicy savedPolicy = StrictMode.allowThreadDiskReads();

Why override the policy? Why not post this to a background thread?

But this seems to be called from the executor service - does it really fire off StrictMode warnings?

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581600 [details] [diff] [review]
Part 3: Read the number of queued tabs from  shared pref

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

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +94,4 @@
>              StrictMode.setThreadPolicy(savedPolicy);
>          }
>      }
> +    

nit: ws

@@ +94,5 @@
>              StrictMode.setThreadPolicy(savedPolicy);
>          }
>      }
> +    
> +    static public int getTabQueueLength(Context context) {

nit: syntactically correct but I've only ever seen `public static` in our code base

nit: final

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581600 [details] [diff] [review]
Part 3: Read the number of queued tabs from  shared pref

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

Similar question on thread policy - perhaps it would be better to assert that we're not on the UI thread, if that's what this is meant to do?

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581605 [details] [diff] [review]
Part 5: add delete file method to gecko profile

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

lgtm w/ nits

::: mobile/android/base/GeckoProfile.java
@@ +679,4 @@
>          }
>      }
>  
> +    public void deleteFile(String fileName) {

nit: -> `deleteProfileFile` or `deleteFileFromProfileDir` or similar

I missed the getDir() and thought this would delete any file.

Note: this will throw if filename is null - is it guaranteed not to be null? If not, I'd add an IllegalArgumentException, or log & return.

@@ +680,5 @@
>      }
>  
> +    public void deleteFile(String fileName) {
> +        File file = new File(getDir(), fileName);
> +        if (file.exists()) {

We may not need this check - I think delete may return false if the file DNE. Did you try this code without it?

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Open tab queue file when fennec starts or resumes

MEGA PATCH - this addresses nits, sorry I couldn't be bothered to put it all in to micro patches :)

감사합니다

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8583173 [details] [diff] [review]
Open tab queue file when fennec starts or  resumes

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

r+ w/ nits.

::: mobile/android/base/BrowserApp.java
@@ +904,5 @@
>      public void onAttachedToWindow() {
>          // We can't show the first run experience until Gecko has finished initialization (bug 1077583).
>          checkFirstrun(this, new SafeIntent(getIntent()));
> +
> +        processTabQueue();

Why is this necessary here? Shouldn't doing this in onResume suffice?

@@ +912,5 @@
> +        if (AppConstants.NIGHTLY_BUILD && AppConstants.MOZ_ANDROID_TAB_QUEUE) {
> +            ThreadUtils.postToBackgroundThread(new Runnable() {
> +                @Override
> +                public void run() {
> +                    if (TabQueueHelper.shouldOpenTabQueueUrls(getApplicationContext())) {

Make a comment why we're using the application context here (rather than passing in `this`.

Also, store the context as a function-local var.

@@ +913,5 @@
> +            ThreadUtils.postToBackgroundThread(new Runnable() {
> +                @Override
> +                public void run() {
> +                    if (TabQueueHelper.shouldOpenTabQueueUrls(getApplicationContext())) {
> +                        TabQueueHelper.openQueuedUrls(getApplicationContext(), mProfile, TabQueueHelper.FILE_NAME);

nit: I might prefer if openQueuedUrls was something like `openQueuedUrlsIfAvailable`, and just to open the urls if there are > 0 and it's enabled.

i.e. combine shouldOpenTabQueueUrls & openQueuedUrls

@@ +939,5 @@
>  
>          EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener)this,
>              "Prompt:ShowTop");
> +
> +        processTabQueue();

I'm guessing this is onResume? If so, good. :)

::: mobile/android/base/GeckoProfile.java
@@ +679,5 @@
>          }
>      }
>  
> +    public boolean deleteFileFromProfileDir(String fileName) throws IllegalArgumentException {
> +        if(TextUtils.isEmpty(fileName)) {

nit: if (

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +90,5 @@
> +        // any urls to open
> +        final SharedPreferences prefs = GeckoSharedPrefs.forApp(context);
> +
> +        int openInBackgroundCount = prefs.getInt(TabQueueHelper.PREF_TAB_QUEUE_COUNT, 0);
> +        prefs.edit().putInt(TabQueueHelper.PREF_TAB_QUEUE_COUNT, openInBackgroundCount + 1).apply();

My latest fx-team pull, my best guess of this method would be showNotification() - why is the count of tabs in background increased here? And shouldn't we be using the tabsQueued argument?

I see this is called by TabQueueService.addURLToTabQueue - I think this functionality is better put there.

@@ +96,5 @@
> +
> +    public static boolean shouldOpenTabQueueUrls(final Context context) {
> +        ThreadUtils.assertNotOnUiThread();
> +
> +        final SharedPreferences prefs = GeckoSharedPrefs.forApp(context);

Followup bug: why are these preferences scoped to the app? Why not per profile? Add a comment to the main tab queue class entry point.

@@ +104,5 @@
> +
> +        return tabQueueEnabled && tabsQueued > 0;
> +    }
> +
> +    public static int getTabQueueLength(final Context context) {

nit: private?

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Martyn Haigh (:mhaigh) from comment #14)
> MEGA PATCH - this addresses nits, sorry I couldn't be bothered to put it all
> in to micro patches :)

괜찮아요~!

Comment 17

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f098476c08bb

Comment 18

[Martyn Haigh (:mhaigh)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eda1b8828dfb