Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9323015:
[Sync] - In about:sync page display if the first sync after restart has taken place or not. (Closed)
|
Created:
8 years, 10 months ago by lipalani1 Modified:
8 years, 6 months ago Reviewers:
rlarocque CC:
chromium-reviews, Raghu Simha, ncarter (slow), arv (Not doing code reviews), akalin, tim (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAfter restart if a sync has not taken place then most of the values in about:sync page are speculative. Indicate that by a new summary called UnInitialized and in the description say that the values displayed here are speculative.
BUG=112229
TEST=try bots
Patch Set 1 #
Total comments: 14
Patch Set 2 : For review. #Patch Set 3 : For review. #
Total comments: 2
Patch Set 4 : For review. #
Total comments: 3
Messages
Total messages: 12 (0 generated)
Please review.
Some comments. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... File chrome/browser/sync/engine/all_status.cc (left): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... chrome/browser/sync/engine/all_status.cc:37: status.initial_sync_ended = false; How is this change related to the intentions described in the commit message? http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... File chrome/browser/sync/engine/all_status.cc (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... chrome/browser/sync/engine/all_status.cc:104: status_.summary = sync_api::SyncManager::Status::OFFLINE_UNUSABLE; Why? This would make the OFFLINE_UNSYNCED branch a higher priority than the OFFLINE_UNUSABLE branch, meaning that in many cases where we used to display OFFLINE_UNUSABLE, we will now display OFFLINE_UNSYNCED. I'm not necessarily against the change, but I'd like to know why it's being made. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... chrome/browser/sync/engine/all_status.cc:105: } For completeness, I'd like to see an else branch that handles the status_.sync_count == 0 case. That should make it more clear to readers that there is a specific summary value intended to be used when we haven't attempted to sync yet. Otherwise, this behaviour might look like an accident. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/internal_ap... File chrome/browser/sync/internal_api/sync_manager.h (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/internal_ap... chrome/browser/sync/internal_api/sync_manager.h:98: UNININITIALIZED, I no longer think this is a good name. It makes sense in this context (especially in the constructor's member initializers) but not in about:sync. I worry it could be misinterpreted as meaning that the sync service is not initialized, as opposed to indicating that the sync status has not been initialized. Maybe UNKNOWN would be a better choice? Sorry about the flip-flop. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... File chrome/browser/sync/sync_ui_util.cc (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... chrome/browser/sync/sync_ui_util.cc:513: if (full_status.summary == sync_api::SyncManager::Status::UNININITIALIZED) { I'm not sure this is a good idea. What is the purpose of about:sync? It's certainly not built for users. If it were, it would be prettier and internationalized. If the purpose is to provide useful information for developers, then this is unnecessary. A developer who wishes to know the cause of this status would read the code to find out where it came from. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... chrome/browser/sync/sync_ui_util.cc:570: full_status.sync_count > 0); That's not what initial_sync_ended used to mean. The old initial_sync_ended was set only when we had fully downloaded all data for all enabled data types. This usually requires several *successful* sync sessions, and is based on information that is persisted to the database after first-time sync.
PTAL. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... File chrome/browser/sync/engine/all_status.cc (left): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... chrome/browser/sync/engine/all_status.cc:37: status.initial_sync_ended = false; I thought about it some more. I had few reasons for removing this but have to agree with you that may be it should be a patch by itself. On 2012/02/02 22:44:13, rlarocque wrote: > How is this change related to the intentions described in the commit message? http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... File chrome/browser/sync/engine/all_status.cc (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... chrome/browser/sync/engine/all_status.cc:104: status_.summary = sync_api::SyncManager::Status::OFFLINE_UNUSABLE; If you look at the comments for this enum OFFLINE UNSYNCED means there are local changes and sync failed. OFFLINE UNUSABLE means there are no local changes and sync failed. Poor choice of names and probably a better way of expression them would be as 2 seperate variables. one just saying offline and other saying unsynced is true/false. but doing that would be beyond the scope of this CL. I will add a todo. On 2012/02/02 22:44:13, rlarocque wrote: > Why? This would make the OFFLINE_UNSYNCED branch a higher priority than the > OFFLINE_UNUSABLE branch, meaning that in many cases where we used to display > OFFLINE_UNUSABLE, we will now display OFFLINE_UNSYNCED. > > I'm not necessarily against the change, but I'd like to know why it's being > made. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... chrome/browser/sync/engine/all_status.cc:105: } On 2012/02/02 22:44:13, rlarocque wrote: > For completeness, I'd like to see an else branch that handles the > status_.sync_count == 0 case. That should make it more clear to readers that > there is a specific summary value intended to be used when we haven't attempted > to sync yet. Otherwise, this behaviour might look like an accident. Done. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/internal_ap... File chrome/browser/sync/internal_api/sync_manager.h (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/internal_ap... chrome/browser/sync/internal_api/sync_manager.h:98: UNININITIALIZED, our target audience is: 1. We expect only developers to see this. But the developer need not work on chrome sync or he could just be a chrome committer. What I have done is the status text would read READY but the description would read there has been no sync since restart so the following values are speculative. With that in mind I think UNINITIALIZED looks to be a reasonably right name for this enum. On 2012/02/02 22:44:13, rlarocque wrote: > I no longer think this is a good name. It makes sense in this context > (especially in the constructor's member initializers) but not in about:sync. I > worry it could be misinterpreted as meaning that the sync service is not > initialized, as opposed to indicating that the sync status has not been > initialized. > > Maybe UNKNOWN would be a better choice? > > Sorry about the flip-flop. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... File chrome/browser/sync/sync_ui_util.cc (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... chrome/browser/sync/sync_ui_util.cc:513: if (full_status.summary == sync_api::SyncManager::Status::UNININITIALIZED) { Answer in the previous comment. On 2012/02/02 22:44:13, rlarocque wrote: > I'm not sure this is a good idea. > > What is the purpose of about:sync? It's certainly not built for users. If it > were, it would be prettier and internationalized. > > If the purpose is to provide useful information for developers, then this is > unnecessary. A developer who wishes to know the cause of this status would read > the code to find out where it came from. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... chrome/browser/sync/sync_ui_util.cc:570: full_status.sync_count > 0); code reverted. I kind of don't like that piece of information as it is misleading. people could interpret this as the first sync of the account or first sync after restart. but may be in another CL. On 2012/02/02 22:44:13, rlarocque wrote: > That's not what initial_sync_ended used to mean. The old initial_sync_ended was > set only when we had fully downloaded all data for all enabled data types. This > usually requires several *successful* sync sessions, and is based on information > that is persisted to the database after first-time sync.
More comments. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... File chrome/browser/sync/engine/all_status.cc (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/engine/all_... chrome/browser/sync/engine/all_status.cc:104: status_.summary = sync_api::SyncManager::Status::OFFLINE_UNUSABLE; On 2012/02/03 01:55:39, lipalani1 wrote: > If you look at the comments for this enum OFFLINE UNSYNCED means there are local > changes and sync failed. OFFLINE UNUSABLE means there are no local changes and > sync failed. Poor choice of names and probably a better way of expression them > would be as 2 seperate variables. one just saying offline and other saying > unsynced is true/false. but doing that would be beyond the scope of this CL. I > will add a todo. OK, but this is still a big change to the definition of OFFLINE_UNUSABLE. Whereas OFFLINE_UNUSABLE used to be based on sync setup having failed or not yet completed (!status_.initial_sync_ended), this bases it off of the number of syncs since last restart. Could you make defer this change to that separate CL with the other initial_sync_ended changes? btw, I've seen the most recent patch set (#3). I don't think removing the state is the right way to go, either. http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... File chrome/browser/sync/sync_ui_util.cc (right): http://codereview.chromium.org/9323015/diff/1/chrome/browser/sync/sync_ui_uti... chrome/browser/sync/sync_ui_util.cc:513: if (full_status.summary == sync_api::SyncManager::Status::UNININITIALIZED) { On 2012/02/03 01:55:39, lipalani1 wrote: > Answer in the previous comment. > On 2012/02/02 22:44:13, rlarocque wrote: > > I'm not sure this is a good idea. > > > > What is the purpose of about:sync? It's certainly not built for users. If it > > were, it would be prettier and internationalized. > > > > If the purpose is to provide useful information for developers, then this is > > unnecessary. A developer who wishes to know the cause of this status would > read > > the code to find out where it came from. > I still think this is unusual. We have plenty of other variables that are even more misleading that don't have any explanatory text. Why annotate this one? Does it make sense to hard-code the explanation here, rather than in AllStatus where the flag is given meaning in the first place? Wouldn't it be easier to give the value an easier to interpret name? http://codereview.chromium.org/9323015/diff/14/chrome/browser/sync/internal_a... File chrome/browser/sync/internal_api/sync_manager.h (right): http://codereview.chromium.org/9323015/diff/14/chrome/browser/sync/internal_a... chrome/browser/sync/internal_api/sync_manager.h:109: // Can't connect to server. If we're going to change the meaning of an enum, we should change its name, too, just to avoid confusion. Otherwise we'll have to go digging through commit logs to figure out what "OFFLINE_UNUSABLE" actually means for a given revision number every time that status shows up on crbug. Regardless, I don't think we should be making this change in a commit that is meant to fix a different bug and is headed for M18. See also: comment in all_status.cc
Sorry got caught up in other patches. The OFFLINE_UNUSED http://codereview.chromium.org/9323015/diff/14/chrome/browser/sync/internal_a... File chrome/browser/sync/internal_api/sync_manager.h (right): http://codereview.chromium.org/9323015/diff/14/chrome/browser/sync/internal_a... chrome/browser/sync/internal_api/sync_manager.h:109: // Can't connect to server. On 2012/02/03 18:55:03, rlarocque wrote: > If we're going to change the meaning of an enum, we should change its name, too, > just to avoid confusion. Otherwise we'll have to go digging through commit logs > to figure out what "OFFLINE_UNUSABLE" actually means for a given revision number > every time that status shows up on crbug. > > Regardless, I don't think we should be making this change in a commit that is > meant to fix a different bug and is headed for M18. > > See also: comment in all_status.cc Done.
Hit send too soon. The offline_unused was an oversight in my part. put the code back exactly the way it was. However there is some refactoring in CalcStatusChanges. Now for the uninitialized variable, I think the goal is for non sync developers who only look at that text to not get confused. A descriptive name is great, if we can find one. In the absence of one the extra explanation is justified as it clarifies things much easily for non sync developers.
On 2012/02/07 23:36:45, lipalani1 wrote: > Hit send too soon. > The offline_unused was an oversight in my part. put the code back exactly the > way it was. > > However there is some refactoring in CalcStatusChanges. > > Now for the uninitialized variable, I think the goal is for non sync developers > who only look at that text to not get confused. A descriptive name is great, if > we can find one. In the absence of one the extra explanation is justified as it > clarifies things much easily for non sync developers. I still think the explanatory text is asking for trouble. Are you sure there's no way you could express this information with a better named enum value? No other summary value requires explanatory text.
Inline comments did not publish on the first try. Let's try again... http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/engine/a... File chrome/browser/sync/engine/all_status.cc (right): http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/engine/a... chrome/browser/sync/engine/all_status.cc:101: if (status_.syncing) { Why not make this a big series of if/else if branches? http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:1040: const char* strings[] = {"INVALID", "READY", "OFFLINE", "OFFLINE_UNSYNCED", This is much more clever than it ought to be. No one will expect that the enum-to-string method has been tweaked to return the same string for two different enum values. It's not very discoverable, either; one would have to carefully examine the the definitions here and in the .h file to notice this.
Hmm.. I thought about this little hard. here are 2 observations: 1. I thought about a new name for the variable. some of the names that I thought about include "waiting_to_sync"(might give an impression that we are not fully loaded yet), "not_synced_since_restart"(this will look like there is an error), "will_sync_in_some_time"(we are not sure of that), "sync_initialized"(Pretty generic). 2. The output on about:sync as would be displayed by this change has zero chance of confusing people(at least non sync developers). There are other errors like unrecoverable error that displays extra text. As you can see even today we got an email from some googler who looked at about:sync page. and a non descriptive summary is bound to confuse him. An extra line on the summary is probably justified. In fact my preference is to add that extra line for OFFLINE_UNUSED, OFFLINE etc. Those names are pretty hard to understand!! 3. Regarding the status reading READY and that confusing sync developers: My bet is it could be confusing the first time around but after that it should be OK. But ideally what we want to display there is only 1 of 2 states. READY or DISCONNECTED. (after all it is the summary). Then display this variable as a line item somewhere else. I think this is probably the right change for M18 and do few more things for M19 and beyond. On 2012/02/08 00:07:37, rlarocque wrote: > Inline comments did not publish on the first try. Let's try again... > > http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/engine/a... > File chrome/browser/sync/engine/all_status.cc (right): > > http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/engine/a... > chrome/browser/sync/engine/all_status.cc:101: if (status_.syncing) { > Why not make this a big series of if/else if branches? > > http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/profile_... > File chrome/browser/sync/profile_sync_service.cc (right): > > http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/profile_... > chrome/browser/sync/profile_sync_service.cc:1040: const char* strings[] = > {"INVALID", "READY", "OFFLINE", "OFFLINE_UNSYNCED", > This is much more clever than it ought to be. No one will expect that the > enum-to-string method has been tweaked to return the same string for two > different enum values. It's not very discoverable, either; one would have to > carefully examine the the definitions here and in the .h file to notice this.
answer to your inline comment. http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/engine/a... File chrome/browser/sync/engine/all_status.cc (right): http://codereview.chromium.org/9323015/diff/8001/chrome/browser/sync/engine/a... chrome/browser/sync/engine/all_status.cc:101: if (status_.syncing) { Nothing functionally wrong with that. The reason is maintainability. If the 'if else' branching was based on the value of 1 or even 2 variables it would not be this bad. This is on few different random variables. What I am hoping to have is some structure in the code so if you are messing with syncing state you modify the first block, for idle state modify the second block and for error cases(the most likely case that will get changed soon) modify the third block. On 2012/02/08 00:07:37, rlarocque wrote: > Why not make this a big series of if/else if branches?
On 2012/02/08 19:40:16, lipalani1 wrote: > Hmm.. I thought about this little hard. here are 2 observations: > 1. I thought about a new name for the variable. some of the names that I thought > about include "waiting_to_sync"(might give an impression that we are not fully > loaded yet), "not_synced_since_restart"(this will look like there is an error), > "will_sync_in_some_time"(we are not sure of that), "sync_initialized"(Pretty > generic). I think "initialized" is on the right track. Maybe "recently_initialized", to indicate that it's a transient state? > 2. The output on about:sync as would be displayed by this change has zero chance > of confusing people(at least non sync developers). There are other errors like > unrecoverable error that displays extra text. As you can see even today we got > an email from some googler who looked at about:sync page. and a non descriptive > summary is bound to confuse him. An extra line on the summary is probably > justified. In fact my preference is to add that extra line for OFFLINE_UNUSED, > OFFLINE etc. Those names are pretty hard to understand!! > 3. Regarding the status reading READY and that confusing sync developers: My bet > is it could be confusing the first time around but after that it should be OK. > But ideally what we want to display there is only 1 of 2 states. READY or > DISCONNECTED. (after all it is the summary). Then display this variable as a > line item somewhere else. I disagree with this. The goal for M18 should be to fix the bug with the smallest change possible. I think adding explanatory text is an extra feature that is not required for this milestone, regardless of its usefulness. Also, taking us way off-topic for a moment: I think there's a larger problem with the status summary. There's no way to tell that the server is reachable unless we maintain a TCP connection and ping it regularly. Making inferences based on the most recent connection attempt and displaying them as if they were known facts is a bad idea. The UI should show the results of the most recent connections. That's all the system really knows, anyway. If the person reading about:sync uses that information to guess about the status of the server, that's fine. At least that person will be well informed as to what evidence is being used to make that guess. I intend to remove the "offline" states from AllStatus. I have a CL out for review that does so. I wouldn't worry too much about adding text to explain them in M19.
After discussing offline, I'm willing to drop the objection to the explanatory text in sync_ui_util. I still think the enum values should be better named, and that BuildSyncSummaryText() should perform a straight conversion from enum names to strings, though. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
