1292191 - Unnecessary check for mShouldRestore in restoreSessionTabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Mike Kaply [:mkaply]]

Looking at the function restoreSessionTabs, it checks mShouldRestore to decide if it should do something:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1791

But the only caller of restoreSessionTabs:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1378

Only calls it if mShouldRestore is true.

So there's no need for this check inside of restoreSessionTabs.

Comment 1

[Julian Chu [:walkingice]]

After looked into source code and related commits, I think the duplicated chekcing could be removed as you mentioned.

Since I am newbie to here, I would like to take this issue as my first step to learn how to contribute to Mozilla Android app.

Comment 2

[Mike Kaply [:mkaply]]

> Since I am newbie to here, I would like to take this issue as my first step to learn how to contribute to Mozilla Android app.

That's great. Please let me know if you need anything.

Comment 3

[Zero One]

Attachment: Remove redundant check

Hey, i'm new to this, still learning how to do things here. Here's restoreSessionTabs without redundant check.

Comment 4

[Zero One]

Comment on attachment 8780510 [details]
Remove redundant check

@WorkerThread
    private String restoreSessionTabs(final boolean isExternalURL) throws SessionRestoreException {
        try {
            String sessionString = getProfile().readSessionFile(false);
            if (sessionString == null) {
                throw new SessionRestoreException("Could not read from session file");
            }

            // If we are doing an OOM restore, parse the session data and
            // stub the restored tabs immediately. This allows the UI to be
            // updated before Gecko has restored.
            final JSONArray tabs = new JSONArray();
            final JSONObject windowObject = new JSONObject();

            LastSessionParser parser = new LastSessionParser(tabs, windowObject, isExternalURL);

            if (mPrivateBrowsingSession == null) {
                parser.parse(sessionString);
            } else {
                parser.parse(sessionString, mPrivateBrowsingSession);
            }

            if (tabs.length() > 0) {
                windowObject.put("tabs", tabs);
                sessionString = new JSONObject().put("windows", new JSONArray().put(windowObject)).toString();
            } else {
                if (parser.allTabsSkipped()) {
                        // If we intentionally skipped all tabs we've read from the session file, we
                        // set mShouldRestore back to false at this point already, so the calling code
                        // can infer that the exception wasn't due to a damaged session store file.
                    mShouldRestore = false;
                }
                throw new SessionRestoreException("No tabs could be read from session file");
            }
            
            JSONObject restoreData = new JSONObject();
            restoreData.put("sessionString", sessionString);
            return restoreData.toString();
        } catch (JSONException e) {
            throw new SessionRestoreException(e);
        }
    }

    @Override
    public GeckoProfile getProfile() {
        return GeckoThread.getActiveProfile();
    }

Comment 5

[Zero One]

Comment on attachment 8780510 [details]
Remove redundant check

>@WorkerThread
>    private String restoreSessionTabs(final boolean isExternalURL) throws SessionRestoreException {
>        try {
>            String sessionString = getProfile().readSessionFile(false);
>            if (sessionString == null) {
>                throw new SessionRestoreException("Could not read from session file");
>            }
>
>            // If we are doing an OOM restore, parse the session data and
>            // stub the restored tabs immediately. This allows the UI to be
>            // updated before Gecko has restored.
>            final JSONArray tabs = new JSONArray();
>            final JSONObject windowObject = new JSONObject();
>
>            LastSessionParser parser = new LastSessionParser(tabs, windowObject, isExternalURL);
>
>            if (mPrivateBrowsingSession == null) {
>                parser.parse(sessionString);
>            } else {
>                parser.parse(sessionString, mPrivateBrowsingSession);
>            }
>
>            if (tabs.length() > 0) {
>                windowObject.put("tabs", tabs);
>                sessionString = new JSONObject().put("windows", new JSONArray().put(windowObject)).toString();
>            } else {
>                if (parser.allTabsSkipped()) {
>                        // If we intentionally skipped all tabs we've read from the session file, we
>                        // set mShouldRestore back to false at this point already, so the calling code
>                        // can infer that the exception wasn't due to a damaged session store file.
>                    mShouldRestore = false;
>                }
>                throw new SessionRestoreException("No tabs could be read from session file");
>            }
>            
>            JSONObject restoreData = new JSONObject();
>            restoreData.put("sessionString", sessionString);
>            return restoreData.toString();
>        } catch (JSONException e) {
>            throw new SessionRestoreException(e);
>        }
>    }
>
>    @Override
>    public GeckoProfile getProfile() {
>        return GeckoThread.getActiveProfile();
>    }

