Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1822853002:
[Offline pages] Refactoring: Merge Connectivity Listener into TabObserver (Closed)
Description[Offline pages] Refactoring: Merge Connectivity Listener into TabObserver
This change starts by covering functionality of:
* OfflinePagesTabObserver, and
* OfflinePagesConnectivityListener
with unit tests using junit. Next, the functionality of both classes is
merged into OPTabObserver. In order to make some of the code
more testable and mockable, it uses:
* a helper class to access tab methods,
* extracts some calls to external objects into one-liner functions
BUG=594252, 583436
R=petewil@chromium.org
Committed: https://crrev.com/b7d76ce35ca3e56261f98ea257ff564371905485
Cr-Commit-Position: refs/heads/master@{#383770}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding tests for tab observer #Patch Set 3 : Ensuring tests are added properly #Patch Set 4 : Updating OfflinePageUtils #
Total comments: 47
Patch Set 5 : Addressing CR feedback #Messages
Total messages: 22 (8 generated)
petewil@chromium.org changed reviewers: + petewil@chromium.org
https://codereview.chromium.org/1822853002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (left): https://codereview.chromium.org/1822853002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:58: observer.setConnected(connected); Was it the intent to remove this? Are you also changing things to no longer use existing observers? If so, that's fine, but I missed seeing the change to not use existing observers. I realize the change may not yet be complete, that's fine if so.
Hey, let me know what you think. If we don't want to check in code with so many test hooks, at least I have some base to solve the unified observer problem from. https://codereview.chromium.org/1822853002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (left): https://codereview.chromium.org/1822853002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:58: observer.setConnected(connected); On 2016/03/22 00:59:07, Pete Williamson wrote: > Was it the intent to remove this? Are you also changing things to no longer use > existing observers? If so, that's fine, but I missed seeing the change to not > use existing observers. I realize the change may not yet be complete, that's > fine if so. I replaced it with a check whether we are connected below. where we actually use it.
Patchset #3 (id:40001) has been deleted
Description was changed from ========== [WIP][Offline pages] Adding junit tests to OfflinePageConnectivityListener Adds junit tests for OfflinePageConnectivityListener. It also adds simple methods to OfflinePageConnectivityListener that allow to mock out pieces of functionality or query for internal state. BUG=594252 ========== to ========== [Offline pages] Refactoring: Merge Connectivity Listener into TabObserver This change starts by covering functionality of: * OfflinePagesTabObserver, and * OfflinePagesConnectivityListener with unit tests using junit. Next, the functionality of both classes is merged into OPTabObserver. In order to make some of the code more testable and mockable, it uses: * a helper class to access tab methods, * extracts some calls to external objects into one-liner functions BUG=594252,583436 R=petewil@chromium.org ==========
Pete, PTAL
fgorski@chromium.org changed reviewers: + dewittj@chromium.org, yusufo@chromium.org
Justin: please code review Yusuf: please take a look at dealing with final Tab (an adapter that can be mocked)
Approach looks good. Just to make sure, did you check that refactoring did not break the existing browser tests too? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:171: // TODO(fgorski): Or do we actually care, because of onPageLoadStarted? As log as we get a didFailPageLoad() event even when we are not observing a page, it should be safe to track only offline pages. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:186: // connection. When both conditions are met a snackbar is shown. If both conditions are already met, will this still show a snackbar? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... File chrome/android/java_sources.gni (left): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... chrome/android/java_sources.gni:502: "java/src/org/chromium/chrome/browser/offlinepages/OfflinePageConnectivityListener.java", This looks good, but why is it required? Do we need to manually update gni files the same way we used to update .gyp files every time we add or remove a file? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:132: mTabId = (int) actionData; How does the tab id make it into action data? I missed that somehow in my review. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:9: import static org.mockito.Mockito.any; Approach question: Why use Mockito instead of creating custom test stubs? I usually find the code with custom stubs easier to write, maintain, and reason about. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:57: // This call has to be mocked out until we update OfflinePageUtils. Perhaps we should have a TODO here to come back and update these when we do update OfflinePageUtils. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:68: doReturn(true).when(mTabHelper).isTabShowing(any(Tab.class)); Does it make sense to mock these in the individual tests instead of here? We might want to have tests where the tab is not showing at some point, or we start with an online page instead of an offline page. It's OK if your answer is "let's keep what we have and refactor later". It's also OK (but a bit confusing) if other mocks in the test itself override this as a default. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:142: // Hide the tab. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:146: // Testing on shown when connected. -> // If we are connected and we show the tab, we should see the reload snackbar show once. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:160: doReturn(false).when(mTabHelper).isTabShowing(any(Tab.class)); // Hide the tab. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:170: public void testOnHidden_whenSnackbarShown() { maybe rename: testOnHidden_snackbarNotShownForHiddenPage ?
https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:27: public class OfflinePageTabObserver This name is not so accurate anymore, now that it observes multiple types of things. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:36: private boolean mIsObservingNetworkChagnes; nit: fix spelling. Also, this bit is completely unnecessary (except that you inspect it in test). Can we actually remove this field from production code, and keep the boolean up-to-date in the tests using mocking code? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:45: static class TabHelper { any reason not to expose an interface, instead of a concrete class like this? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:127: mWasSnackbarShown = false; This could use a comment: why is this being reset? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:208: nit: add line // Methods from ConnectionTypeObserver https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:228: boolean isObservingTab(Tab tab) { Any way to test this by inspecting the tab itself? Better to test using the public api and observe external state changes. TabHelperForTest could keep track of which tabs were observed. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:168: createReloadSnackbarController(activity.getTabModelSelector()); should OfflinePageUtils#createReloadSnackbarController become part of OfflinePageTabObserver? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:81: assertFalse(observer.wasSnackbarShown()); can you use SnackbarManager#isShowing() or SnackbarManager#getCurrentSnackbarForTesting() instead of a private method?
Comments addressed. Pete, I actually updated the browser test in this patch to reflect the action data payload change in the reload snackbar going from button type to tab ID. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:27: public class OfflinePageTabObserver On 2016/03/28 20:17:59, dewittj wrote: > This name is not so accurate anymore, now that it observes multiple types of > things. Please suggest a better name. Primary task of this class is observing the tab and it serves a purpose beyond just reload snackbar, see onPageLoadStarted() We will have opportunity to rename it in future, once we start moving reload snackbar controller from offlinepageutils. Perhaps OPReloadSnackbarController is a better name for this class ;) https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:36: private boolean mIsObservingNetworkChagnes; On 2016/03/28 20:17:59, dewittj wrote: > nit: fix spelling. Done. > > Also, this bit is completely unnecessary (except that you inspect it in test). > Can we actually remove this field from production code, and keep the boolean > up-to-date in the tests using mocking code? 185, 201 https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:45: static class TabHelper { On 2016/03/28 20:17:59, dewittj wrote: > any reason not to expose an interface, instead of a concrete class like this? There is no benefit to do so. We don't intend to expose it beyond this class and the only override is provided by test mocks. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:127: mWasSnackbarShown = false; On 2016/03/28 20:17:59, dewittj wrote: > This could use a comment: why is this being reset? Done. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:171: // TODO(fgorski): Or do we actually care, because of onPageLoadStarted? On 2016/03/28 19:52:38, Pete Williamson wrote: > As log as we get a didFailPageLoad() event even when we are not observing a > page, it should be safe to track only offline pages. Done. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:208: On 2016/03/28 20:17:59, dewittj wrote: > nit: add line > // Methods from ConnectionTypeObserver Done. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:228: boolean isObservingTab(Tab tab) { On 2016/03/28 20:17:59, dewittj wrote: > Any way to test this by inspecting the tab itself? Better to test using the > public api and observe external state changes. TabHelperForTest could keep > track of which tabs were observed. I want to track observed tabs internally. Changed the code to reflect that. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:168: createReloadSnackbarController(activity.getTabModelSelector()); On 2016/03/28 20:17:59, dewittj wrote: > should OfflinePageUtils#createReloadSnackbarController become part of > OfflinePageTabObserver? Yes, that is the plan, but we have at least one more patch to get ther. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:186: // connection. When both conditions are met a snackbar is shown. On 2016/03/28 19:52:38, Pete Williamson wrote: > If both conditions are already met, will this still show a snackbar? Actually you are right. I added a piece of code that shows the snackbar, and then saw a failure in test that had to explicitly call onShown to get the snackbar showing. I think we are not getting onShown in Clank the first time we register, as the tab is visible as the url load is completed. Either way, now the code works better. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... File chrome/android/java_sources.gni (left): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... chrome/android/java_sources.gni:502: "java/src/org/chromium/chrome/browser/offlinepages/OfflinePageConnectivityListener.java", On 2016/03/28 19:52:38, Pete Williamson wrote: > This looks good, but why is it required? Do we need to manually update gni > files the same way we used to update .gyp files every time we add or remove a > file? Don't know what to tell you, except that we still have to update BUILD.gn files, which include these. What you don't need is the explicit step generating projects before running ninja. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java:132: mTabId = (int) actionData; On 2016/03/28 19:52:38, Pete Williamson wrote: > How does the tab id make it into action data? I missed that somehow in my > review. It is passed as action data when the reload snackbar is shown in OfflinePageUtils#showReloadSnackbar. setAction(..., tabId) https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:9: import static org.mockito.Mockito.any; On 2016/03/28 19:52:39, Pete Williamson wrote: > Approach question: Why use Mockito instead of creating custom test stubs? I > usually find the code with custom stubs easier to write, maintain, and reason > about. Reasons to use Mockito: Amongst it's weponry are such diverse elements as: * Boilerplate is gone with this approach * Mocks are easily (re-)configurable * You can verify what calls where made and which not much easier. * (My favorite) You can wrap a spy over the mocked class to actually invoke the actual implementation * The fluent interface applied in mockito makes things very readable * blah blah blah https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:57: // This call has to be mocked out until we update OfflinePageUtils. On 2016/03/28 19:52:39, Pete Williamson wrote: > Perhaps we should have a TODO here to come back and update these when we do > update OfflinePageUtils. Done. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:68: doReturn(true).when(mTabHelper).isTabShowing(any(Tab.class)); On 2016/03/28 19:52:39, Pete Williamson wrote: > Does it make sense to mock these in the individual tests instead of here? We > might want to have tests where the tab is not showing at some point, or we start > with an online page instead of an offline page. It's OK if your answer is > "let's keep what we have and refactor later". It's also OK (but a bit > confusing) if other mocks in the test itself override this as a default. Done. I added a comment in code. There are the common values used in most tests. Individual tests apply different values where it is required. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:81: assertFalse(observer.wasSnackbarShown()); On 2016/03/28 20:17:59, dewittj wrote: > can you use SnackbarManager#isShowing() or > SnackbarManager#getCurrentSnackbarForTesting() instead of a private method? I don't want to. This is part of the logic of this class. Snackbar might have been shown, but is no longer visible at the time wasSnackbarShown is invoked. This test makes sure that it is set to a reasonable value after creation. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:142: On 2016/03/28 19:52:38, Pete Williamson wrote: > // Hide the tab. Done. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:146: // Testing on shown when connected. On 2016/03/28 19:52:38, Pete Williamson wrote: > -> // If we are connected and we show the tab, we should see the reload snackbar > show once. Done. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:160: doReturn(false).when(mTabHelper).isTabShowing(any(Tab.class)); On 2016/03/28 19:52:39, Pete Williamson wrote: > // Hide the tab. Done. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:170: public void testOnHidden_whenSnackbarShown() { On 2016/03/28 19:52:39, Pete Williamson wrote: > maybe rename: testOnHidden_snackbarNotShownForHiddenPage ? Done.
lgtm https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:36: private boolean mIsObservingNetworkChagnes; On 2016/03/28 22:52:32, fgorski wrote: > On 2016/03/28 20:17:59, dewittj wrote: > > nit: fix spelling. > Done. > > > > Also, this bit is completely unnecessary (except that you inspect it in test). > > > Can we actually remove this field from production code, and keep the boolean > > up-to-date in the tests using mocking code? > > 185, 201 Technically, StopObservingNetworkChanges can be called without actually checking if we are observing network changes, since ObserverList#removeObserver bails early if there is no such observer. On second thought, that would mean we'd be depending on this particular behavior of ObserverList which might not stay the same forever, so I could see why you might want to keep track of the bit. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:81: assertFalse(observer.wasSnackbarShown()); On 2016/03/28 22:52:32, fgorski wrote: > On 2016/03/28 20:17:59, dewittj wrote: > > can you use SnackbarManager#isShowing() or > > SnackbarManager#getCurrentSnackbarForTesting() instead of a private method? > > I don't want to. This is part of the logic of this class. Snackbar might have > been shown, but is no longer visible at the time wasSnackbarShown is invoked. > This test makes sure that it is set to a reasonable value after creation. I'll leave it up to you then. *aside* I'm asking these types of questions simply because this type of test is brittle to changes. If you assert that a certain bit is set or not, then bugs in changing the bit could end up passing a test. If the test whether a snackbar was shown or not, then the test reflects the desired outcome, not internal state. Most of your tests verify both that showReloadSnackbar was or wasn't called, and also check Observer#wasSnackbarShown. I'd argue that asserting the value of Observer#wasSnackbarShown() is harmful to the resiliency of this test.
LGTM with one question https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:186: // connection. When both conditions are met a snackbar is shown. On 2016/03/28 22:52:32, fgorski wrote: > On 2016/03/28 19:52:38, Pete Williamson wrote: > > If both conditions are already met, will this still show a snackbar? > > Actually you are right. I added a piece of code that shows the snackbar, and > then saw a failure in test that had to explicitly call onShown to get the > snackbar showing. > > I think we are not getting onShown in Clank the first time we register, as the > tab is visible as the url load is completed. Either way, now the code works > better. Sorry, did you make a change in response to this comment? If so, where? https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... File chrome/android/java_sources.gni (left): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... chrome/android/java_sources.gni:502: "java/src/org/chromium/chrome/browser/offlinepages/OfflinePageConnectivityListener.java", On 2016/03/28 22:52:32, fgorski wrote: > On 2016/03/28 19:52:38, Pete Williamson wrote: > > This looks good, but why is it required? Do we need to manually update gni > > files the same way we used to update .gyp files every time we add or remove a > > file? > > Don't know what to tell you, except that we still have to update BUILD.gn files, > which include these. What you don't need is the explicit step generating > projects before running ninja. OK, I guess that might have happened while I was off working in another code tree, I missed the announcement that we need to add files to BUILD.gn https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:170: public void testOnHidden_whenSnackbarShown() { On 2016/03/28 19:52:39, Pete Williamson wrote: > maybe rename: testOnHidden_snackbarNotShownForHiddenPage ? Done.
fgorski@chromium.org changed reviewers: + newt@chromium.org
+newt@ please take a look at .gni and .gn https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:36: private boolean mIsObservingNetworkChagnes; On 2016/03/28 23:40:02, dewittj wrote: > On 2016/03/28 22:52:32, fgorski wrote: > > On 2016/03/28 20:17:59, dewittj wrote: > > > nit: fix spelling. > > Done. > > > > > > Also, this bit is completely unnecessary (except that you inspect it in > test). > > > > > Can we actually remove this field from production code, and keep the boolean > > > up-to-date in the tests using mocking code? > > > > 185, 201 > > Technically, StopObservingNetworkChanges can be called without actually checking > if we are observing network changes, since ObserverList#removeObserver bails > early if there is no such observer. On second thought, that would mean we'd be > depending on this particular behavior of ObserverList which might not stay the > same forever, so I could see why you might want to keep track of the bit. Acknowledged. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:186: // connection. When both conditions are met a snackbar is shown. On 2016/03/29 08:09:36, Pete Williamson wrote: > On 2016/03/28 22:52:32, fgorski wrote: > > On 2016/03/28 19:52:38, Pete Williamson wrote: > > > If both conditions are already met, will this still show a snackbar? > > > > Actually you are right. I added a piece of code that shows the snackbar, and > > then saw a failure in test that had to explicitly call onShown to get the > > snackbar showing. > > > > I think we are not getting onShown in Clank the first time we register, as the > > tab is visible as the url load is completed. Either way, now the code works > > better. > > Sorry, did you make a change in response to this comment? If so, where? Code was added at the end of OPTabObserver#startObservingTab https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... File chrome/android/java_sources.gni (left): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/java_sou... chrome/android/java_sources.gni:502: "java/src/org/chromium/chrome/browser/offlinepages/OfflinePageConnectivityListener.java", On 2016/03/29 08:09:36, Pete Williamson wrote: > On 2016/03/28 22:52:32, fgorski wrote: > > On 2016/03/28 19:52:38, Pete Williamson wrote: > > > This looks good, but why is it required? Do we need to manually update gni > > > files the same way we used to update .gyp files every time we add or remove > a > > > file? > > > > Don't know what to tell you, except that we still have to update BUILD.gn > files, > > which include these. What you don't need is the explicit step generating > > projects before running ninja. > > OK, I guess that might have happened while I was off working in another code > tree, I missed the announcement that we need to add files to BUILD.gn Acknowledged. https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java (right): https://codereview.chromium.org/1822853002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java:81: assertFalse(observer.wasSnackbarShown()); On 2016/03/28 23:40:02, dewittj wrote: > On 2016/03/28 22:52:32, fgorski wrote: > > On 2016/03/28 20:17:59, dewittj wrote: > > > can you use SnackbarManager#isShowing() or > > > SnackbarManager#getCurrentSnackbarForTesting() instead of a private method? > > > > I don't want to. This is part of the logic of this class. Snackbar might have > > been shown, but is no longer visible at the time wasSnackbarShown is invoked. > > This test makes sure that it is set to a reasonable value after creation. > > I'll leave it up to you then. > > *aside* > > I'm asking these types of questions simply because this type of test is brittle > to changes. If you assert that a certain bit is set or not, then bugs in > changing the bit could end up passing a test. If the test whether a snackbar > was shown or not, then the test reflects the desired outcome, not internal > state. > > Most of your tests verify both that showReloadSnackbar was or wasn't called, and > also check Observer#wasSnackbarShown. I'd argue that asserting the value of > Observer#wasSnackbarShown() is harmful to the resiliency of this test. Once I get OfflinePageUtils more testable I'll consider if exposing this still makes sense.
gn(i) changes lgtm
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1822853002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1822853002/100001
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Refactoring: Merge Connectivity Listener into TabObserver This change starts by covering functionality of: * OfflinePagesTabObserver, and * OfflinePagesConnectivityListener with unit tests using junit. Next, the functionality of both classes is merged into OPTabObserver. In order to make some of the code more testable and mockable, it uses: * a helper class to access tab methods, * extracts some calls to external objects into one-liner functions BUG=594252,583436 R=petewil@chromium.org ========== to ========== [Offline pages] Refactoring: Merge Connectivity Listener into TabObserver This change starts by covering functionality of: * OfflinePagesTabObserver, and * OfflinePagesConnectivityListener with unit tests using junit. Next, the functionality of both classes is merged into OPTabObserver. In order to make some of the code more testable and mockable, it uses: * a helper class to access tab methods, * extracts some calls to external objects into one-liner functions BUG=594252,583436 R=petewil@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Refactoring: Merge Connectivity Listener into TabObserver This change starts by covering functionality of: * OfflinePagesTabObserver, and * OfflinePagesConnectivityListener with unit tests using junit. Next, the functionality of both classes is merged into OPTabObserver. In order to make some of the code more testable and mockable, it uses: * a helper class to access tab methods, * extracts some calls to external objects into one-liner functions BUG=594252,583436 R=petewil@chromium.org ========== to ========== [Offline pages] Refactoring: Merge Connectivity Listener into TabObserver This change starts by covering functionality of: * OfflinePagesTabObserver, and * OfflinePagesConnectivityListener with unit tests using junit. Next, the functionality of both classes is merged into OPTabObserver. In order to make some of the code more testable and mockable, it uses: * a helper class to access tab methods, * extracts some calls to external objects into one-liner functions BUG=594252,583436 R=petewil@chromium.org Committed: https://crrev.com/b7d76ce35ca3e56261f98ea257ff564371905485 Cr-Commit-Position: refs/heads/master@{#383770} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b7d76ce35ca3e56261f98ea257ff564371905485 Cr-Commit-Position: refs/heads/master@{#383770} |
