Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815913004:
Use ScopedObserver with WebStateListObserverBridge in FIP mediator (Closed)
|
Created:
3 years, 8 months ago by lpromero Modified:
3 years, 8 months ago Reviewers:
sdefresne CC:
chromium-reviews, marq+scrutinize_chromium.org, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, ios-reviews+clean_chromium.org, marq+watch_chromium.org, lpromero+watch_chromium.org, sdefresne+watch_chromium.org, rohitrao (ping after 24h) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse ScopedObserver with WebStateListObserverBridge in FIP mediator
BUG=none
R=sdefresne@chromium.org
CC=rohitrao@chromium.org
Review-Url: https://codereview.chromium.org/2815913004
Cr-Commit-Position: refs/heads/master@{#465593}
Committed: https://chromium.googlesource.com/chromium/src/+/bf434a47ac992ba878ab24524548bba55e967a0a
Patch Set 1 #Patch Set 2 : Null initialize the we_state_list_ when not used. #Patch Set 3 : Use ScopedObserver #Patch Set 4 : Fix scoped #
Total comments: 2
Patch Set 5 : Fix scoped #
Total comments: 2
Patch Set 6 : Move ivar layout #Patch Set 7 : Fix reference #Messages
Total messages: 44 (34 generated)
The CQ bit was checked by lpromero@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 lpromero@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.
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
not lgtm WebStateListObserver can observe multiple WebStateLists like all observers (except WebStateObserver which has a really strange API).
The CQ bit was checked by lpromero@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...
Description was changed from ========== Add constructor to WebStateListObserver to automatically start observing. BUG=none R=rohitrao@chromium.org CC=sdefresne@chromium.org ========== to ========== Use ScopedObserver with WebStateListObserverBridge in FIP mediator BUG=none R=sdefresne@chromium.org CC=rohitrao@chromium.org ==========
lpromero@chromium.org changed reviewers: - rohitrao@chromium.org
Updated to use ScopedObserver.
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 lpromero@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...
Fixed the issue with the scoped observer where I passed in a MakeUnique.get().
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...)
https://codereview.chromium.org/2815913004/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right): https://codereview.chromium.org/2815913004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:39: std::unique_ptr<ScopedObserver<WebStateList, WebStateListObserverBridge>> Move this below the declaration of _webStateListObserver to ensure that the RemoveObserver call happens before the WebStateListObserverBridge is destroyed.
The CQ bit was checked by lpromero@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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
https://codereview.chromium.org/2815913004/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right): https://codereview.chromium.org/2815913004/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:63: &_webStateListObserver); This should be _webStateList.get()
The CQ bit was checked by lpromero@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 lpromero@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.
https://codereview.chromium.org/2815913004/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right): https://codereview.chromium.org/2815913004/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:39: std::unique_ptr<ScopedObserver<WebStateList, WebStateListObserverBridge>> On 2017/04/19 13:19:42, sdefresne wrote: > Move this below the declaration of _webStateListObserver to ensure that the > RemoveObserver call happens before the WebStateListObserverBridge is destroyed. Done. https://codereview.chromium.org/2815913004/diff/80001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right): https://codereview.chromium.org/2815913004/diff/80001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:63: &_webStateListObserver); On 2017/04/19 13:39:19, sdefresne wrote: > This should be _webStateList.get() Done. (_webStateListObserver.get())
lgtm
The CQ bit was checked by lpromero@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": 120001, "attempt_start_ts": 1492611352918660,
"parent_rev": "4c8239ac8c3c3dd581963aeb6e5dcb2e381d0b58", "commit_rev":
"bf434a47ac992ba878ab24524548bba55e967a0a"}
Message was sent while issue was closed.
Description was changed from ========== Use ScopedObserver with WebStateListObserverBridge in FIP mediator BUG=none R=sdefresne@chromium.org CC=rohitrao@chromium.org ========== to ========== Use ScopedObserver with WebStateListObserverBridge in FIP mediator BUG=none R=sdefresne@chromium.org CC=rohitrao@chromium.org Review-Url: https://codereview.chromium.org/2815913004 Cr-Commit-Position: refs/heads/master@{#465593} Committed: https://chromium.googlesource.com/chromium/src/+/bf434a47ac992ba878ab24524548... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/bf434a47ac992ba878ab24524548... |
