Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906313003:
[ios clean] ToolsMenu Mediator observes Webstate. (Closed)
Description[ios clean] ToolsMenu Mediator observes Webstate.
-ToolsMenu observes Webstate so it can correctly update the Overflow controls
stop/reload buttons.
-Some unittest refactoring.
Display stop while loading screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM
Display reload after it has loaded screenshot:
https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU
BUG=682880
Review-Url: https://codereview.chromium.org/2906313003
Cr-Commit-Position: refs/heads/master@{#476103}
Committed: https://chromium.googlesource.com/chromium/src/+/f58f1c268f7ec4a304a87e23d117b563188de22d
Patch Set 1 #
Total comments: 26
Patch Set 2 : CL Feedback #
Total comments: 2
Patch Set 3 : Improved comments and test nits #
Total comments: 1
Messages
Total messages: 30 (20 generated)
Description was changed from ========== [ios clean] ToolsMenu observes Webstate. BUG=682880 ========== to ========== [ios clean] ToolsMenu observes Webstate. ToolsMenu observes Webstate so it can correctly update the Overflow controls stop/reload buttons. Display stop while loading screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM Display reload after it has loaded screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU BUG=682880 ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, marq@chromium.org
Description was changed from ========== [ios clean] ToolsMenu observes Webstate. ToolsMenu observes Webstate so it can correctly update the Overflow controls stop/reload buttons. Display stop while loading screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM Display reload after it has loaded screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU BUG=682880 ========== to ========== [ios clean] ToolsMenu observes Webstate. -ToolsMenu observes Webstate so it can correctly update the Overflow controls stop/reload buttons. -Some unittest refactoring. Display stop while loading screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM Display reload after it has loaded screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU BUG=682880 ==========
The CQ bit was checked by sczs@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 ========== [ios clean] ToolsMenu observes Webstate. -ToolsMenu observes Webstate so it can correctly update the Overflow controls stop/reload buttons. -Some unittest refactoring. Display stop while loading screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM Display reload after it has loaded screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU BUG=682880 ========== to ========== [ios clean] ToolsMenu Mediator observes Webstate. -ToolsMenu observes Webstate so it can correctly update the Overflow controls stop/reload buttons. -Some unittest refactoring. Display stop while loading screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM Display reload after it has loaded screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU BUG=682880 ==========
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sczs@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...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_coordinator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.h:11: class WebState; nit: alpha order for forward declarations, so web::WebState comes after ToolsMenuConfiguration. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.h:22: @property(nonatomic, assign, nullable) web::WebState* webState; Is 'nullable' meaningful for pointers to C++ objects? https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.h:20: andConfiguration: Naming nit: Don't use 'and' to connect keywords in method names. 'and' is only used to indicate a second action the method performs ("-doSomethingWithThis:andDoSomethingElseWithThis:"). Generally keywords after the first in initializers shouldn't need prepositions or conjunctions, so 'initWithConsumer:configuration:' should be sufficient. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:24: @interface TestToolsMediator : ToolsMediator<CRWWebStateObserver> This doesn't seem to be necessary. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:35: std::unique_ptr<ToolbarTestNavigationManager> navigation_manager = Use a C++ initializer list here? https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:39: configuration_ = OCMClassMock([ToolsMenuConfiguration class]); I'd prefer not to use a mock for ToolsMenuConfiguration, since it's really just a read-only data object. We don't need to verify that any calls are made against it. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:63: mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_ Any reason to not initialize mediator_ in the test initializer? https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:115: OCMStub([configuration_ isInTabSwitcher]).andReturn(NO); Should you also test these cases when -isInTabSwitcher is YES?
Addressed the comments. PTAL. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_coordinator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.h:11: class WebState; On 2017/05/30 09:24:54, marq (ping after 24h) wrote: > nit: alpha order for forward declarations, so web::WebState comes after > ToolsMenuConfiguration. Done. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.h:22: @property(nonatomic, assign, nullable) web::WebState* webState; On 2017/05/30 09:24:54, marq (ping after 24h) wrote: > Is 'nullable' meaningful for pointers to C++ objects? Yes, it works with C++ objects. I think the most meaningful thing is the documentation part of stating that this can be nullptr. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.h:20: andConfiguration: On 2017/05/30 09:24:54, marq (ping after 24h) wrote: > Naming nit: Don't use 'and' to connect keywords in method names. 'and' is only > used to indicate a second action the method performs > ("-doSomethingWithThis:andDoSomethingElseWithThis:"). > > Generally keywords after the first in initializers shouldn't need prepositions > or conjunctions, so 'initWithConsumer:configuration:' should be sufficient. Thanks for the tip! Done. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.h:27: @property(nonatomic, assign, nullable) web::WebState* webState; Thought it would be a good idea to start using Nullability annotations after the WSL discussion. I know we still haven't decided if we are going to use them, or which style to follow. So I can remove these if you have any objection or concern. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:24: @interface TestToolsMediator : ToolsMediator<CRWWebStateObserver> On 2017/05/30 09:24:54, marq (ping after 24h) wrote: > This doesn't seem to be necessary. You're right. Done. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:35: std::unique_ptr<ToolbarTestNavigationManager> navigation_manager = On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > Use a C++ initializer list here? Done, though I'm not sure if this is what you meant. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:39: configuration_ = OCMClassMock([ToolsMenuConfiguration class]); On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > I'd prefer not to use a mock for ToolsMenuConfiguration, since it's really just > a read-only data object. We don't need to verify that any calls are made against > it. Done. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:63: mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_ On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > Any reason to not initialize mediator_ in the test initializer? Since the configuration is not exposed as a property it can only be set when initializing the mediator. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:115: OCMStub([configuration_ isInTabSwitcher]).andReturn(NO); On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > Should you also test these cases when -isInTabSwitcher is YES? You're right, it should always reject! Since no Webstate is supposed to be set then. I will also add this Check on the Mediator code. Done.
LGTM with nits. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_coordinator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.h:22: @property(nonatomic, assign, nullable) web::WebState* webState; On 2017/05/31 03:05:47, sczs wrote: > On 2017/05/30 09:24:54, marq (ping after 24h) wrote: > > Is 'nullable' meaningful for pointers to C++ objects? > > Yes, it works with C++ objects. I think the most meaningful thing is the > documentation part of stating that this can be nullptr. My current thinking on nullability annotations are that they were added by Apple to make interoperating with Swift's monads, but they are starting to be used as if they were enforced the way 'const' is in C++ (which they are not). I'm fine with this as-is, but remember we can always just use comments as well. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.h:27: @property(nonatomic, assign, nullable) web::WebState* webState; On 2017/05/31 03:05:47, sczs wrote: > Thought it would be a good idea to start using Nullability annotations after the > WSL discussion. I know we still haven't decided if we are going to use them, or > which style to follow. So I can remove these if you have any objection or > concern. We have them in other places. As a matter of general ObjC style, it's under heavy discussion; I myself don't have a clear idea about what I'd like to see. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:35: std::unique_ptr<ToolbarTestNavigationManager> navigation_manager = On 2017/05/31 03:05:50, sczs wrote: > On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > > Use a C++ initializer list here? > > Done, though I'm not sure if this is what you meant. It is, but I meant for all of the member variables. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:63: mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_ On 2017/05/31 03:05:51, sczs wrote: > On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > > Any reason to not initialize mediator_ in the test initializer? > > Since the configuration is not exposed as a property it can only be set when > initializing the mediator. And calling the initializer triggers updates to the consumer. No change for this CL, but I think I prefer the pattern where we set the consumer through a property. There's some additional cost in managing state, but it means that initialization and action are separated. https://codereview.chromium.org/2906313003/diff/40001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.mm (right): https://codereview.chromium.org/2906313003/diff/40001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:64: // If in tab switcher we don't need to observe the Webstate since there are no Don't use 'we' in comments. Maybe reword to something like: " In the tab switcher, there's no overflow controls to update, so the WebState isn't observed. "
The CQ bit was checked by sczs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org Link to the patchset: https://codereview.chromium.org/2906313003/#ps60001 (title: "Improved comments and test nits")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sczs@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_coordinator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_coordinator.h:22: @property(nonatomic, assign, nullable) web::WebState* webState; On 2017/05/31 10:40:31, marq (ping after 24h) wrote: > On 2017/05/31 03:05:47, sczs wrote: > > On 2017/05/30 09:24:54, marq (ping after 24h) wrote: > > > Is 'nullable' meaningful for pointers to C++ objects? > > > > Yes, it works with C++ objects. I think the most meaningful thing is the > > documentation part of stating that this can be nullptr. > > My current thinking on nullability annotations are that they were added by Apple > to make interoperating with Swift's monads, but they are starting to be used as > if they were enforced the way 'const' is in C++ (which they are not). I'm fine > with this as-is, but remember we can always just use comments as well. Acknowledged. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.h (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.h:27: @property(nonatomic, assign, nullable) web::WebState* webState; On 2017/05/31 10:40:31, marq (ping after 24h) wrote: > On 2017/05/31 03:05:47, sczs wrote: > > Thought it would be a good idea to start using Nullability annotations after > the > > WSL discussion. I know we still haven't decided if we are going to use them, > or > > which style to follow. So I can remove these if you have any objection or > > concern. > > We have them in other places. As a matter of general ObjC style, it's under > heavy discussion; I myself don't have a clear idea about what I'd like to see. Acknowledged. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:35: std::unique_ptr<ToolbarTestNavigationManager> navigation_manager = On 2017/05/31 10:40:32, marq (ping after 24h) wrote: > On 2017/05/31 03:05:50, sczs wrote: > > On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > > > Use a C++ initializer list here? > > > > Done, though I'm not sure if this is what you meant. > > It is, but I meant for all of the member variables. Done. https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:63: mediator_ = [[TestToolsMediator alloc] initWithConsumer:consumer_ On 2017/05/31 10:40:32, marq (ping after 24h) wrote: > On 2017/05/31 03:05:51, sczs wrote: > > On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > > > Any reason to not initialize mediator_ in the test initializer? > > > > Since the configuration is not exposed as a property it can only be set when > > initializing the mediator. > > And calling the initializer triggers updates to the consumer. No change for this > CL, but I think I prefer the pattern where we set the consumer through a > property. There's some additional cost in managing state, but it means that > initialization and action are separated. Acknowledged. https://codereview.chromium.org/2906313003/diff/40001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator.mm (right): https://codereview.chromium.org/2906313003/diff/40001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator.mm:64: // If in tab switcher we don't need to observe the Webstate since there are no On 2017/05/31 10:40:32, marq (ping after 24h) wrote: > Don't use 'we' in comments. Maybe reword to something like: > > " In the tab switcher, there's no overflow controls to update, so the WebState > isn't observed. " Done.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496265703202650,
"parent_rev": "de4a076b99fbbb93298df1e3af27945b3a718de2", "commit_rev":
"f58f1c268f7ec4a304a87e23d117b563188de22d"}
Message was sent while issue was closed.
Description was changed from ========== [ios clean] ToolsMenu Mediator observes Webstate. -ToolsMenu observes Webstate so it can correctly update the Overflow controls stop/reload buttons. -Some unittest refactoring. Display stop while loading screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM Display reload after it has loaded screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU BUG=682880 ========== to ========== [ios clean] ToolsMenu Mediator observes Webstate. -ToolsMenu observes Webstate so it can correctly update the Overflow controls stop/reload buttons. -Some unittest refactoring. Display stop while loading screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgcFppc1hyU1VaSHM Display reload after it has loaded screenshot: https://drive.google.com/open?id=0Byo6-Nuda2jgTWx4UzhJVUZlclU BUG=682880 Review-Url: https://codereview.chromium.org/2906313003 Cr-Commit-Position: refs/heads/master@{#476103} Committed: https://chromium.googlesource.com/chromium/src/+/f58f1c268f7ec4a304a87e23d117... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f58f1c268f7ec4a304a87e23d117...
Message was sent while issue was closed.
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): https://codereview.chromium.org/2906313003/diff/20001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:35: std::unique_ptr<ToolbarTestNavigationManager> navigation_manager = On 2017/05/30 09:24:55, marq (ping after 24h) wrote: > Use a C++ initializer list here? This was incorrect, those were not member variables but local variables. https://codereview.chromium.org/2906313003/diff/60001/ios/clean/chrome/browse... File ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm (right): https://codereview.chromium.org/2906313003/diff/60001/ios/clean/chrome/browse... ios/clean/chrome/browser/ui/tools/tools_mediator_unittest.mm:32: navigation_manager(base::MakeUnique<ToolbarTestNavigationManager>()) { This should not be a member variable if you move it. Instead it should be a local variable (original code before comment by marq was correct). Can you fix as a followup? Correct code should be: ToolsMediatorTest() : consumer_(OCMProtocolMock(@protocol(ToolsConsumer))), configuration_( [[ToolsMenuConfiguration alloc] initWithDisplayView:nil]) { auto navigation_manager = base::MakeUnique<ToolbarTestNavigationManager>(); test_web_state_.SetNavigationManager(std::move(navigation_manager)); } ToolbarTestNavigationManager* navigation_manager() { return static_cast<ToolbarTestNavigationManager*>( test_web_state_.GetNavigationManager()); } Then change the code to access the navigation manager via the accessor instead of the local raw pointer (and remove the navigation_manager_ and navigation_manager member fields). |