Comment 6

[Zero One]

D:
I'm sorry for spamming, how can i delete comment here?

Comment 7

[Mike Kaply [:mkaply]]

Don't worry about it, it happens.

Unfortunately you can't delete comments in Bugzilla.

So for the review, you can attach a diff file with context and that will allow us to review.

If you checked out the code view mercurial, you should be able to do hg diff.

Comment 8

[Julian Chu [:walkingice]]

If I understand it correctly, is to use mozreview to submit patch a recommend way? If so, may ZeroOne can also take a look of that.(Search "mozilla how to submit a patch"). There are lots of documentations and I am also still learning from that.

Comment 9

[Zero One]

Attachment: gecko.diff.txt

Something like this?

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8780608 [details]
gecko.diff.txt

You can set the review flag to request review from someone. Otherwise this might get lost. :)

Comment 11

[Mike Kaply [:mkaply]]

Comment on attachment 8780608 [details]
gecko.diff.txt

This looks great.

FYI, when you're only changing whitespace, you can do a diff that doesn't include whitespace and it makes it more obvious how small the change is.

This is great though.

So now that your patch has been approved, I'll check it in.

Comment 12

[Mike Kaply [:mkaply]]

Zero One: I do need a name and email in order to checkin, and also so it can be credited to you when we announce our new contributors for each release.

Comment 13

[Zero One]

(In reply to Mike Kaply [:mkaply] from comment #12)
> Zero One: I do need a name and email in order to checkin, and also so it can
> be credited to you when we announce our new contributors for each release.

May I remain anonymous for now? It's not a big deal, I just want to learn for now. Maybe after I fix some serious bug, or gain more experience, I'll reveal myself. :D

Comment 14

[Jan Henning [:JanH]]

This will need rebasing now that bug 1190627 is in the process of landing.

Comment 15

[ameya]

Attachment: Patch for Bug 1292191 - Unnecessary check for mShouldRestore in restoreSessionTabs

Hi team,

 Please find attached the patch for the bug 1292191 -Unnecessary check for mShouldRestore in restoreSessionTabs. This is my first patch submission so please provide me feedback for the same. This was not much of a code change but I have tested my code change with the following test cases :

1. Tabs open correctly on reopening the application
2. If logged in to any website, on reopening the application session is not lost

Thanks.

Comment 16

[Mike Kaply [:mkaply]]

ameya:

I appreciate your efforts, but there was already a patch here done by Zero One (that hasn't been checked in yet).

I hope you'll find another bug to contribute to.

Comment 17

[Mike Kaply [:mkaply]]

So based on the new changed for bug 1190627, this code is no longer necessary.

the mShouldRestore is actually being used now.

See:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java?q=file%3Ageckoapp.java&redirect_type=single#1302

Thank you for your patch and I hope you'll continue to contribute.

Comment 18

[Mike Kaply [:mkaply]]

I closed too fast. There is still a superfluous mRestoreTabs. Sorry.

Comment 19

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/888c7dec3cae94f06b6690684408e11b3ec3a57a
Bug 1292191 - Remove unneccesary check for mShouldRestore. r=mkaply

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/888c7dec3cae
Remove unneccesary check for mShouldRestore. r=mkaply

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/888c7dec3cae