extensions: Fix sessions API getRecentlyClosed() bug on desktop Android [chromium/src : main]
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
Zoraiz, please take a look. Just a bug I noticed in passing. Thanks!
Related details
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
// to check again as Java can add at most one window.The second part of the comment is not entirely clear to me. No need to check what?
Related details
- James Cook
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 2 comments![]( "Open in Gerrit")
The second part of the comment is not entirely clear to me. No need to check what?
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Code-Review | +1 |
1 comment
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Achuith Bhandarkar added 4 comments![]( "Open in Gerrit")
I could be wrong below, could you please check?
max_results >= 0 &&I believe we can drop this check since size_t is always positive
if (result_.size() == max_results) {Does this early return create a problem? If `result_` is filled to `max_results` with tabs from the `TabRestoreService`, this will return before querying for a potentially more recently closed window from the Java side.
Since the API should return the most recently closed sessions, this could lead to incorrect results. For example, if `maxResults` is 1, a tab was recently closed, and then a window was closed (making it the most recent item), this implementation would return the tab without ever checking for the window.
A potential solution would be to perform the trimming *after* the window from the Java side has been added. You could store `max_results` as a member variable, and then `OnRecentlyClosedWindowOrTabCallback` could trim `result_` to the correct size before calling `Respond()`.
// Querying with a maxResults limit of 1 should return 1 tab and not theThis test case seems to be asserting incorrect behavior. In this test, a window is closed after a tab, making the window the most recently closed session. Therefore, a call to `getRecentlyClosed` with `maxResults: 1` should return the window, not the tab.
This test appears to validate the new logic in `sessions_api.cc` which may fail to fetch the window if the results list is already full of tabs. Since the API is documented to return sessions sorted by recency, this test should probably be changed to assert the correct behavior (i.e., that the window is returned).
Related details
- James Cook
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 6 comments![]( "Open in Gerrit")
Achuith, please take another look.
Calder, could you take a look at *android* and sessions_api.cc (for JNI). I have some failing tests (only on the bots!) and need some directional advice on how I'm passing a pair items from Java to C++. Thanks!
Zoraiz to CC, just FYI
public static class TabModelAndTimestamp {Calder, I now need the getRecentlyClosedWindow() JNI method to take a JniCallback with both a TabModel and a timestamp, but JniCallback only takes one parameter. So I created this class, which I unpack by hand on the C++ side.
Is there a better way to do this? If so please let me know (and I'll need some hand-holding, I'm not very good with Java or JNI).
CHECK(tab_model_field_id);Calder, this CHECK fails, but only on the bots. Locally everything works fine. Am I doing something wrong? Are class paths different somehow?
I believe we can drop this check since size_t is always positive
Done
Does this early return create a problem? If `result_` is filled to `max_results` with tabs from the `TabRestoreService`, this will return before querying for a potentially more recently closed window from the Java side.
Since the API should return the most recently closed sessions, this could lead to incorrect results. For example, if `maxResults` is 1, a tab was recently closed, and then a window was closed (making it the most recent item), this implementation would return the tab without ever checking for the window.
A potential solution would be to perform the trimming *after* the window from the Java side has been added. You could store `max_results` as a member variable, and then `OnRecentlyClosedWindowOrTabCallback` could trim `result_` to the correct size before calling `Respond()`.
Done. I now add the window (if there is one). If I added one, I sort the results by time and truncate the list if necessary.
// Querying with a maxResults limit of 1 should return 1 tab and not theThis test case seems to be asserting incorrect behavior. In this test, a window is closed after a tab, making the window the most recently closed session. Therefore, a call to `getRecentlyClosed` with `maxResults: 1` should return the window, not the tab.
This test appears to validate the new logic in `sessions_api.cc` which may fail to fetch the window if the results list is already full of tabs. Since the API is documented to return sessions sorted by recency, this test should probably be changed to assert the correct behavior (i.e., that the window is returned).
- Achuith Bhandarkar
- Calder Kitagawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Achuith Bhandarkar added 1 comment![]( "Open in Gerrit")
base::android::JavaRef<jobject>::CreateLeaky(env, tab_model);My Java is also pretty weak. Is it possible to use
`base::android::ScopedJavaLocalRef<jobject> j_tab_model(env, tab_model);`
instead of the leaky reference to delete j_tab_model? Also in `SessionsRestoreFunction::OnGetRecentlyClosedWindow`.
Related details
- Calder Kitagawa
- James Cook
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Achuith Bhandarkar (Gerrit)
Achuith Bhandarkar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Achuith Bhandarkar
| Code-Review | +1 |
1 comment
lgtm modulo Calder's feedback
Related details
- Calder Kitagawa
- James Cook
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |