Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936003002:
[Tab Metrics] Adding the tab loading state to the WebContentsData. (Closed)
Description[Tab Metrics] Adding the tab loading state to the WebContentsData.
Adding the tab loading state as a preliminary step for adding the number of
switches to not loading, loading, and loaded tabs during session restore and
background tab loading.
Initially, the tab/WebContentsData starts as not loaded. We consider the tab to
be loading when the main frame begins a navigation. This happens with the
initial page navigation, if a page refreshes itself, or if navigating to a new
page and the WebContents is being reused. This does not include cases where
individual iframes update content after the page is loaded. The tab is
considered to be loaded when receiving the WebContentsObserver::DidStopLoading
signal, which corresponds to when the spinner stops.
BUG=732837
Review-Url: https://codereview.chromium.org/2936003002
Cr-Commit-Position: refs/heads/master@{#480108}
Committed: https://chromium.googlesource.com/chromium/src/+/2b1e1bf1a312185ac0162c31805b49333fc1cb3b
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address comments #
Total comments: 8
Patch Set 3 : Remove state check and fix style #
Total comments: 5
Patch Set 4 : Fix nits #
Total comments: 1
Patch Set 5 : Rename enum max #
Total comments: 8
Patch Set 6 : More comments, move test, inline state change function #
Total comments: 6
Patch Set 7 : Fix style issues, add TODO #
Messages
Total messages: 47 (14 generated)
shaseley@google.com changed reviewers: + fmeawad@chromium.org, panicker@chromium.org, zhenw@chromium.org
zhenw@google.com changed reviewers: + zhenw@google.com
https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:39: // iframe loading new content. Add TODO here to switch to done signal. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:49: // know if the load is happening in the main frame or an iframe. Add TODO here to consider NavigationThrottle later. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:28: TAB_IS_NOT_LOADING = 0, Are those states going to be part of the states in lifecycle? If so, the names is better to be consistent and maybe we can even just call it TabLifecycleState. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: RESOURCE_COORDINATOR_TAB_LOADING_STATES_UMA_MAX, I think you do not need RESOURCE_COORDINATOR_ prefix. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:114: TabLoadingState GetTabLoadingState() const; s/GetTabLoadingState/TabLoadingState This seems to be the style in this file. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:120: void MarkTabAsLoaded(); Why should those 2 functions be public? If your test needs to use those, you can make your test a FRIEND_TEST. See the example below for CopyState. And they can be combined into one function: SetTabLoadingState(state).
https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:39: // iframe loading new content. On 2017/06/13 18:57:39, zhenw wrote: > Add TODO here to switch to done signal. Done. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:49: // know if the load is happening in the main frame or an iframe. On 2017/06/13 18:57:39, zhenw wrote: > Add TODO here to consider NavigationThrottle later. Done. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:28: TAB_IS_NOT_LOADING = 0, On 2017/06/13 18:57:39, zhenw wrote: > Are those states going to be part of the states in lifecycle? If so, the names > is better to be consistent and maybe we can even just call it TabLifecycleState. Good question. I started with the names that exist in the session restore stats collector that tracks tab load states, but we should change this to whatever is best for this purpose. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: RESOURCE_COORDINATOR_TAB_LOADING_STATES_UMA_MAX, On 2017/06/13 18:57:39, zhenw wrote: > I think you do not need RESOURCE_COORDINATOR_ prefix. Done. https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:114: TabLoadingState GetTabLoadingState() const; On 2017/06/13 18:57:40, zhenw wrote: > s/GetTabLoadingState/TabLoadingState > > This seems to be the style in this file. Ah, yes. I prefixed with "Get" to avoid a name conflict with the enumeration. Convention also dictated not to pluralize the enum or use tab_loading_state() since it has to reach into tab_data_. I also wanted to avoid changing all the signatures to "enum TabLoadingState ...". Do you have a recommendation for this case? https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:120: void MarkTabAsLoaded(); On 2017/06/13 18:57:39, zhenw wrote: > Why should those 2 functions be public? If your test needs to use those, you can > make your test a FRIEND_TEST. See the example below for CopyState. > > And they can be combined into one function: SetTabLoadingState(state). Done.
https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/1/chrome/browser/resource_coo... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:28: TAB_IS_NOT_LOADING = 0, On 2017/06/13 21:45:25, shaseley wrote: > On 2017/06/13 18:57:39, zhenw wrote: > > Are those states going to be part of the states in lifecycle? If so, the names > > is better to be consistent and maybe we can even just call it > TabLifecycleState. > > Good question. I started with the names that exist in the session restore stats > collector that tracks tab load states, but we should change this to whatever is > best for this purpose. As discussed the lifecycle API states are about the app lifecycle. By proxy the hosting tab could be in the same state but they are separate things and tabs live longer than the hosted app.
The CQ bit was checked by shaseley@google.com 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...
https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:41: // TODO(shaseley): switch to the new done signal (network quiescence) when s/network quiescence/network and cpu quiescence https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:55: if (tab_data_.tab_loading_state != TAB_IS_NOT_LOADING && I don't think we should check tab_data_.tab_loading_state here. https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:58: } I think the style here is to error out early. That is: if (!navigation_handle->IsInMainFrame()) return; MarkTabAsLoading(); https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: TAB_LOADING_STATES_UMA_MAX, s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX
https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:41: // TODO(shaseley): switch to the new done signal (network quiescence) when On 2017/06/13 23:57:22, Zhen Wang wrote: > s/network quiescence/network and cpu quiescence Done. https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:55: if (tab_data_.tab_loading_state != TAB_IS_NOT_LOADING && On 2017/06/13 23:57:22, Zhen Wang wrote: > I don't think we should check tab_data_.tab_loading_state here. Done. https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:58: } On 2017/06/13 23:57:22, Zhen Wang wrote: > I think the style here is to error out early. That is: > > if (!navigation_handle->IsInMainFrame()) > return; > > MarkTabAsLoading(); Done.
On 2017/06/13 23:57:23, Zhen Wang wrote: > https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... > chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:55: if > (tab_data_.tab_loading_state != TAB_IS_NOT_LOADING && > I don't think we should check tab_data_.tab_loading_state here. > Should we not check it in DidStopLoading() then as well? It would probably make more sense to just let MarkTabAsLoad[ing|ed] check the state if it ever does anything more complex.
On 2017/06/14 00:14:16, shaseley wrote: > On 2017/06/13 23:57:23, Zhen Wang wrote: > > > https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... > > chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:55: if > > (tab_data_.tab_loading_state != TAB_IS_NOT_LOADING && > > I don't think we should check tab_data_.tab_loading_state here. > > > > Should we not check it in DidStopLoading() then as well? It would probably make > more sense to just let MarkTabAsLoad[ing|ed] check the state if it ever does > anything more complex. I think we should still check it in DidStopLoading(). MarkTabAsLoad[ing|ed] should just do what the function does. The complex logic is more related to the observer API. By the way, have you uploaded the new patch?
lgtm
On 2017/06/14 00:18:39, zhenw wrote: > On 2017/06/14 00:14:16, shaseley wrote: > > On 2017/06/13 23:57:23, Zhen Wang wrote: > > > > > > https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... > > > chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:55: if > > > (tab_data_.tab_loading_state != TAB_IS_NOT_LOADING && > > > I don't think we should check tab_data_.tab_loading_state here. > > > > > > > Should we not check it in DidStopLoading() then as well? It would probably > make > > more sense to just let MarkTabAsLoad[ing|ed] check the state if it ever does > > anything more complex. > > I think we should still check it in DidStopLoading(). > > MarkTabAsLoad[ing|ed] should just do what the function does. The complex logic > is more related to the observer API. I see, that makes sense. > By the way, have you uploaded the new patch? Yes, patch set 3 should have addressed all of the comments.
https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: TAB_LOADING_STATES_UMA_MAX, Just one nit that I think you forget this: s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX
https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: TAB_LOADING_STATES_UMA_MAX, On 2017/06/14 00:24:06, zhenw wrote: > Just one nit that I think you forget this: > s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX Done.
https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/20001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: TAB_LOADING_STATES_UMA_MAX, On 2017/06/13 23:57:23, Zhen Wang wrote: > s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX Done.
https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: TAB_LOADING_STATES_UMA_MAX, On 2017/06/14 00:31:11, shaseley wrote: > On 2017/06/14 00:24:06, zhenw wrote: > > Just one nit that I think you forget this: > > s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX > > Done. Also remove "UMA". Is that a requirement to report UMA?
https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: TAB_LOADING_STATES_UMA_MAX, On 2017/06/14 00:35:12, zhenw wrote: > On 2017/06/14 00:31:11, shaseley wrote: > > On 2017/06/14 00:24:06, zhenw wrote: > > > Just one nit that I think you forget this: > > > s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX > > > > Done. > > Also remove "UMA". Is that a requirement to report UMA? OK, will do. It's not a requirement, but was used in the session restore code that I was looking at while making this change. Happy to get rid of it.
https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: TAB_LOADING_STATES_UMA_MAX, On 2017/06/14 00:39:29, shaseley wrote: > On 2017/06/14 00:35:12, zhenw wrote: > > On 2017/06/14 00:31:11, shaseley wrote: > > > On 2017/06/14 00:24:06, zhenw wrote: > > > > Just one nit that I think you forget this: > > > > s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX > > > > > > Done. > > > > Also remove "UMA". Is that a requirement to report UMA? > > OK, will do. It's not a requirement, but was used in the session restore code > that I was looking at while making this change. Happy to get rid of it. Are you referring to SessionRestoreActionsUma? The reason of SESSION_RESTORE_ACTIONS_UMA_MAX is because the enum name already contains Uma. So I think the style is: enum SomeEnumName { ..., SOME_ENUM_NAME_MAX, }
On 2017/06/14 00:48:36, zhenw wrote: > https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... > File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h > (right): > > https://codereview.chromium.org/2936003002/diff/40001/chrome/browser/resource... > chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:31: > TAB_LOADING_STATES_UMA_MAX, > On 2017/06/14 00:39:29, shaseley wrote: > > On 2017/06/14 00:35:12, zhenw wrote: > > > On 2017/06/14 00:31:11, shaseley wrote: > > > > On 2017/06/14 00:24:06, zhenw wrote: > > > > > Just one nit that I think you forget this: > > > > > s/TAB_LOADING_STATES_UMA_MAX/TAB_LOADING_STATE_MAX > > > > > > > > Done. > > > > > > Also remove "UMA". Is that a requirement to report UMA? > > > > OK, will do. It's not a requirement, but was used in the session restore code > > that I was looking at while making this change. Happy to get rid of it. > > Are you referring to SessionRestoreActionsUma? > > The reason of SESSION_RESTORE_ACTIONS_UMA_MAX is because the enum name already > contains Uma. So I think the style is: > > enum SomeEnumName { > ..., > SOME_ENUM_NAME_MAX, > } Right, my mistake. FWIW, there are also some SOME_ENUM_NAME_COUNT (matches the README here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... MAX_SOME_ENUM_NAME floating around.
lgtm https://codereview.chromium.org/2936003002/diff/60001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc (right): https://codereview.chromium.org/2936003002/diff/60001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.cc:211: tab_loading_state == right.tab_loading_state; unrelated to this CL, I think the time checks should be at the top as they are more unique and they will shortcut the condition sooner if not same object.
https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data_unittest.cc (right): https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data_unittest.cc:212: EXPECT_EQ(TAB_IS_LOADED, tab_data()->GetTabLoadingState()); Can you move this test up next to the tab_data tests?
https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data_unittest.cc (right): https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data_unittest.cc:212: EXPECT_EQ(TAB_IS_LOADED, tab_data()->GetTabLoadingState()); On 2017/06/14 17:46:22, fmeawad wrote: > Can you move this test up next to the tab_data tests? Sure, moved it up just above the CopyState test, with the rest of the tab_data field tests.
lpy@chromium.org changed reviewers: + lpy@chromium.org
https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:154: void MarkTabAsLoading(); Are we planning to expose these two methods? IMHO if we are not planning to expose setting methods, then we should probably inline into the call sites and eliminate these two methods. And having two methods is too verbose for just setting a state. I would suggest two options: 1. Just have a SetTabLoadingState(TabLoadingState) method, or 2. Inline these two methods WDYT?
LGTM https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:27: enum TabLoadingState { Could you add a comment here explaining what triggers the state transitions, and note that okay to transition from loaded to loading for new nav. (similar to what's in the CL description: "..Initially, the tab starts as not loaded.... ")
https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:154: void MarkTabAsLoading(); On 2017/06/14 18:41:11, lpy wrote: > Are we planning to expose these two methods? Maybe. Depending on how the WebContentsData is made aware of the new state change signals, we may need to expose these to the TabManager. > IMHO if we are not planning to expose setting methods, then we should probably > inline into the call sites and eliminate these two methods. And having two > methods is too verbose for just setting a state. > > I would suggest two options: > > 1. Just have a SetTabLoadingState(TabLoadingState) method, or > 2. Inline these two methods > > WDYT? For this bug, maybe just doing (1) with inlining is the right thing to do? We can change this as needed when we change the state change signals or if state changes become more complex, like if they trigger observer events. Zhen and Fadi, thoughts?
On 2017/06/14 20:51:34, shaseley wrote: > https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... > File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h > (right): > > https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... > chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:154: void > MarkTabAsLoading(); > On 2017/06/14 18:41:11, lpy wrote: > > Are we planning to expose these two methods? > > Maybe. Depending on how the WebContentsData is made aware of the new state > change signals, we may need to expose these to the TabManager. > > > IMHO if we are not planning to expose setting methods, then we should probably > > inline into the call sites and eliminate these two methods. And having two > > methods is too verbose for just setting a state. > > > > I would suggest two options: > > > > 1. Just have a SetTabLoadingState(TabLoadingState) method, or > > 2. Inline these two methods > > > > WDYT? > > For this bug, maybe just doing (1) with inlining is the right thing to do? We > can change this as needed when we change the state change signals or if state > changes become more complex, like if they trigger observer events. > > Zhen and Fadi, thoughts? I defer to Zhen around this matter.
https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:154: void MarkTabAsLoading(); On 2017/06/14 20:51:34, shaseley wrote: > On 2017/06/14 18:41:11, lpy wrote: > > Are we planning to expose these two methods? > > Maybe. Depending on how the WebContentsData is made aware of the new state > change signals, we may need to expose these to the TabManager. > > > IMHO if we are not planning to expose setting methods, then we should probably > > inline into the call sites and eliminate these two methods. And having two > > methods is too verbose for just setting a state. > > > > I would suggest two options: > > > > 1. Just have a SetTabLoadingState(TabLoadingState) method, or > > 2. Inline these two methods > > > > WDYT? > > For this bug, maybe just doing (1) with inlining is the right thing to do? We > can change this as needed when we change the state change signals or if state > changes become more complex, like if they trigger observer events. > > Zhen and Fadi, thoughts? SetTabLoadingState(TabLoadingState) is good to me.
https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:27: enum TabLoadingState { On 2017/06/14 19:38:36, panicker wrote: > Could you add a comment here explaining what triggers the state transitions, and > note that okay to transition from loaded to loading for new nav. > (similar to what's in the CL description: "..Initially, the tab starts as not > loaded.... ") Done. https://codereview.chromium.org/2936003002/diff/80001/chrome/browser/resource... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:154: void MarkTabAsLoading(); On 2017/06/15 15:54:53, zhenw wrote: > On 2017/06/14 20:51:34, shaseley wrote: > > On 2017/06/14 18:41:11, lpy wrote: > > > Are we planning to expose these two methods? > > > > Maybe. Depending on how the WebContentsData is made aware of the new state > > change signals, we may need to expose these to the TabManager. > > > > > IMHO if we are not planning to expose setting methods, then we should > probably > > > inline into the call sites and eliminate these two methods. And having two > > > methods is too verbose for just setting a state. > > > > > > I would suggest two options: > > > > > > 1. Just have a SetTabLoadingState(TabLoadingState) method, or > > > 2. Inline these two methods > > > > > > WDYT? > > > > For this bug, maybe just doing (1) with inlining is the right thing to do? We > > can change this as needed when we change the state change signals or if state > > changes become more complex, like if they trigger observer events. > > > > Zhen and Fadi, thoughts? > > SetTabLoadingState(TabLoadingState) is good to me. Done.
lgtm
zhenw@chromium.org changed reviewers: + chrisha@chromium.org
+chrisha
The CQ bit was checked by shaseley@google.com 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...
Some nits... https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:27: // WebContents. The state changes to loaded when we receive the DidStopLoading Do we want to be using the WebContents::DidStopLoading signal, or the custom "loading done" signal we're adding as a session restore refinement? We should probably be consistent. (Which means, leave a TODO to migrate this to that signal once its ready?) https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:121: TabLoadingState GetTabLoadingState() const; This is a simple accessor, so should just be "tab_loading_state()". Meanwhile, the "set_time_to_purge" and others should actually be SetTimeToPurge, etc. If you care to clean these up and make them consistent in a follow-up CL that would be great :) https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:162: tab_data_.tab_loading_state = state; Inline both are neither of the new functions, but be consistent? (FWIW, I prefer inlining for simple accessors/mutators.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... File chrome/browser/resource_coordinator/tab_manager_web_contents_data.h (right): https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:27: // WebContents. The state changes to loaded when we receive the DidStopLoading On 2017/06/15 20:46:44, chrisha wrote: > Do we want to be using the WebContents::DidStopLoading signal, or the custom > "loading done" signal we're adding as a session restore refinement? We should > probably be consistent. > > (Which means, leave a TODO to migrate this to that signal once its ready?) Done. https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:121: TabLoadingState GetTabLoadingState() const; On 2017/06/15 20:46:44, chrisha wrote: > This is a simple accessor, so should just be "tab_loading_state()". Done. > Meanwhile, the "set_time_to_purge" and others should actually be SetTimeToPurge, > etc. If you care to clean these up and make them consistent in a follow-up CL > that would be great :) Will do! But, I'm a little confused about the setter style rule. I see set_* in a lot of places in the code, and the style guides are (IMO) a little ambiguous here: Google C++ --> OK for simple accessors/mutators, Chrome C++ style only mentions accessors, and in the context of inlining. https://codereview.chromium.org/2936003002/diff/100001/chrome/browser/resourc... chrome/browser/resource_coordinator/tab_manager_web_contents_data.h:162: tab_data_.tab_loading_state = state; On 2017/06/15 20:46:44, chrisha wrote: > Inline both are neither of the new functions, but be consistent? > > (FWIW, I prefer inlining for simple accessors/mutators.) Thanks, done; inlined the accessor as well.
lgtm!
The CQ bit was checked by shaseley@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@google.com, fmeawad@chromium.org, panicker@chromium.org, zhenw@chromium.org Link to the patchset: https://codereview.chromium.org/2936003002/#ps120001 (title: "Fix style issues, add TODO")
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": 1497634185423050,
"parent_rev": "120380c724ca5748ec6b1822121ca038c0f139df", "commit_rev":
"2b1e1bf1a312185ac0162c31805b49333fc1cb3b"}
Message was sent while issue was closed.
Description was changed from ========== [Tab Metrics] Adding the tab loading state to the WebContentsData. Adding the tab loading state as a preliminary step for adding the number of switches to not loading, loading, and loaded tabs during session restore and background tab loading. Initially, the tab/WebContentsData starts as not loaded. We consider the tab to be loading when the main frame begins a navigation. This happens with the initial page navigation, if a page refreshes itself, or if navigating to a new page and the WebContents is being reused. This does not include cases where individual iframes update content after the page is loaded. The tab is considered to be loaded when receiving the WebContentsObserver::DidStopLoading signal, which corresponds to when the spinner stops. BUG=732837 ========== to ========== [Tab Metrics] Adding the tab loading state to the WebContentsData. Adding the tab loading state as a preliminary step for adding the number of switches to not loading, loading, and loaded tabs during session restore and background tab loading. Initially, the tab/WebContentsData starts as not loaded. We consider the tab to be loading when the main frame begins a navigation. This happens with the initial page navigation, if a page refreshes itself, or if navigating to a new page and the WebContents is being reused. This does not include cases where individual iframes update content after the page is loaded. The tab is considered to be loaded when receiving the WebContentsObserver::DidStopLoading signal, which corresponds to when the spinner stops. BUG=732837 Review-Url: https://codereview.chromium.org/2936003002 Cr-Commit-Position: refs/heads/master@{#480108} Committed: https://chromium.googlesource.com/chromium/src/+/2b1e1bf1a312185ac0162c31805b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2b1e1bf1a312185ac0162c31805b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
