Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1425653003:
Show offline filter view if offline page exists and offline (Closed)
DescriptionShow offline filter view if offline page exists and offline
This is not sticky. That is, when online, the saved view is still shown.
BUG=491352
Committed: https://crrev.com/2e4062e61846f45a95a07167060d407e791dcc51
Cr-Commit-Position: refs/heads/master@{#356726}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address feedback #
Total comments: 3
Messages
Total messages: 17 (3 generated)
jianli@chromium.org changed reviewers: + fgorski@chromium.org, kkimlabs@chromium.org
Description was changed from ========== Show offline filter view if offline page exists and offline BUG=491352 ========== to ========== Show offline filter view if offline page exists and offline This is not sticky. That is, when online, the saved view is still shown. BUG=491352 ==========
https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:381: state = UIState.STATE_FILTER; 1. since the responsibility of function is notifying state changes, I think it's better to put this code block in the beginning of |setState| function. Also, we already have fallback code for handling invalid state there. 2. Q: so if user clicks "All items" on the navigation panel, this will take it to filter view, right? I think that could be somewhat confusing... what do you think?
https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:381: state = UIState.STATE_FILTER; On 2015/10/27 00:43:12, Kibeom Kim wrote: > 1. since the responsibility of function is notifying state changes, I think it's > better to put this code block in the beginning of |setState| function. Also, we > already have fallback code for handling invalid state there. Since going to offline filter view is just temporary thing when offline, we don't want to do it in setState to save the value. > > 2. Q: so if user clicks "All items" on the navigation panel, this will take it > to filter view, right? I think that could be somewhat confusing... what do you > think? Yes, it will take to the offline filter view when there is no network connection and offline page exists. We have the discussion on this behavior internally. But since this only applies in offline mode and we will not keep this sticky, we think it will cause least confusion to the users.
https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:381: state = UIState.STATE_FILTER; On 2015/10/27 00:56:48, jianli wrote: > On 2015/10/27 00:43:12, Kibeom Kim wrote: > > 1. since the responsibility of function is notifying state changes, I think > it's > > better to put this code block in the beginning of |setState| function. Also, > we > > already have fallback code for handling invalid state there. > > Since going to offline filter view is just temporary thing when offline, we > don't want to do it in setState to save the value. > > > > > 2. Q: so if user clicks "All items" on the navigation panel, this will take it > > to filter view, right? I think that could be somewhat confusing... what do you > > think? > > Yes, it will take to the offline filter view when there is no network connection > and offline page exists. We have the discussion on this behavior internally. But > since this only applies in offline mode and we will not keep this sticky, we > think it will cause least confusion to the users. > I see. OK. https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:393: assert mUIObservers.isEmpty(); I think STATE_LOADING should be an exception to STATE_FILTER override.
https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:381: state = UIState.STATE_FILTER; On 2015/10/27 17:31:49, Kibeom Kim wrote: > On 2015/10/27 00:56:48, jianli wrote: > > On 2015/10/27 00:43:12, Kibeom Kim wrote: > > > 1. since the responsibility of function is notifying state changes, I think > > it's > > > better to put this code block in the beginning of |setState| function. Also, > > we > > > already have fallback code for handling invalid state there. > > > > Since going to offline filter view is just temporary thing when offline, we > > don't want to do it in setState to save the value. > > > > > > > > 2. Q: so if user clicks "All items" on the navigation panel, this will take > it > > > to filter view, right? I think that could be somewhat confusing... what do > you > > > think? > > > > Yes, it will take to the offline filter view when there is no network > connection > > and offline page exists. We have the discussion on this behavior internally. > But > > since this only applies in offline mode and we will not keep this sticky, we > > think it will cause least confusion to the users. > > > > I see. OK. Jian, I am not sure if I understand your response. If I understand correctly, the scenario that Kibeom is talking about here is the following: 1. User goes offline 2. Clicks on Saved pages to open EB (opens Saved page filter) 3. Clicks on All saved pages to change the list to a different view They should be seeing all saved pages and not be limited to the filter in that case. Is that how your code will behave here?
https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:381: state = UIState.STATE_FILTER; On 2015/10/27 20:20:22, fgorski wrote: > On 2015/10/27 17:31:49, Kibeom Kim wrote: > > On 2015/10/27 00:56:48, jianli wrote: > > > On 2015/10/27 00:43:12, Kibeom Kim wrote: > > > > 1. since the responsibility of function is notifying state changes, I > think > > > it's > > > > better to put this code block in the beginning of |setState| function. > Also, > > > we > > > > already have fallback code for handling invalid state there. > > > > > > Since going to offline filter view is just temporary thing when offline, we > > > don't want to do it in setState to save the value. > > > > > > > > > > > 2. Q: so if user clicks "All items" on the navigation panel, this will > take > > it > > > > to filter view, right? I think that could be somewhat confusing... what do > > you > > > > think? > > > > > > Yes, it will take to the offline filter view when there is no network > > connection > > > and offline page exists. We have the discussion on this behavior internally. > > But > > > since this only applies in offline mode and we will not keep this sticky, we > > > think it will cause least confusion to the users. > > > > > > > I see. OK. > > Jian, I am not sure if I understand your response. If I understand correctly, > the scenario that Kibeom is talking about here is the following: > 1. User goes offline > 2. Clicks on Saved pages to open EB (opens Saved page filter) > 3. Clicks on All saved pages to change the list to a different view > > They should be seeing all saved pages and not be limited to the filter in that > case. > Is that how your code will behave here? Good catch. Moved the logic to setState. https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:393: assert mUIObservers.isEmpty(); On 2015/10/27 17:31:49, Kibeom Kim wrote: > I think STATE_LOADING should be an exception to STATE_FILTER override. Moved the logic to setState and STATE_LOADING is handled.
please review again. thanks On Tuesday, October 27, 2015, <jianli@chromium.org> wrote: > > > https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... > File > > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java > (right): > > > https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:381: > state = UIState.STATE_FILTER; > On 2015/10/27 20:20:22, fgorski wrote: > >> On 2015/10/27 17:31:49, Kibeom Kim wrote: >> > On 2015/10/27 00:56:48, jianli wrote: >> > > On 2015/10/27 00:43:12, Kibeom Kim wrote: >> > > > 1. since the responsibility of function is notifying state >> > changes, I > >> think >> > > it's >> > > > better to put this code block in the beginning of |setState| >> > function. > >> Also, >> > > we >> > > > already have fallback code for handling invalid state there. >> > > >> > > Since going to offline filter view is just temporary thing when >> > offline, we > >> > > don't want to do it in setState to save the value. >> > > >> > > > >> > > > 2. Q: so if user clicks "All items" on the navigation panel, >> > this will > >> take >> > it >> > > > to filter view, right? I think that could be somewhat >> > confusing... what do > >> > you >> > > > think? >> > > >> > > Yes, it will take to the offline filter view when there is no >> > network > >> > connection >> > > and offline page exists. We have the discussion on this behavior >> > internally. > >> > But >> > > since this only applies in offline mode and we will not keep this >> > sticky, we > >> > > think it will cause least confusion to the users. >> > > >> > >> > I see. OK. >> > > Jian, I am not sure if I understand your response. If I understand >> > correctly, > >> the scenario that Kibeom is talking about here is the following: >> 1. User goes offline >> 2. Clicks on Saved pages to open EB (opens Saved page filter) >> 3. Clicks on All saved pages to change the list to a different view >> > > They should be seeing all saved pages and not be limited to the filter >> > in that > >> case. >> Is that how your code will behave here? >> > > Good catch. Moved the logic to setState. > > > https://codereview.chromium.org/1425653003/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:393: > assert mUIObservers.isEmpty(); > On 2015/10/27 17:31:49, Kibeom Kim wrote: > >> I think STATE_LOADING should be an exception to STATE_FILTER override. >> > > Moved the logic to setState and STATE_LOADING is handled. > > https://codereview.chromium.org/1425653003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:298: if (mStateStack.isEmpty()) { I think there is one corner case you want to cover: if the current state is STATE_LOADING, this is the first time to set an actual state, but since we only check isEmpty() here, it won't set offline page in that case.
On 2015/10/28 22:16:10, Kibeom Kim wrote: > https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java > (right): > > https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:298: > if (mStateStack.isEmpty()) { > I think there is one corner case you want to cover: if the current state is > STATE_LOADING, this is the first time to set an actual state, but since we only > check isEmpty() here, it won't set offline page in that case. To test manually, I think we can insert thread sleep code somewhere in bookmark model initialization path.
https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:298: if (mStateStack.isEmpty()) { On 2015/10/28 22:16:10, Kibeom Kim wrote: > I think there is one corner case you want to cover: if the current state is > STATE_LOADING, this is the first time to set an actual state, but since we only > check isEmpty() here, it won't set offline page in that case. Not sure I understand how to hit this corner case. If current state is STATE_LOADING, it means that setState should have already been called to set a loading state with the actual URL and we should already check if we want to switch to offline filter view.
lgtm https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java (right): https://codereview.chromium.org/1425653003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkManager.java:298: if (mStateStack.isEmpty()) { On 2015/10/28 23:22:50, jianli wrote: > On 2015/10/28 22:16:10, Kibeom Kim wrote: > > I think there is one corner case you want to cover: if the current state is > > STATE_LOADING, this is the first time to set an actual state, but since we > only > > check isEmpty() here, it won't set offline page in that case. > > Not sure I understand how to hit this corner case. If current state is > STATE_LOADING, it means that setState should have already been called to set a > loading state with the actual URL and we should already check if we want to > switch to offline filter view. Sorry! yeah you're right, I just re-read the code and seems my memory was incorrect :/.
The CQ bit was checked by jianli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1425653003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1425653003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2e4062e61846f45a95a07167060d407e791dcc51 Cr-Commit-Position: refs/heads/master@{#356726} |
