Closed Bug 1145579 Opened 7 years ago Closed 7 years ago

Open tab queue file when fennec starts or resumes

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(1 file, 7 obsolete files)

When Fennec loads or resumes, read the tab queue file and load tabs for each entry.
Blocks: tab-queue
Depends on: 1134235
Blocks: 1145581
Original scope was a bit too broad for this bug : this bug will track reading the tab queue file when fennec starts or resumes
Summary: Load queued urls from tab queue file → Open tab queue file when fennec starts or resumes
Blocks: 1146325
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)
Attachment #8581587 - Flags: review?(michael.l.comella)
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.
Attachment #8581597 - Flags: review?(michael.l.comella)
Method to read and return the number of tabs queued from the shared pref added in the last patch
Attachment #8581600 - Flags: review?(michael.l.comella)
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.
Attachment #8581604 - Flags: review?(michael.l.comella)
Adding a method to delete a file via the gecko profile
Attachment #8581605 - Flags: review?(michael.l.comella)
Here we add the method which will read and process the tab queue.  We also perform some cleanup after the data has been processed.
Attachment #8581627 - Flags: review?(michael.l.comella)
Here we add hooks in to BrowserApp to process the tab queue when gecko has initialised and when it resumes.
Attachment #8581628 - Flags: review?(michael.l.comella)
Blocks: 1146589
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
Attachment #8581587 - Flags: review?(michael.l.comella) → review+
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 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 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 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?
Attachment #8581605 - Flags: review?(michael.l.comella) → review+
MEGA PATCH - this addresses nits, sorry I couldn't be bothered to put it all in to micro patches :)

감사합니다
Attachment #8581587 - Attachment is obsolete: true
Attachment #8581597 - Attachment is obsolete: true
Attachment #8581600 - Attachment is obsolete: true
Attachment #8581604 - Attachment is obsolete: true
Attachment #8581605 - Attachment is obsolete: true
Attachment #8581627 - Attachment is obsolete: true
Attachment #8581628 - Attachment is obsolete: true
Attachment #8581597 - Flags: review?(michael.l.comella)
Attachment #8581600 - Flags: review?(michael.l.comella)
Attachment #8581604 - Flags: review?(michael.l.comella)
Attachment #8581627 - Flags: review?(michael.l.comella)
Attachment #8581628 - Flags: review?(michael.l.comella)
Attachment #8583173 - Flags: review?(michael.l.comella)
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?
Attachment #8583173 - Flags: review?(michael.l.comella) → review+
(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 :)

괜찮아요~!
https://hg.mozilla.org/mozilla-central/rev/eda1b8828dfb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.