Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2674893002:
Refactor TabModel initialisation from a saved session. (Closed)
|
Created:
3 years, 10 months ago by sdefresne Modified:
3 years, 10 months ago Reviewers:
rohitrao (ping after 24h) CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor TabModel initialisation from a saved session.
There was two code path to restore Tabs from a saved sessions, one
in the TabModel initialiser and one in -restoreSessionWindow:. They
were almost the same with the few differences (used helper methods
vs manipulating _tabs directly, thus not sending notifications).
When considering the fact that there are no observer registered at
the time the initialiser is called, nor are there any Tabs in the
_tabs array, the only significant difference left was whether the
state was persisted after updating _currentTab.
Refactor the initialiser to call -restoreSessionWindow: method by
adding an helper method allowing to choose whether the state should
be persisted or not.
While restoring a session in the initialiser via the shared code path,
the selection of the selected tab will be recorded to be reported
as a metric. To not change the metric, the count is reset after
restoring the session in the initialiser.
BUG=687207
Review-Url: https://codereview.chromium.org/2674893002
Cr-Commit-Position: refs/heads/master@{#448598}
Committed: https://chromium.googlesource.com/chromium/src/+/44c09dc47b0df3de230ad862c98ffeccf1997699
Patch Set 1 #Patch Set 2 : Remove unused local variable. #
Total comments: 10
Patch Set 3 : Address comments & rebase on https://codereview.chromium.org/2678823002. #Patch Set 4 : Rebase. #Messages
Total messages: 19 (10 generated)
The CQ bit was checked by sdefresne@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...
sdefresne@chromium.org changed reviewers: + rohitrao@chromium.org
Please take a look.
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...)
lgtm https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:272: [self restoreSessionWindow:window persistState:NO]; Is it worth DCHECKing that _observers is still empty before calling restoreSessionWindow? It's possible that we could accidentally add an observer at some point in the future. https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:273: [self resetSessionMetrics]; This was another difference between the two functions -- whether or not adding the tabs logged any metrics. It may be worth calling this out in the CL description, and adding a line saying that metrics are logged, but immediately wiped. https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:524: TabParentingGlobalObserver::GetInstance()->OnTabParented(tab.webState); This is called a lot more often now, isn't it? It previously was only called when tabs were added during init.
Description was changed from ========== Refactor TabModel initialisation from a saved session. There was two code path to restore Tabs from a saved sessions, one in the TabModel initialiser and one in -restoreSessionWindow:. They were almost the same with the few differences (used helper methods vs manipulating _tabs directly, thus not sending notifications). When considering the fact that there are no observer registered at the time the initialiser is called, nor are there any Tabs in the _tabs array, the only significant difference left was whether the state was persisted after updating _currentTab. Refactor the initialiser to call -restoreSessionWindow: method by adding an helper method allowing to choose whether the state should be persisted or not. BUG=687207 ========== to ========== Refactor TabModel initialisation from a saved session. There was two code path to restore Tabs from a saved sessions, one in the TabModel initialiser and one in -restoreSessionWindow:. They were almost the same with the few differences (used helper methods vs manipulating _tabs directly, thus not sending notifications). When considering the fact that there are no observer registered at the time the initialiser is called, nor are there any Tabs in the _tabs array, the only significant difference left was whether the state was persisted after updating _currentTab. Refactor the initialiser to call -restoreSessionWindow: method by adding an helper method allowing to choose whether the state should be persisted or not. While restoring a session in the initialiser via the shared code path, the selection of the selected tab will be recorded to be reported as a metric. To not change the metric, the count is reset after restoring the session in the initialiser. BUG=687207 ==========
I've answered your questions. Could you take another look? https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:272: [self restoreSessionWindow:window persistState:NO]; On 2017/02/06 13:45:40, rohitrao wrote: > Is it worth DCHECKing that _observers is still empty before calling > restoreSessionWindow? It's possible that we could accidentally add an observer > at some point in the future. The only code path that can lead to adding observer is -addObserver:. Since this is a public method of that class, I can't see how it can be called before the initialiser returns. I can add a DCHECK here but I think it is not useful (especially given that _observers is reset 20 lines higher in the same method). https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:273: [self resetSessionMetrics]; On 2017/02/06 13:45:40, rohitrao wrote: > This was another difference between the two functions -- whether or not adding > the tabs logged any metrics. It may be worth calling this out in the CL > description, and adding a line saying that metrics are logged, but immediately > wiped. Done. https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:524: TabParentingGlobalObserver::GetInstance()->OnTabParented(tab.webState); On 2017/02/06 13:45:40, rohitrao wrote: > This is called a lot more often now, isn't it? It previously was only called > when tabs were added during init. The method should have been called here too. The method documentation says that TabParentingGlobalObserver "[a]llows clients to observe every tab (i.e., WebState) that is parented". Previously it was only called for Tab created in the initialiser when restoring a session during a cold start. So I think it was a bug that this method was not invoked here too. There are two client registering with TabParentingGlobalObserver: - IOSChromeLocalSessionEventRouter - IOSChromeMetricsServiceClient The second, IOSChromeMetricsServiceClient, informs the metrics code that the client is not idle, which I think is coreect (i.e. if the user opens a new tab, I would expect that would mean the client is not idle). The same method is called every time the user interact with the omnibox, so no ill-effect I think. The first, IOSChromeLocalSessionEventRouter, does inform the sync service that there were some activity on the WebState. Since it is a web::GlobalWebStateObserver, it will also be notified when any navigation events happens for the WebState, so I don't think it would have any ill-effect. BTW, I think that this method TabParentingGlobalObserver::GetInstance()->OnTabParented() may also have to be called from -replaceTab:withTab: since this cause the Tab* to be parented with TabModel. WDYT?
https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:272: [self restoreSessionWindow:window persistState:NO]; As an example, what would happen if we modified _tabUsageRecorder or _syncedWindowDelegate to add themselves as delegates? What happens if you call a public method on an object that's still being initialized? =) https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:524: TabParentingGlobalObserver::GetInstance()->OnTabParented(tab.webState); I looked at the callsites as well and decided that this change would be fine. Should we move it (and the additional call in replaceTab) to a separate CL, since these are larger behavioral changes than anything else in this CL?
https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:272: [self restoreSessionWindow:window persistState:NO]; On 2017/02/06 14:15:57, rohitrao wrote: > As an example, what would happen if we modified _tabUsageRecorder or > _syncedWindowDelegate to add themselves as delegates? What happens if you call > a public method on an object that's still being initialized? =) Called public method on an object that is not initialised can happens but is discouraged (this is why the style recommend not using properties in the initialiser as a subclass may do some processing in the property setter). We've also had issues with some classes we inherited from that called a public method at the end of their initialiser (something like [self reloadData]) in MDC IIRC. I could argue that adding _syncedWindowDelegate or _tabUsageRecorder to _observers would be something that the owner of TabModel would make intentionally and thus they would be able to decide whether they want to be invoked for the events (if they don't they can change the order of the object initialisation) :-) But since I don't care that much, I've just added the DCHECK. https://codereview.chromium.org/2674893002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:524: TabParentingGlobalObserver::GetInstance()->OnTabParented(tab.webState); On 2017/02/06 14:15:57, rohitrao wrote: > I looked at the callsites as well and decided that this change would be fine. > Should we move it (and the additional call in replaceTab) to a separate CL, > since these are larger behavioral changes than anything else in this CL? Sure, done => https://codereview.chromium.org/2678823002.
PTAL
lgtm
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2674893002/#ps60001 (title: "Rebase.")
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": 60001, "attempt_start_ts": 1486464548643400,
"parent_rev": "3867c9867bbb9bbe1d1d4c2c68b60073cb45caaf", "commit_rev":
"44c09dc47b0df3de230ad862c98ffeccf1997699"}
Message was sent while issue was closed.
Description was changed from ========== Refactor TabModel initialisation from a saved session. There was two code path to restore Tabs from a saved sessions, one in the TabModel initialiser and one in -restoreSessionWindow:. They were almost the same with the few differences (used helper methods vs manipulating _tabs directly, thus not sending notifications). When considering the fact that there are no observer registered at the time the initialiser is called, nor are there any Tabs in the _tabs array, the only significant difference left was whether the state was persisted after updating _currentTab. Refactor the initialiser to call -restoreSessionWindow: method by adding an helper method allowing to choose whether the state should be persisted or not. While restoring a session in the initialiser via the shared code path, the selection of the selected tab will be recorded to be reported as a metric. To not change the metric, the count is reset after restoring the session in the initialiser. BUG=687207 ========== to ========== Refactor TabModel initialisation from a saved session. There was two code path to restore Tabs from a saved sessions, one in the TabModel initialiser and one in -restoreSessionWindow:. They were almost the same with the few differences (used helper methods vs manipulating _tabs directly, thus not sending notifications). When considering the fact that there are no observer registered at the time the initialiser is called, nor are there any Tabs in the _tabs array, the only significant difference left was whether the state was persisted after updating _currentTab. Refactor the initialiser to call -restoreSessionWindow: method by adding an helper method allowing to choose whether the state should be persisted or not. While restoring a session in the initialiser via the shared code path, the selection of the selected tab will be recorded to be reported as a metric. To not change the metric, the count is reset after restoring the session in the initialiser. BUG=687207 Review-Url: https://codereview.chromium.org/2674893002 Cr-Commit-Position: refs/heads/master@{#448598} Committed: https://chromium.googlesource.com/chromium/src/+/44c09dc47b0df3de230ad862c98f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/44c09dc47b0df3de230ad862c98f... |
