Closed Bug 1292191 Opened 3 years ago Closed 3 years ago

Unnecessary check for mShouldRestore in restoreSessionTabs

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: mkaply, Unassigned, Mentored)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(2 files, 1 obsolete file)

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.
Mentor: s.kaspari, mozilla
Priority: -- → P5
Whiteboard: [lang=java][good first bug]
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.
> 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.
Attached file Remove redundant check (obsolete) —
Hey, i'm new to this, still learning how to do things here. Here's restoreSessionTabs without redundant check.
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 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();
>    }
D:
I'm sorry for spamming, how can i delete comment here?
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.
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.
Attached file gecko.diff.txt
Something like this?
Attachment #8780510 - Attachment is obsolete: true
Comment on attachment 8780608 [details]
gecko.diff.txt

You can set the review flag to request review from someone. Otherwise this might get lost. :)
Attachment #8780608 - Flags: review?(mozilla)
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.
Attachment #8780608 - Flags: review?(mozilla) → review+
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.
(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
This will need rebasing now that bug 1190627 is in the process of landing.
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.
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.
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
I closed too fast. There is still a superfluous mRestoreTabs. Sorry.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/888c7dec3cae
Remove unneccesary check for mShouldRestore. r=mkaply
https://hg.mozilla.org/mozilla-central/rev/888c7dec3cae
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.