Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2780423003:
[ios] Use web_state_list in web_contents_mediator. (Closed)
Description[ios] Use web_state_list in web_contents_mediator
This CL enables the user to select a tab in the tab switcher
and the new tab is reflected immediately in the currently
open tab container.
BUG=none
Review-Url: https://codereview.chromium.org/2780423003
Cr-Commit-Position: refs/heads/master@{#462776}
Committed: https://chromium.googlesource.com/chromium/src/+/ac5bc014fa46a1c13fa1e5702d01593279ecee64
Patch Set 1 #Patch Set 2 : Update unittests. #
Total comments: 16
Patch Set 3 : Improve unittests. #Patch Set 4 : Rebase. #
Messages
Total messages: 67 (54 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [ios] Use web_state_list in web_contents_mediator BUG= ========== to ========== [ios] Use web_state_list in web_contents_mediator This CL enables the user to select a tab in the tab switcher and the new tab is reflected immediately in the currently open tab container. BUG=none ==========
edchin@chromium.org changed reviewers: + lpromero@chromium.org, marq@chromium.org, 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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by edchin@chromium.org
The CQ bit was unchecked by edchin@chromium.org
The CQ bit was checked by edchin@chromium.org to run a CQ dry run
Patchset #1 (id:40001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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:80001) has been deleted
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.
PTAL. The unittests have been updated and improved.
edchin@chromium.org changed reviewers: + rohitrao@chromium.org
lgtm with comments. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:76: - (void)updateActiveWebState:(web::WebState*)webState { Add a comment. The name is a bit misleading, it's not updating the active web state, but updating the web contents consumer for the active web state. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:72: web_state_list_->ActivateWebStateAt(0); Your web_state_list_ doesn’t include web_state_. Is it an issue? https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:93: mediator.webStateList = web_state_list_.get(); The web state list is not used in the method you test. Is it a requirement to have a web state list set here? https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:96: [mediator webStateList:web_state_list_.get() Idem, you could pass nullptr here, right? https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:106: // Test that a url is loaded if the new active web state has zero navigation URL https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:44: web::WebState* webState = self.browser->web_state_list().GetActiveWebState(); I'd move this to the WebContentsMediator and here in the stop method I'd release the mediator: self.mediator = nil; self.viewController = nil;
https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:69: oldWebState->SetWebUsageEnabled(false); Did you copy this line from somewhere in the old app? Where, exactly? https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:96: [mediator webStateList:web_state_list_.get() Philosophical question: should we be "faking" WSL observer methods like this, or should we just call ActivateWebStateAt() and let the real observer machinery do the work? This method doesn't need to be in the public API of the mediator, so we could remove it if we tested the call indirectly. You could also imagine having a mediator doing something like DCHECK_EQ(new_web_state, list->GetActiveWebState()) and then failing because the state of the given WSL doesn't match what the observer method's arguments say is happening.
Patchset #3 (id:120001) has been deleted
The latest improvements to unittests implements a -disconnect method on the mediator. The mediator pattern will likely need to evolve to require something like this method for all mediators. If our thinking on mediators diverges from this approach, we can update this class in the future. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:69: oldWebState->SetWebUsageEnabled(false); On 2017/04/03 23:48:51, rohitrao wrote: > Did you copy this line from somewhere in the old app? Where, exactly? Line 23 of this file, prior to this change. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:76: - (void)updateActiveWebState:(web::WebState*)webState { On 2017/04/03 09:48:36, lpromero wrote: > Add a comment. The name is a bit misleading, it's not updating the active web > state, but updating the web contents consumer for the active web state. Done. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:72: web_state_list_->ActivateWebStateAt(0); On 2017/04/03 09:48:36, lpromero wrote: > Your web_state_list_ doesn’t include web_state_. Is it an issue? This setup was not very readable. I've attempted to make it better. PTAL. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:93: mediator.webStateList = web_state_list_.get(); On 2017/04/03 09:48:36, lpromero wrote: > The web state list is not used in the method you test. Is it a requirement to > have a web state list set here? This testing approach was not very readable. I've attempted to make it better. PTAL. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:96: [mediator webStateList:web_state_list_.get() On 2017/04/03 09:48:36, lpromero wrote: > Idem, you could pass nullptr here, right? This testing approach was not very readable. I've attempted to make it better. PTAL. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:96: [mediator webStateList:web_state_list_.get() On 2017/04/03 23:48:51, rohitrao wrote: > Philosophical question: should we be "faking" WSL observer methods like this, or > should we just call ActivateWebStateAt() and let the real observer machinery do > the work? > > This method doesn't need to be in the public API of the mediator, so we could > remove it if we tested the call indirectly. You could also imagine having a > mediator doing something like DCHECK_EQ(new_web_state, > list->GetActiveWebState()) and then failing because the state of the given WSL > doesn't match what the observer method's arguments say is happening. Both you and marq@ brought up this philosophical point. I agree and have changed the approach so as not to "fake" the observer methods. Now it uses the real observer machinery. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_contents_mediator_unittest.mm:106: // Test that a url is loaded if the new active web state has zero navigation On 2017/04/03 09:48:36, lpromero wrote: > URL Done. Found two instances. https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right): https://codereview.chromium.org/2780423003/diff/100001/ios/clean/chrome/brows... ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:44: web::WebState* webState = self.browser->web_state_list().GetActiveWebState(); On 2017/04/03 09:48:36, lpromero wrote: > I'd move this to the WebContentsMediator and here in the stop method I'd release > the mediator: > > self.mediator = nil; > self.viewController = nil; I added a -disconnect method. PTAL.
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
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 lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2780423003/#ps200001 (title: "Rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by edchin@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #5 (id:220001) has been deleted
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...
Patchset #4 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
Patchset #4 (id:240001) has been deleted
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 lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2780423003/#ps260001 (title: "Rebase.")
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 edchin@chromium.org
The CQ bit was checked by edchin@chromium.org
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": 1491543207347020,
"parent_rev": "18bf204d6960d6074f8bb5be4fb93af9e92ed908", "commit_rev":
"ac5bc014fa46a1c13fa1e5702d01593279ecee64"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Use web_state_list in web_contents_mediator This CL enables the user to select a tab in the tab switcher and the new tab is reflected immediately in the currently open tab container. BUG=none ========== to ========== [ios] Use web_state_list in web_contents_mediator This CL enables the user to select a tab in the tab switcher and the new tab is reflected immediately in the currently open tab container. BUG=none Review-Url: https://codereview.chromium.org/2780423003 Cr-Commit-Position: refs/heads/master@{#462776} Committed: https://chromium.googlesource.com/chromium/src/+/ac5bc014fa46a1c13fa1e5702d01... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:260001) as https://chromium.googlesource.com/chromium/src/+/ac5bc014fa46a1c13fa1e5702d01... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
