Closed
Bug 1292191
Opened 5 years ago
Closed 5 years ago
Unnecessary check for mShouldRestore in restoreSessionTabs
Categories
(Firefox for Android Graveyard :: General, defect, P5)
Firefox for Android Graveyard
General
Tracking
(firefox51 fixed)
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)
2.91 KB,
text/plain
|
mkaply
:
review+
|
Details |
9.29 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•5 years ago
|
Mentor: s.kaspari, mozilla
Priority: -- → P5
Whiteboard: [lang=java][good first bug]
Comment 1•5 years ago
|
||
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.
Reporter | ||
Comment 2•5 years ago
|
||
> 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.
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(); > }
Reporter | ||
Comment 7•5 years ago
|
||
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•5 years ago
|
||
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.
Something like this?
Attachment #8780510 -
Attachment is obsolete: true
Comment 10•5 years ago
|
||
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)
Reporter | ||
Comment 11•5 years ago
|
||
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+
Reporter | ||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
(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•5 years ago
|
||
This will need rebasing now that bug 1190627 is in the process of landing.
Comment 15•5 years ago
|
||
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.
Reporter | ||
Comment 16•5 years ago
|
||
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.
Reporter | ||
Comment 17•5 years ago
|
||
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: 5 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 18•5 years ago
|
||
I closed too fast. There is still a superfluous mRestoreTabs. Sorry.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Reporter | ||
Comment 19•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/888c7dec3cae94f06b6690684408e11b3ec3a57a Bug 1292191 - Remove unneccesary check for mShouldRestore. r=mkaply
Comment 20•5 years ago
|
||
Pushed by mozilla@kaply.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/888c7dec3cae Remove unneccesary check for mShouldRestore. r=mkaply
Comment 21•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/888c7dec3cae
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Assignee | ||
Updated•4 months ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•