Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2904053002:
[ios] Active web state observer in tab collection. (Closed)
Description[ios] Active web state observer
Tab collection now listens for updates to the active web state
and updates the tab cell. For example, the web state completes
navigation and the tab's title is updated.
This CL also fixes the cell's selected state, which was previously
using a buggy affine transformation on the cell's selected
background view.
This CL also dismisses the tab and displays the tab grid
when the last tab is closed in the tab strip.
BUG=686770
Review-Url: https://codereview.chromium.org/2904053002
Cr-Commit-Position: refs/heads/master@{#476770}
Committed: https://chromium.googlesource.com/chromium/src/+/daec770ed8e9a12165629a75e2a30d192ea6865c
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments. #
Total comments: 18
Patch Set 3 : Address comments. #
Total comments: 6
Patch Set 4 : Address optional comments. #Patch Set 5 : Update unittest #Messages
Total messages: 48 (36 generated)
The CQ bit was checked by edchin@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Description was changed from ========== [ios] Active web state observer BUG= ========== to ========== [ios] Active web state observer Tab collection now listens for updates to the active web state and updates the tab cell. For example, the web state completes navigation and the tab's title is updated. This CL also fixes the cell's selected state, which was previously using a buggy affine transformation on the cell's selected background view. BUG=686770 ==========
edchin@chromium.org changed reviewers: + rohitrao@chromium.org, sczs@chromium.org
edchin@chromium.org changed required reviewers: + sczs@chromium.org
The CQ bit was checked by edchin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by edchin@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ios] Active web state observer Tab collection now listens for updates to the active web state and updates the tab cell. For example, the web state completes navigation and the tab's title is updated. This CL also fixes the cell's selected state, which was previously using a buggy affine transformation on the cell's selected background view. BUG=686770 ========== to ========== [ios] Active web state observer Tab collection now listens for updates to the active web state and updates the tab cell. For example, the web state completes navigation and the tab's title is updated. This CL also fixes the cell's selected state, which was previously using a buggy affine transformation on the cell's selected background view. This CL also closes the tab strip when the last tab is closed. BUG=686770 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In the CL description, the changes here close the entire TabCoordinator's view when closing the last tab. The current phrasing "closing the tab strip", implies that the strip itself is shrunk but the toolbar+webcontents remain visible. This CL actually drops back into the tab grid. https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:119: - (void)webState:(web::WebState*)webState didLoadPageWithSuccess:(BOOL)success { Please add a comment explaining why we listen for page load notifications. Are we doing this to update the page title? Or to update the snapshot? https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm (right): https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:148: self.selectedIndex++; Is this VC independently trying to keep track of how the active tab index changes when tabs are added and removed? I don't think we should have this kind of logic here -- it'll be too easy to get out of sync if we change the corresponding logic in WebStateList. Can we push the active tab index into the consumer instead?
https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:20: @interface TabCollectionMediator ()<CRWWebStateObserver> Could we also move the WebstateListObserverving to the implementation file for consistency? https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm (right): https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:157: if (self.selectedIndex == index) { I also agree with Rohit, is it possible to push the selectedIndex from the mediator?
Description was changed from ========== [ios] Active web state observer Tab collection now listens for updates to the active web state and updates the tab cell. For example, the web state completes navigation and the tab's title is updated. This CL also fixes the cell's selected state, which was previously using a buggy affine transformation on the cell's selected background view. This CL also closes the tab strip when the last tab is closed. BUG=686770 ========== to ========== [ios] Active web state observer Tab collection now listens for updates to the active web state and updates the tab cell. For example, the web state completes navigation and the tab's title is updated. This CL also fixes the cell's selected state, which was previously using a buggy affine transformation on the cell's selected background view. This CL also dismisses the tab and displays the tab grid when the last tab is closed in the tab strip. BUG=686770 ==========
I also updated the issue description to address Rohit's comment. https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:20: @interface TabCollectionMediator ()<CRWWebStateObserver> On 2017/05/26 16:12:52, sczs wrote: > Could we also move the WebstateListObserverving to the implementation file for > consistency? WSLO is needed by the subclasses so it is exposed. https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:119: - (void)webState:(web::WebState*)webState didLoadPageWithSuccess:(BOOL)success { On 2017/05/26 12:44:01, rohitrao (ping after 24h) wrote: > Please add a comment explaining why we listen for page load notifications. Are > we doing this to update the page title? Or to update the snapshot? Done. https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm (right): https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:148: self.selectedIndex++; On 2017/05/26 12:44:01, rohitrao (ping after 24h) wrote: > Is this VC independently trying to keep track of how the active tab index > changes when tabs are added and removed? I don't think we should have this kind > of logic here -- it'll be too easy to get out of sync if we change the > corresponding logic in WebStateList. > > Can we push the active tab index into the consumer instead? The VC will get into an internally inconsistent state without this code. For example, a new cell is inserted by the end of this method. The highlighted cell was previously cell 5, but now it is cell 6. But the selected index was not updated, so this VC thinks it should be highlighting cell 5. You have inconsistent state at the end of this method. We could pass the active index along with this method. (perhaps that's what you were suggesting all along) https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:157: if (self.selectedIndex == index) { On 2017/05/26 16:12:52, sczs wrote: > I also agree with Rohit, is it possible to push the selectedIndex from the > mediator? See comment above.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
PTAL. The consumer protocol was updated so that the selectedIndex is always pushed along with inserts/deletes. This prevents inconsistent internal state inside the collection view. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:76: FindTabHelper* helper = FindTabHelper::FromWebState(webState); This crashes when the last web state is detached while the tab container is shown. So I added the nil check above. Does stopFinding need to be called at the time the web state is detached?
Thanks Ed! lgtm Rohit should be able to comment on the FIP issue. https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:20: @interface TabCollectionMediator ()<CRWWebStateObserver> On 2017/05/26 18:09:00, edchin wrote: > On 2017/05/26 16:12:52, sczs wrote: > > Could we also move the WebstateListObserverving to the implementation file for > > consistency? > > WSLO is needed by the subclasses so it is exposed. Acknowledged.
marq@chromium.org changed reviewers: + marq@chromium.org
https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_consumer.h (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_consumer.h:13: // Inserts |item| into tab collection at |index|. Here and below: document what |selectedIndex| means. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_consumer.h:27: - (void)replaceItemAtIndex:(int)index withItem:(TabCollectionItem*)item; Does this also need a |selectedIndex| parameter? https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:132: #pragma mark - Private Each private method needs a comment. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:168: _webStateObserver = Setting the WSO seems disconnected from populating the consumer. Is this correct place to do this? If so, it merits some comments. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm:26: - (void)setupSelectedBackgroundView { This is fine for this CL, but consider getting in the habit of keeping the changes in each CL logically grouped. This fix is small and would have landed quickly as an independent CL. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:147: [self selectItemAtIndex:selectedIndex]; I think I don't like the view controller calling consumer methods on itself. I'd rather have (say) a custom setter for selectedIndex that does the work currently in -selectItemAtIndex. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:187: [self.tabs reloadItemsAtIndexPaths:indexPaths]; Should this use -selectItemsAtIndexPath:animated:scrollPosition: and deselectItemAtIndexPath:animated: instead of reloading the items? https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:199: - (NSIndexPath*)indexPathForIndex:(int)index { Does this helper method pull its weight? https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_strip/tab_strip_coordinator.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_strip/tab_strip_coordinator.mm:68: if (self.webStateList.count() == 0) { if (self.webStateList.empty()) {
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
PTAL. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_consumer.h (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_consumer.h:13: // Inserts |item| into tab collection at |index|. On 2017/05/30 11:09:43, marq (ping after 24h) wrote: > Here and below: document what |selectedIndex| means. Done. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_consumer.h:27: - (void)replaceItemAtIndex:(int)index withItem:(TabCollectionItem*)item; On 2017/05/30 11:09:43, marq (ping after 24h) wrote: > Does this also need a |selectedIndex| parameter? The selection does not change with a replace. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:168: _webStateObserver = On 2017/05/30 11:09:43, marq (ping after 24h) wrote: > Setting the WSO seems disconnected from populating the consumer. Is this correct > place to do this? If so, it merits some comments. Moved to a more appropriate place. It should be set when the WSL is set, and when the active WS is changed. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.mm:26: - (void)setupSelectedBackgroundView { On 2017/05/30 11:09:43, marq (ping after 24h) wrote: > This is fine for this CL, but consider getting in the habit of keeping the > changes in each CL logically grouped. This fix is small and would have landed > quickly as an independent CL. Acknowledged. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:147: [self selectItemAtIndex:selectedIndex]; On 2017/05/30 11:09:43, marq (ping after 24h) wrote: > I think I don't like the view controller calling consumer methods on itself. I'd > rather have (say) a custom setter for selectedIndex that does the work currently > in -selectItemAtIndex. Good idea. Done. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:187: [self.tabs reloadItemsAtIndexPaths:indexPaths]; On 2017/05/30 11:09:44, marq (ping after 24h) wrote: > Should this use -selectItemsAtIndexPath:animated:scrollPosition: and > deselectItemAtIndexPath:animated: instead of reloading the items? Yes, selection should be done on the collection view rather than on cells. This is now updated to work properly. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:199: - (NSIndexPath*)indexPathForIndex:(int)index { On 2017/05/30 11:09:44, marq (ping after 24h) wrote: > Does this helper method pull its weight? Removed. https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_strip/tab_strip_coordinator.mm (right): https://codereview.chromium.org/2904053002/diff/120001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_strip/tab_strip_coordinator.mm:68: if (self.webStateList.count() == 0) { On 2017/05/30 11:09:44, marq (ping after 24h) wrote: > if (self.webStateList.empty()) { Done.
LGTM, all comments are optional. https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:159: [items Optional: The line wrap here is pretty hard to read. Maybe use some local variables to make it easier to scan? https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.h (right): https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.h:22: @property(nonatomic, assign) int selectedIndex; Does this need to be public? The consumer protocol provides a setter for it. https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm (right): https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:66: [self.tabs selectItemAtIndexPath:[NSIndexPath indexPathForItem:selectedIndex Do you also need to deselect the previous selection, or does the collection view not supporting multiple selections take care of this automatically?
Will make optional changes and land this CL. https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm (right): https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:159: [items On 2017/06/02 11:31:00, marq (ping after 24h) wrote: > Optional: The line wrap here is pretty hard to read. Maybe use some local > variables to make it easier to scan? Done. https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.h (right): https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.h:22: @property(nonatomic, assign) int selectedIndex; On 2017/06/02 11:31:00, marq (ping after 24h) wrote: > Does this need to be public? The consumer protocol provides a setter for it. Done. https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm (right): https://codereview.chromium.org/2904053002/diff/220001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm:66: [self.tabs selectItemAtIndexPath:[NSIndexPath indexPathForItem:selectedIndex On 2017/06/02 11:31:00, marq (ping after 24h) wrote: > Do you also need to deselect the previous selection, or does the collection view > not supporting multiple selections take care of this automatically? Deselect happens automatically.
The CQ bit was checked by edchin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by edchin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edchin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sczs@chromium.org, marq@chromium.org Link to the patchset: https://codereview.chromium.org/2904053002/#ps260001 (title: "Update unittest")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496434188249870,
"parent_rev": "109806d8e01bfe430b3305bd66c866b1029d19c0", "commit_rev":
"daec770ed8e9a12165629a75e2a30d192ea6865c"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Active web state observer Tab collection now listens for updates to the active web state and updates the tab cell. For example, the web state completes navigation and the tab's title is updated. This CL also fixes the cell's selected state, which was previously using a buggy affine transformation on the cell's selected background view. This CL also dismisses the tab and displays the tab grid when the last tab is closed in the tab strip. BUG=686770 ========== to ========== [ios] Active web state observer Tab collection now listens for updates to the active web state and updates the tab cell. For example, the web state completes navigation and the tab's title is updated. This CL also fixes the cell's selected state, which was previously using a buggy affine transformation on the cell's selected background view. This CL also dismisses the tab and displays the tab grid when the last tab is closed in the tab strip. BUG=686770 Review-Url: https://codereview.chromium.org/2904053002 Cr-Commit-Position: refs/heads/master@{#476770} Committed: https://chromium.googlesource.com/chromium/src/+/daec770ed8e9a12165629a75e2a3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:260001) as https://chromium.googlesource.com/chromium/src/+/daec770ed8e9a12165629a75e2a3... |
