Issue 268643002: Use the DefaultSearchManager as the exclusive authority on DSE, ignoring Web Data.

erikwright (departed)

pkasting: PTAL. I need to (1) integrate Cait's work to support extensions (2) make the ...

6 years, 7 months ago (2014-05-02 02:22:34 UTC) #1

erikwright (departed)

Uploaded a new patchset that fixes almost all tests besides sync. https://codereview.chromium.org/268643002/diff/60001/chrome/browser/search_engines/default_search_manager.h File chrome/browser/search_engines/default_search_manager.h (right): ...

6 years, 7 months ago (2014-05-02 16:35:56 UTC) #2

Peter Kasting

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h File chrome/browser/search/instant_service.h (right): https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h#newcode104 chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher : public TemplateURLServiceObserver { What do we ...

6 years, 7 months ago (2014-05-03 00:28:18 UTC) #3

erikwright (departed)

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h File chrome/browser/search/instant_service.h (right): https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h#newcode104 chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher : public TemplateURLServiceObserver { On 2014/05/03 00:28:18, ...

6 years, 7 months ago (2014-05-03 00:54:08 UTC) #4

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher :
public TemplateURLServiceObserver {
On 2014/05/03 00:28:18, Peter Kasting wrote:
> What do we gain by having this class instead of making InstantService a
> TemplateURLServiceObserver directly?  It seems like this just calls the
> identically-named method on its owning class.  I can't see the benefit.

It maintains encapsulation. OnTemplateURLServiceChanged is not intended to be
part of the public API of InstantService. Clients should not be able to invoke
it directly. Private inheritance would be an alternative approach but it
violates our style guide.

The style guide also notes that "Composition is often more appropriate than
inheritance."

In fact, some of the tests that had to be changed in this CL were tests that
called TemplateURLService::Observe directly. That's one example of a case where
weak encapsulation made this work harder.

I realize this is not the most common practice in Chromium, but I do think it
would be better if it were more widely used. I will go whichever way you prefer.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:907: // so choose.
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Wait, so the user says "delete engine A" and then we don't delete A?  That
seems
> bad.  Can't we handle this state correctly?

Yes and no.

(1) We could reset the user-selected pref. But that would cause a sync event,
which would potentially override the (not-yet-received) sync event from another
Chrome which contained the user's new choice of DSE.
(2) We could maintain some in-memory state about the pending deletion. But this
would not solve the problem if Chrome exits before the Pref sync is received.
(3) We could add some state to the DB record indicating the pending deletion.
(4) We could add some state to preferences indicating the pending deletion (in a
distinct, unsynced preference).

I suppose (1) would be safe if it was clear that a changed record would override
a deleted record in conflict resolution. I can ask around about that.

(2) Would not be perfect, but would probably significantly reduce the impact of
this issue.

(4) seems acceptable.

Thoughts?

Peter Kasting

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h File chrome/browser/search/instant_service.h (right): https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h#newcode104 chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher : public TemplateURLServiceObserver { On 2014/05/03 00:54:08, ...

6 years, 7 months ago (2014-05-03 01:18:16 UTC) #5

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher :
public TemplateURLServiceObserver {
On 2014/05/03 00:54:08, erikwright wrote:
> On 2014/05/03 00:28:18, Peter Kasting wrote:
> > What do we gain by having this class instead of making InstantService a
> > TemplateURLServiceObserver directly?  It seems like this just calls the
> > identically-named method on its owning class.  I can't see the benefit.
> 
> It maintains encapsulation. OnTemplateURLServiceChanged is not intended to be
> part of the public API of InstantService. Clients should not be able to invoke
> it directly.

That's fine, you should be declaring that method private anyway, which prevents
people from calling it unless they're holding a valid pointer to a
TemplateURLServiceObserver (in which case they probably ought to be able to call
this -- but in general no one should do this).  That should allow you to
subclass directly.

(This is a reason I always advise people to declare method overrides "as private
as possible" instead of matching the visibility of the method declaration in the
base class.)

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:907: // so choose.
On 2014/05/03 00:54:08, erikwright wrote:
> On 2014/05/03 00:28:18, Peter Kasting wrote:
> > Wait, so the user says "delete engine A" and then we don't delete A?  That
> seems
> > bad.  Can't we handle this state correctly?
> 
> Yes and no.
> 
> (1) We could reset the user-selected pref. But that would cause a sync event,
> which would potentially override the (not-yet-received) sync event from
another
> Chrome which contained the user's new choice of DSE.

Wait a minute, this code is ACTION_DELETE on the current DSP.  So wouldn't the
right description be "This can happen if the user changes the default in one
client to engine A, then deletes engine A in a separate client"?  I think you
should explicitly ask the sync folks how to handle this.

erikwright (departed)

On 2014/05/03 01:18:16, Peter Kasting wrote: > https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h > File chrome/browser/search/instant_service.h (right): > > https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h#newcode104 ...

6 years, 7 months ago (2014-05-03 01:24:53 UTC) #6

On 2014/05/03 01:18:16, Peter Kasting wrote:
>
https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
> File chrome/browser/search/instant_service.h (right):
> 
>
https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
> chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher
:
> public TemplateURLServiceObserver {
> On 2014/05/03 00:54:08, erikwright wrote:
> > On 2014/05/03 00:28:18, Peter Kasting wrote:
> > > What do we gain by having this class instead of making InstantService a
> > > TemplateURLServiceObserver directly?  It seems like this just calls the
> > > identically-named method on its owning class.  I can't see the benefit.
> > 
> > It maintains encapsulation. OnTemplateURLServiceChanged is not intended to
be
> > part of the public API of InstantService. Clients should not be able to
invoke
> > it directly.
> 
> That's fine, you should be declaring that method private anyway, which
prevents
> people from calling it unless they're holding a valid pointer to a
> TemplateURLServiceObserver (in which case they probably ought to be able to
call
> this -- but in general no one should do this).  That should allow you to
> subclass directly.
> 
> (This is a reason I always advise people to declare method overrides "as
private
> as possible" instead of matching the visibility of the method declaration in
the
> base class.)
> 
>
https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
> File chrome/browser/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service.cc:907: // so choose.
> On 2014/05/03 00:54:08, erikwright wrote:
> > On 2014/05/03 00:28:18, Peter Kasting wrote:
> > > Wait, so the user says "delete engine A" and then we don't delete A?  That
> > seems
> > > bad.  Can't we handle this state correctly?
> > 
> > Yes and no.
> > 
> > (1) We could reset the user-selected pref. But that would cause a sync
event,
> > which would potentially override the (not-yet-received) sync event from
> another
> > Chrome which contained the user's new choice of DSE.
> 
> Wait a minute, this code is ACTION_DELETE on the current DSP.  So wouldn't the
> right description be "This can happen if the user changes the default in one
> client to engine A, then deletes engine A in a separate client"?  I think you
> should explicitly ask the sync folks how to handle this.

Keep in mind that we have a synced keyword and a synced pref. So when the user
changes their DSP and also deletes the value that was the previous DSP, we will
receive two events - one pref change, and one keyword deletion.

This doesn't require acting in two separate clients.

I will query the sync experts.

erikwright (departed)

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h File chrome/browser/search/instant_service.h (right): https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h#newcode104 chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher : public TemplateURLServiceObserver { On 2014/05/03 01:18:17, ...

6 years, 7 months ago (2014-05-03 01:24:59 UTC) #7

Peter Kasting

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h File chrome/browser/search/instant_service.h (right): https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/instant_service.h#newcode104 chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher : public TemplateURLServiceObserver { On 2014/05/03 01:24:59, ...

6 years, 7 months ago (2014-05-03 01:31:01 UTC) #8

erikwright (departed)

I responded to some of the more straightforward comments. I see the following remaining: (1) ...

6 years, 7 months ago (2014-05-05 01:49:37 UTC) #9

I responded to some of the more straightforward comments. I see the following
remaining:

(1) external feedback required for sync and chrome/test/base/...
(2) reconsider the code that is comparing by sync_guid
(3) search_provider_install_data TODO
(4) remaining test fixes

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.h:104: class DefaultSearchEngineWatcher :
public TemplateURLServiceObserver {
On 2014/05/03 01:31:01, Peter Kasting wrote:
> On 2014/05/03 01:24:59, erikwright wrote:
> > It's not as though converting from an InstantService to a
> > TemplateURLServiceObserver requires any hoops.
> 
> It requires an explicit upcast or assignment.  That's enough of a hurdle that
> any reviewer ought to look askance at such a line.
> 
> > It's not production code that is likely to do this, it's tests, where nobody
> is
> > going to bat an eye if the line preceding a call to
> OnTemplateURLServiceChanged
> > is preceded by an assignment using implicit cast.
> 
> Well, as I just said, they ought to bat an eye.  But still, calling this
> function directly _isn't_ actually problematic.  It doesn't do anything bad or
> cause any state inconsistency.  This is an issue of theoretical API purity,
but
> it's _not_ an issue of practical problematic API, which means if
non-production
> code that does something unusual wants to call this, nothing is going to go
> haywire -- and if in the future this does result in state issues, then the
> affected code will be test code.  Which is precisely what you want to have
break
> if people are using things wrong.
> 
> I don't think you should feel like you're doing something suboptimal and lame
> here.  This really does seem fine to me -- certainly safer than e.g.
> FRIEND_TEST(), which we use all over the place.

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.h:106: explicit
DefaultSearchEngineWatcher(InstantService* outer);
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: I suggest explicitly declaring a virtual destructor.  (It's possible the
> style linter would ask you for a destructor on this class anyway.)  Also,
> |outer| seems like perhaps a worse name than |parent| or |owner|.

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_service.h:111:
DISALLOW_COPY_AND_ASSIGN(DefaultSearchEngineWatcher);
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Tiny nit: I'd put a blank line above this.  Also probably between the
> constructor and the virtual function above.

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
File chrome/browser/search/instant_unittest_base.h (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search/i...
chrome/browser/search/instant_unittest_base.h:52: // BrowserWithTestWindowTest
override
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: " override" -> ":"

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_manager.cc (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.cc:122: TemplateURLData*
DefaultSearchManager::GetDefaultSearchEngine(
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Given the number of times you call this just to get the search engine source,
it
> seems like perhaps leaving things as two separate functions would be better. 
> This would make a lot of callers shorter.
> 
> You could even leave GetDefaultSearchEngine() as taking an arg, which would
mean
> the unittest code that really wants this combined form could still use it; 
and
> the implementation here would basically just be the old version of the code
plus
> a trailing call to:
> 
>   if (source)
>     *source = GetDefaultSearchEngineSource();

It makes sense to have a single function calculate both since the algorithm is
identical. Separate functions would end up having (hopefully) duplicate logic.

So I've added a second method that only returns the Source, but delegates to
this one to do so.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.cc:208: Source old_source =
FROM_FALLBACK;
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: I think initting this before the call, when the call is guaranteed to set
> it, is more confusing than helpful.  (I don't _think_ any of our bots ought to
> mandate that you do so.)  (3 places)

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.cc:211: if (old_source <=
FROM_EXTENSION)
On 2014/05/03 00:28:18, Peter Kasting wrote:
> This implies that the Source values are in sorted order, which may happen to
be
> true, but isn't documented or enforced elsewhere.
> 
> It seems like a clearer and safer method would be something like:
> 
>   extension_default_search_.reset(data);
>   Source source;
>   GetDefaultSearchEngine(&source);
>   if (source == FROM_EXTENSION)
>     NotifyObserver();

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.cc:244: }
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Simpler:
> 
>   Source source;
>   GetDefaultSearchEngine(&source);
>   LoadDefaultSearchEngineFromPrefs();
>   if ((source != FROM_USER) && (source != FROM_POLICY))
>     GetDefaultSearchEngine(&source);
>   if ((source == FROM_USER) || (source == FROM_POLICY))
>     NotifyObserver();

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.cc:265: pref_service_,
&default_search_index);
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Indenting is wrong (3 instead of 4) -- I would leave things as they were
> before, I think that's arguably better and certainly not worse.

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.cc:302: disabled_by_policy)
{
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_manager.h (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.h:74: 
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Extra newline

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.h:113: // policy changes.
Calls |LoadDefaultSearchEngineFromPrefs| and
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Don't put pipes around function names; use a trailing "()" instead
(several
> places)

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.h:117: // Handle changes to
kSearchProviderOverrides pref. Calls
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Handle -> Handles

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.h:126: // Read default
search provider data from |pref_service_|, updating
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Read -> Reads (2 places)

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.h:128: // Will invoke
|MergePrefsDataWithPrepopulated|.
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Will invoke -> Invokes (or Calls) (2 places)

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_manager_unittest.cc (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager_unittest.cc:190:
EXPECT_EQ(DefaultSearchManager::Source::FROM_FALLBACK, source);
On 2014/05/03 00:28:18, Peter Kasting wrote:
> As the compile failures note, you need to change all instances of
> DefaultSearchManager::Source::XXX to DefaultSearchManager::XXX.

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/search_provider_install_data.cc:268:
SetDefault(NULL);
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Um, what is the effect of not getting this right today?  "SetDefault(NULL)"
> worries me that you're going to break something important.

I intend to resolve this TODO before committing.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:509: if
(template_url->extension_info_->wants_to_be_default_engine) {
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:528:
WebDataService::KeywordBatchModeScoper keyword_scoper(service_.get());
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: Leave this just above RemoveNoNotify() where it was before

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:637: return 
default_search_provider_source_ <= DefaultSearchManager::FROM_USER &&
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Again, I don't like the way this relies on the enum ordering, use explicit ==
> and != comparisons instead.
> 
> Nit: Extra space; use () around binary subexprs

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:768: ?
initial_default_search_provider_.get()
On 2014/05/03 00:28:18, Peter Kasting wrote:
> This clang-format output is incorrect; format as:
> 
>       (default_search_provider_source_ == DefaultSearchManager::FROM_USER) ?
>           initial_default_search_provider_.get() : NULL,

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:780: // This also calls
NotifyObservers.
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Then should we move up the Notify() call below as well?

Sure. I assume no-one is counting on history to be updated before we say we are
loaded.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1448: : NULL,
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Again, format as
> 
>   OnDefaultSearchChange(
>       initial_default_search_provider_ ?
>           initial_default_search_provider_->data() : NULL,
>       default_search_provider_source_);

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1450:
initial_default_search_provider_.reset();
On 2014/05/03 00:28:18, Peter Kasting wrote:
> You can't reset this here, unless you completely eliminate the idea of
> |load_failed_|.

Oops, I had meant to.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1455: const TemplateURL*
t_url, PrefService* prefs) {
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: One arg per line

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:104: // Saves enough of url
to preferences so that it can be loaded from
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Nit: url -> |url|

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:112: // If the user or the
policy has opted for no default search, this
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Users cannot opt for no default search.  Only policy is supposed to be able to
> do that.

Done.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:607: const
ExtensionKeyword& extension_keyword) const;
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Const functions must not return non-const pointers.  At the very least, don't
> add new functions that do so; if at all possible, please also fix the
signatures
> of these existing functions.

I removed const from each of the method signatures that returned non-const
pointers to the contents of the TemplateURLService. They were never called on a
const instance, so this had no ripple effects.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
File chrome/browser/search_engines/util.cc (right):

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/util.cc:212: (*j)->sync_guid() !=
default_search_provider->sync_guid());
On 2014/05/03 00:28:18, Peter Kasting wrote:
> How does sync have anything to do with this?  It seems like if we can't do a
> pointer comparison any longer, we need to compare the whole TURL.

I will revisit this part of the implementation now.

https://codereview.chromium.org/268643002/diff/100001/chrome/browser/search_e...
chrome/browser/search_engines/util.cc:299: template_url->sync_guid() !=
default_search_provider->sync_guid()))
On 2014/05/03 00:28:18, Peter Kasting wrote:
> Same comment.

Ditto.

erikwright (departed)

pawel: PTAL for browser_with_test_window_test.cc. I think I need this new hook because some keyed services ...

6 years, 7 months ago (2014-05-05 06:01:57 UTC) #10

erikwright (departed)

zea: PTAL at https://codereview.chromium.org/268643002/diff/230001/chrome/browser/search_engines/template_url_service.cc#newcode907 The scenario is: Two Chrome installs (C1, C2) are syncing. The ...

6 years, 7 months ago (2014-05-05 06:09:54 UTC) #11

erikwright (departed)

pkasting: PTAL. (1) I have sought input from pawel@ and zea@ regarding the two aforementioned ...

6 years, 7 months ago (2014-05-05 06:23:22 UTC) #12

Paweł Hajdan Jr.

https://codereview.chromium.org/268643002/diff/240001/chrome/test/base/browser_with_test_window_test.cc File chrome/test/base/browser_with_test_window_test.cc (right): https://codereview.chromium.org/268643002/diff/240001/chrome/test/base/browser_with_test_window_test.cc#newcode219 chrome/test/base/browser_with_test_window_test.cc:219: void BrowserWithTestWindowTest::CustomizeServiceFactories() { Why not just override CreateProfile? https://codereview.chromium.org/268643002/diff/240001/chrome/test/base/browser_with_test_window_test.h ...

6 years, 7 months ago (2014-05-05 11:28:08 UTC) #13

Nicolas Zea

On 2014/05/05 06:09:54, erikwright wrote: > zea: PTAL at > https://codereview.chromium.org/268643002/diff/230001/chrome/browser/search_engines/template_url_service.cc#newcode907 > > The scenario ...

6 years, 7 months ago (2014-05-05 20:09:37 UTC) #14

erikwright (departed)

On Mon, May 5, 2014 at 4:09 PM, <zea@chromium.org> wrote: > On 2014/05/05 06:09:54, erikwright ...

6 years, 7 months ago (2014-05-05 20:22:29 UTC) #15

On Mon, May 5, 2014 at 4:09 PM, <zea@chromium.org> wrote:

> On 2014/05/05 06:09:54, erikwright wrote:
>
>> zea: PTAL at
>>
>
> https://codereview.chromium.org/268643002/diff/230001/
> chrome/browser/search_engines/template_url_service.cc#newcode907
>
>  The scenario is:
>>
>
>  Two Chrome installs (C1, C2) are syncing.
>> The DSE is X. X's properties are stored in prefs, and those properties
>> from
>> prefs are what we rely on as the main source of truth when Chrome starts
>> up.
>> In C1, the user changes their DSE to Y.
>> In C1, the user deletes X.
>> In C2, we receive the delete X event.
>>
>
>  At this point, we have a problem. We are told to delete X, but X is the
>> DSE
>> according to prefs. We could remove X from the Web Data DB, but it will be
>> re-inserted in a subsequent startup due to its presence in Prefs.
>>
>
>  If, at this point, in C2, we clear the value in prefs, what will happen
>> later?
>>
>
>  Will the clear be squashed by the sync of Set(Y)? Will the clear get
>> synced up
>> and end up squashing the Y value in C1?
>>
>
>  Is it impossible to receive the events in this order on C2 if they are
>> sent in
>> the correct order by C1?
>>
>
> If you clear the prefs value, doesn't that mean you locally lose your DSE
> info
> on C2? That seems bad on its own. That said, assuming you didn't sync that
> clear, then yes, once the new DSE arrives, it should overwrite the local
> DSE and
> undo the clear. But there's no guarantee as to when that will happen.
>

This scenario would arise because the previously set DSE is no longer
valid. We assume there is a new DSE coming down the sync pipeline at some
point, but we don't have it yet.

So in the meantime we can either keep the old value or we can wipe it
(which will result in automatically using the prepopulated default engine).

Since the DSE value is now stored in a syncable pref, I don't really have
any control over whether a "clear" syncs. I assume if I ClearPref() the
pref it will sync. If there is an unreceived sync event to set the pref to
some value, I don't know which one will win. Do you?

And yes, theoretically this ordering of events could happen. Is there harm
> in
> leaving the old logic? (undoing the web data delete)
>

The old logic is obliterated by this CL. As the CL stands now, the result
is that the user deleted a search engine on another Chrome and then when we
receive the sync event we just decide not to respect that.

https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On 2014/05/05 20:09:37, Nicolas Zea wrote: > On 2014/05/05 06:09:54, erikwright wrote: > > In ...

6 years, 7 months ago (2014-05-05 20:36:39 UTC) #16

Peter Kasting

On 2014/05/05 20:36:39, Peter Kasting wrote: > On 2014/05/05 20:09:37, Nicolas Zea wrote: > > ...

6 years, 7 months ago (2014-05-05 20:39:13 UTC) #17

erikwright (departed)

brettw: PTAL for chrome/browser/search/. TL;DR: Instead of TemplateURLService updating Prefs whenever the DSE changes, prefs ...

6 years, 7 months ago (2014-05-05 20:50:31 UTC) #18

Nicolas Zea

On 2014/05/05 20:22:29, erikwright wrote: > On Mon, May 5, 2014 at 4:09 PM, <mailto:zea@chromium.org> ...

6 years, 7 months ago (2014-05-05 20:59:35 UTC) #19

On 2014/05/05 20:22:29, erikwright wrote:
> On Mon, May 5, 2014 at 4:09 PM, <mailto:zea@chromium.org> wrote:
> 
> > On 2014/05/05 06:09:54, erikwright wrote:
> >
> >> zea: PTAL at
> >>
> >
> > https://codereview.chromium.org/268643002/diff/230001/
> > chrome/browser/search_engines/template_url_service.cc#newcode907
> >
> >  The scenario is:
> >>
> >
> >  Two Chrome installs (C1, C2) are syncing.
> >> The DSE is X. X's properties are stored in prefs, and those properties
> >> from
> >> prefs are what we rely on as the main source of truth when Chrome starts
> >> up.
> >> In C1, the user changes their DSE to Y.
> >> In C1, the user deletes X.
> >> In C2, we receive the delete X event.
> >>
> >
> >  At this point, we have a problem. We are told to delete X, but X is the
> >> DSE
> >> according to prefs. We could remove X from the Web Data DB, but it will be
> >> re-inserted in a subsequent startup due to its presence in Prefs.
> >>
> >
> >  If, at this point, in C2, we clear the value in prefs, what will happen
> >> later?
> >>
> >
> >  Will the clear be squashed by the sync of Set(Y)? Will the clear get
> >> synced up
> >> and end up squashing the Y value in C1?
> >>
> >
> >  Is it impossible to receive the events in this order on C2 if they are
> >> sent in
> >> the correct order by C1?
> >>
> >
> > If you clear the prefs value, doesn't that mean you locally lose your DSE
> > info
> > on C2? That seems bad on its own. That said, assuming you didn't sync that
> > clear, then yes, once the new DSE arrives, it should overwrite the local
> > DSE and
> > undo the clear. But there's no guarantee as to when that will happen.
> >
> 
> This scenario would arise because the previously set DSE is no longer
> valid. We assume there is a new DSE coming down the sync pipeline at some
> point, but we don't have it yet.
> 
> So in the meantime we can either keep the old value or we can wipe it
> (which will result in automatically using the prepopulated default engine).

Right, my point was that keeping the old value (until the new default arrives)
seems more correct to me.

> 
> Since the DSE value is now stored in a syncable pref, I don't really have
> any control over whether a "clear" syncs. I assume if I ClearPref() the
> pref it will sync. If there is an unreceived sync event to set the pref to
> some value, I don't know which one will win. Do you?

In general, this kind of action is racy, and therefore it's effectively
undefined. Either one could win, which is why I don't think this is the right
approach.

> 
> And yes, theoretically this ordering of events could happen. Is there harm
> > in
> > leaving the old logic? (undoing the web data delete)
> >
> 
> The old logic is obliterated by this CL. As the CL stands now, the result
> is that the user deleted a search engine on another Chrome and then when we
> receive the sync event we just decide not to respect that.

The problem with just ignoring the delete is that if for some reason the local
value changes, you'll try to UPDATE it in sync, when sync doesn't have an
existing entity for it (triggering a sync error). If you want to ignore the
delete, you should do what was being done previously, which is to tell sync to
undelete the item.

> 
> https://codereview.chromium.org/268643002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Nicolas Zea

On 2014/05/05 20:39:13, Peter Kasting wrote: > On 2014/05/05 20:36:39, Peter Kasting wrote: > > ...

6 years, 7 months ago (2014-05-05 20:59:59 UTC) #20

erikwright (departed)

Thanks for the clarifications, Nicolas. Undeleting it is an option, but for users who actually ...

6 years, 7 months ago (2014-05-05 21:23:51 UTC) #21

Thanks for the clarifications, Nicolas.

Undeleting it is an option, but for users who actually do this operation
(change the DSE, delete the old SE) it seems like it's basically 50/50
whether it succeeds, which is annoying.

I don't think syncing over a single data-type is an option.

It's possible to maintain some side state about the fact that this
particular search engine is pending deletion, but that seems painful and
risky.

The proposal I have is therefore the following:

In the current patchset, if we find a DSE from prefs that is not in the web
database we create it.

In the proposed modification, we don't do that if the DSE has a sync guid.

Instead, we will keep the DSE in memory only, and not write it to Web Data.
Later, when/if we receive the sync event that creates the DSE, we will
persist it to Web Data.

Inversely, if we receive a delete for the current DSE, we will delete it
from Web Data but keep it in memory as the active DSE. Upon reboot, we will
read its properties from prefs but not (as described in the previous CL)
attempt to push it into Web DB.

On Mon, May 5, 2014 at 5:00 PM, <zea@chromium.org> wrote:

> On 2014/05/05 20:39:13, Peter Kasting wrote:
>
>> On 2014/05/05 20:36:39, Peter Kasting wrote:
>> > On 2014/05/05 20:09:37, Nicolas Zea wrote:
>> > > On 2014/05/05 06:09:54, erikwright wrote:
>> > > > In C1, the user changes their DSE to Y.
>> > > > In C1, the user deletes X.
>> > > > In C2, we receive the delete X event.
>> > > >
>> > > > Is it impossible to receive the events in this order on C2 if they
>> are
>> sent
>> > in
>> > > > the correct order by C1?
>> > >
>> > > yes, theoretically this ordering of events could happen. Is there
>> harm in
>> > > leaving the old logic? (undoing the web data delete)
>> >
>> > I thought sync was supposed to guarantee that if in C1 the user does
>> (change
>> > DSE, delete engine), then in C2, we will receive and process the events
>> from
>> > sync in the order (change DSE, delete engine).
>>
>
>  Oh wait, you're making these events come from two separate sync datatypes
>> --
>>
> the
>
>> DSE change will be a pref change, and the engine deletion will be a search
>> engines change.  Yeah, that ordering is not guaranteed.  (I think the last
>>
> time
>
>> this came up in discussion a few years ago we basically concluded that
>> this
>>
> was
>
>> a good reason why all changes that affected one's search engines should be
>> scoped to the same datatype.)
>>
>
> Correctly on both counts.
>
> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Nicolas Zea

For the most part this SGTM. To be clear, what will happen in the UI? ...

6 years, 7 months ago (2014-05-05 21:34:30 UTC) #22

For the most part this SGTM. To be clear, what will happen in the UI?
Presumably, the current DSE will be populated, and the user could choose to
locally change the default to something else? Will the old default be deleted at
that point?

On 2014/05/05 21:23:51, erikwright wrote:
> Thanks for the clarifications, Nicolas.
> 
> Undeleting it is an option, but for users who actually do this operation
> (change the DSE, delete the old SE) it seems like it's basically 50/50
> whether it succeeds, which is annoying.
> 
> I don't think syncing over a single data-type is an option.
> 
> It's possible to maintain some side state about the fact that this
> particular search engine is pending deletion, but that seems painful and
> risky.
> 
> The proposal I have is therefore the following:
> 
> In the current patchset, if we find a DSE from prefs that is not in the web
> database we create it.
> 
> In the proposed modification, we don't do that if the DSE has a sync guid.
> 
> Instead, we will keep the DSE in memory only, and not write it to Web Data.
> Later, when/if we receive the sync event that creates the DSE, we will
> persist it to Web Data.
> 
> Inversely, if we receive a delete for the current DSE, we will delete it
> from Web Data but keep it in memory as the active DSE. Upon reboot, we will
> read its properties from prefs but not (as described in the previous CL)
> attempt to push it into Web DB.
> 
> 
> On Mon, May 5, 2014 at 5:00 PM, <mailto:zea@chromium.org> wrote:
> 
> > On 2014/05/05 20:39:13, Peter Kasting wrote:
> >
> >> On 2014/05/05 20:36:39, Peter Kasting wrote:
> >> > On 2014/05/05 20:09:37, Nicolas Zea wrote:
> >> > > On 2014/05/05 06:09:54, erikwright wrote:
> >> > > > In C1, the user changes their DSE to Y.
> >> > > > In C1, the user deletes X.
> >> > > > In C2, we receive the delete X event.
> >> > > >
> >> > > > Is it impossible to receive the events in this order on C2 if they
> >> are
> >> sent
> >> > in
> >> > > > the correct order by C1?
> >> > >
> >> > > yes, theoretically this ordering of events could happen. Is there
> >> harm in
> >> > > leaving the old logic? (undoing the web data delete)
> >> >
> >> > I thought sync was supposed to guarantee that if in C1 the user does
> >> (change
> >> > DSE, delete engine), then in C2, we will receive and process the events
> >> from
> >> > sync in the order (change DSE, delete engine).
> >>
> >
> >  Oh wait, you're making these events come from two separate sync datatypes
> >> --
> >>
> > the
> >
> >> DSE change will be a pref change, and the engine deletion will be a search
> >> engines change.  Yeah, that ordering is not guaranteed.  (I think the last
> >>
> > time
> >
> >> this came up in discussion a few years ago we basically concluded that
> >> this
> >>
> > was
> >
> >> a good reason why all changes that affected one's search engines should be
> >> scoped to the same datatype.)
> >>
> >
> > Correctly on both counts.
> >
> > https://codereview.chromium.org/268643002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

erikwright (departed)

Yes - in the UI changing the DSE to something else would cause this "ghost" ...

6 years, 7 months ago (2014-05-05 21:36:45 UTC) #23

Yes - in the UI changing the DSE to something else would cause this "ghost"
SE to disappear.


On Mon, May 5, 2014 at 5:34 PM, <zea@chromium.org> wrote:

> For the most part this SGTM. To be clear, what will happen in the UI?
> Presumably, the current DSE will be populated, and the user could choose to
> locally change the default to something else? Will the old default be
> deleted at
> that point?
>
>
> On 2014/05/05 21:23:51, erikwright wrote:
>
>> Thanks for the clarifications, Nicolas.
>>
>
>  Undeleting it is an option, but for users who actually do this operation
>> (change the DSE, delete the old SE) it seems like it's basically 50/50
>> whether it succeeds, which is annoying.
>>
>
>  I don't think syncing over a single data-type is an option.
>>
>
>  It's possible to maintain some side state about the fact that this
>> particular search engine is pending deletion, but that seems painful and
>> risky.
>>
>
>  The proposal I have is therefore the following:
>>
>
>  In the current patchset, if we find a DSE from prefs that is not in the
>> web
>> database we create it.
>>
>
>  In the proposed modification, we don't do that if the DSE has a sync guid.
>>
>
>  Instead, we will keep the DSE in memory only, and not write it to Web
>> Data.
>> Later, when/if we receive the sync event that creates the DSE, we will
>> persist it to Web Data.
>>
>
>  Inversely, if we receive a delete for the current DSE, we will delete it
>> from Web Data but keep it in memory as the active DSE. Upon reboot, we
>> will
>> read its properties from prefs but not (as described in the previous CL)
>> attempt to push it into Web DB.
>>
>
>
>  On Mon, May 5, 2014 at 5:00 PM, <mailto:zea@chromium.org> wrote:
>>
>
>  > On 2014/05/05 20:39:13, Peter Kasting wrote:
>> >
>> >> On 2014/05/05 20:36:39, Peter Kasting wrote:
>> >> > On 2014/05/05 20:09:37, Nicolas Zea wrote:
>> >> > > On 2014/05/05 06:09:54, erikwright wrote:
>> >> > > > In C1, the user changes their DSE to Y.
>> >> > > > In C1, the user deletes X.
>> >> > > > In C2, we receive the delete X event.
>> >> > > >
>> >> > > > Is it impossible to receive the events in this order on C2 if
>> they
>> >> are
>> >> sent
>> >> > in
>> >> > > > the correct order by C1?
>> >> > >
>> >> > > yes, theoretically this ordering of events could happen. Is there
>> >> harm in
>> >> > > leaving the old logic? (undoing the web data delete)
>> >> >
>> >> > I thought sync was supposed to guarantee that if in C1 the user does
>> >> (change
>> >> > DSE, delete engine), then in C2, we will receive and process the
>> events
>> >> from
>> >> > sync in the order (change DSE, delete engine).
>> >>
>> >
>> >  Oh wait, you're making these events come from two separate sync
>> datatypes
>> >> --
>> >>
>> > the
>> >
>> >> DSE change will be a pref change, and the engine deletion will be a
>> search
>> >> engines change.  Yeah, that ordering is not guaranteed.  (I think the
>> last
>> >>
>> > time
>> >
>> >> this came up in discussion a few years ago we basically concluded that
>> >> this
>> >>
>> > was
>> >
>> >> a good reason why all changes that affected one's search engines
>> should be
>> >> scoped to the same datatype.)
>> >>
>> >
>> > Correctly on both counts.
>> >
>> > https://codereview.chromium.org/268643002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On 2014/05/05 21:36:45, erikwright wrote: > Yes - in the UI changing the DSE to ...

6 years, 7 months ago (2014-05-05 22:04:32 UTC) #24

erikwright (departed)

I believe there is already a facility for making a SE entry not editable. So ...

6 years, 7 months ago (2014-05-05 22:10:39 UTC) #25

Nicolas Zea

On 2014/05/05 22:10:39, erikwright wrote: > I believe there is already a facility for making ...

6 years, 7 months ago (2014-05-05 22:15:36 UTC) #26

Peter Kasting

On 2014/05/05 22:10:39, erikwright wrote: > I believe there is already a facility for making ...

6 years, 7 months ago (2014-05-05 22:19:28 UTC) #27

erikwright (departed)

On Mon, May 5, 2014 at 6:19 PM, <pkasting@chromium.org> wrote: > On 2014/05/05 22:10:39, erikwright ...

6 years, 7 months ago (2014-05-05 22:22:54 UTC) #28

Peter Kasting

On 2014/05/05 22:22:54, erikwright wrote: > On Mon, May 5, 2014 at 6:19 PM, <mailto:pkasting@chromium.org> ...

6 years, 7 months ago (2014-05-05 22:28:04 UTC) #29

erikwright (departed)

Nicolas called it an "undelete" and implied that the prior code did handle this in ...

6 years, 7 months ago (2014-05-05 22:43:13 UTC) #30

Peter Kasting

On 2014/05/05 22:43:13, erikwright wrote: > Nicolas called it an "undelete" and implied that the ...

6 years, 7 months ago (2014-05-05 22:46:44 UTC) #31

Nicolas Zea

As long as you're using the same sync_tag when you create/update the data, then Sync ...

6 years, 7 months ago (2014-05-05 22:47:09 UTC) #32

As long as you're using the same sync_tag when you create/update the data, then
Sync will ensure all other clients receive this as a change to the existing
tombstone (or an update if they hadn't received the tombstone). There won't be a
duplicate, as sync_tags are unique per datatype.

On 2014/05/05 22:43:13, erikwright wrote:
> Nicolas called it an "undelete" and implied that the prior code did handle
> this in some cases. I had assumed it would become obvious but if he wants
> to chime in here that would be most appreciated.
> 
> 
> On Mon, May 5, 2014 at 6:28 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > On 2014/05/05 22:22:54, erikwright wrote:
> >
> >  On Mon, May 5, 2014 at 6:19 PM, <mailto:pkasting@chromium.org> wrote:
> >> > Wouldn't that result in a duplicated search engine (the new one we
> >> create
> >> > locally, and the new remote entry that will come down later)?  And if
> >> the
> >> > user
> >> > edited something other than the keyword, we'd need to prevent keyword
> >> > conflicts
> >> > between the two, and since we wouldn't have access to the "old" entry
> >> yet,
> >> > we'd
> >> > have to change the "new" entry.  Which isn't good.
> >> >
> >>
> >
> >  We have a sync guid already. All the events, conflicts, etc., will have
> >> that in common. So I believe the existing sync machinery should handle
> >> this
> >> OK.
> >>
> >
> > Handle what?  You have to say precisely what you're going to do before it's
> > clear how the server will "handle" this.
> >
> > When the user edits the URL, do you intend to send sync a "change"
> > notification?
> >  The server thinks this client doesn't have this entity yet.  Won't that
> > cause
> > problems?  Or do you intend to send an "add" notification, in which case
> > we'll
> > be adding an entity that's already on the server with the same ID?  Either
> > one
> > of these sounds problematic.
> >
> > https://codereview.chromium.org/268643002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Nicolas Zea

On 2014/05/05 22:46:44, Peter Kasting wrote: > On 2014/05/05 22:43:13, erikwright wrote: > > Nicolas ...

6 years, 7 months ago (2014-05-05 22:51:01 UTC) #33

erikwright (departed)

zea: your understanding matches mine. On Mon, May 5, 2014 at 6:51 PM, <zea@chromium.org> wrote: ...

6 years, 7 months ago (2014-05-05 22:52:59 UTC) #34

Peter Kasting

On 2014/05/05 22:51:01, Nicolas Zea wrote: > I think I may be confused about the ...

6 years, 7 months ago (2014-05-05 22:55:29 UTC) #35

erikwright (departed)

pkasting: my very limited understanding is that we would send an Add event in this ...

6 years, 7 months ago (2014-05-05 22:59:12 UTC) #36

pkasting: my very limited understanding is that we would send an Add event
in this case, and that magically sync will pick a winner and synthesize the
appropriate messages to the various clients so that we will be eventually
consistent across the different devices.

zea: ??


On Mon, May 5, 2014 at 6:55 PM, <pkasting@chromium.org> wrote:

> On 2014/05/05 22:51:01, Nicolas Zea wrote:
>
>> I think I may be confused about the circumstances here. Are we not
>> discussing
>> the case where the synced entry existed locally and was the default, but
>> then
>> another client deleted it (presumably because it wasn't the default on
>> that
>> client). We receive the deletion locally, and delete it from the web db,
>> but
>> keep the data around in memory and in the preference (including the sync
>>
> guid).
>
>> If the user then modifies the entry, we'll "add" it back to sync, using
>> the
>> pre-existing sync guid, which should therefore prevent duplication.
>>
>
>  Or am I not on the right page?
>>
>
> I'm concerned about a totally different scenario.
>
> Client A:
> * User creates new DSE.  This changes the prefs and sends an "add" to
> search
> engine sync.
>
> Client B:
> * Receives the prefs update to an entity which has a sync_guid we're not
> familiar with.  We create the entry in memory and don't store it in the web
> database.
> * User edits this new engine.  This changes the prefs and ___________.
>
> The issue is whether in the blank we should send the sync server an "add"
> --
> which I assume will cause problems because that entity already exists on
> the
> server -- or a "change" -- which may also cause problems because the server
> knows it hasn't sent client B the entity to begin with yet.
>
> I think we can only make this work right if:
> * We can send the server a "change" and it won't barf,
> * This also cancels any series of add or change events the server was
> going to
> send down to this client,
> * And at the same time we send the change we also go ahead and write this
> to the
> WebDataService.
>
> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Nicolas Zea

On 2014/05/05 22:55:29, Peter Kasting wrote: > On 2014/05/05 22:51:01, Nicolas Zea wrote: > > ...

6 years, 7 months ago (2014-05-05 23:00:52 UTC) #37

On 2014/05/05 22:55:29, Peter Kasting wrote:
> On 2014/05/05 22:51:01, Nicolas Zea wrote:
> > I think I may be confused about the circumstances here. Are we not
discussing
> > the case where the synced entry existed locally and was the default, but
then
> > another client deleted it (presumably because it wasn't the default on that
> > client). We receive the deletion locally, and delete it from the web db, but
> > keep the data around in memory and in the preference (including the sync
> guid).
> > If the user then modifies the entry, we'll "add" it back to sync, using the
> > pre-existing sync guid, which should therefore prevent duplication.
> > 
> > Or am I not on the right page?
> 
> I'm concerned about a totally different scenario.
> 
> Client A:
> * User creates new DSE.  This changes the prefs and sends an "add" to search
> engine sync.
> 
> Client B:
> * Receives the prefs update to an entity which has a sync_guid we're not
> familiar with.  We create the entry in memory and don't store it in the web
> database.
> * User edits this new engine.  This changes the prefs and ___________.
> 
> The issue is whether in the blank we should send the sync server an "add" --
> which I assume will cause problems because that entity already exists on the
> server -- or a "change" -- which may also cause problems because the server
> knows it hasn't sent client B the entity to begin with yet.
> 
> I think we can only make this work right if:
> * We can send the server a "change" and it won't barf,
> * This also cancels any series of add or change events the server was going to
> send down to this client,
> * And at the same time we send the change we also go ahead and write this to
the
> WebDataService.

I see. The server doesn't differentiate between add/update, it just receives
snapshots of data and/or tombstones.

The add/update distinction lies entirely at the clients, and is only something
we care about insofar as the datatype should only add something that doesn't
exist locally in the sync db, and update/delete something that does. So in this
case, it's perfectly valid for two clients to attempt to "add" the same item.
It's undefined which version will win, but one will.

Peter Kasting

On 2014/05/05 22:59:12, erikwright wrote: > pkasting: my very limited understanding is that we would ...

6 years, 7 months ago (2014-05-05 23:01:14 UTC) #38

Peter Kasting

On 2014/05/05 23:00:52, Nicolas Zea wrote: > It's perfectly valid for two clients to attempt ...

6 years, 7 months ago (2014-05-05 23:06:31 UTC) #39

engedy

LGTM on c/b/profile_resetter, I only have the one request below. https://codereview.chromium.org/268643002/diff/390001/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/268643002/diff/390001/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h#newcode72 ...

6 years, 7 months ago (2014-05-05 23:06:38 UTC) #40

Nicolas Zea

On 2014/05/05 23:06:31, Peter Kasting wrote: > On 2014/05/05 23:00:52, Nicolas Zea wrote: > > ...

6 years, 7 months ago (2014-05-05 23:19:22 UTC) #41

erikwright (departed)

Prefs takes precedence over what comes from Web Data. Once it's in memory, updates to ...

6 years, 7 months ago (2014-05-05 23:23:57 UTC) #42

Prefs takes precedence over what comes from Web Data. Once it's in memory,
updates to the DSE in TemplateURLService (whether from sync or from local
editing) will be sent to Prefs if the current DSE is a user-specified DSE
(originally from a local setting or from sync).


On Mon, May 5, 2014 at 7:19 PM, <zea@chromium.org> wrote:

> On 2014/05/05 23:06:31, Peter Kasting wrote:
>
>> On 2014/05/05 23:00:52, Nicolas Zea wrote:
>> > It's perfectly valid for two clients to attempt to "add" the same item.
>> > It's undefined which version will win, but one will.
>>
>
>  OK, that's better than I feared, but does the server ultimately send one
>> or
>>
> both
>
>> clients updates to ensure they both see the same version of the engine?
>>  Even
>>
> if
>
>> the answer is "yes", what if the version that wins is the older version,
>> so
>>
> it's
>
>> out-of-sync with the data stored in prefs?
>>
>
>  It just seems like all of this is hugely hairy.  We have to be really,
>> really
>> careful about how we handle updates and when we copy data from sync to
>> prefs
>>
> and
>
>> vice versa.  I can't even enumerate all the possible different flows.
>>
>
> Both clients are guaranteed to eventually see the same version of the
> data. But
> you're right, in various scenarios either version could win . In most
> cases, the
> fact that Client B hasn't received the webdb data implies it never made it
> to
> the server (for a variety of reasons). In that case, the "new" data would
> win,
> and if for some reason Client A comes back online, it will get the new
> version
> before attempting to commit, and clobber the old version locally, throwing
> it
> away. This is just the common case though; there are corner cases where
> it's
> possible the old version would win in the current sync system.
>
> That said, Erik, correct me if I'm wrong, but I was under the impression
> part of
> the plan was to have DSE data have priority over the web data? Doesn't
> this case
> become moot then? Even if the old Client A web data wins, the DSE data
> would
> still be consistent, and is what the user would see?
>
> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On 2014/05/05 23:19:22, Nicolas Zea wrote: > That said, Erik, correct me if I'm wrong, ...

6 years, 7 months ago (2014-05-05 23:25:11 UTC) #43

Nicolas Zea

On 2014/05/05 23:25:11, Peter Kasting wrote: > The DSE prefs take priority over the web ...

6 years, 7 months ago (2014-05-06 00:15:34 UTC) #44

erikwright (departed)

The "ghost entry" approach is quite complicated. The existing implementation already has the issue of ...

6 years, 7 months ago (2014-05-06 00:38:01 UTC) #45

The "ghost entry" approach is quite complicated.

The existing implementation already has the issue of ignoring
deletes<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/search_engines/template_url_service.cc&q=ideal%20file:template_url_service.cc&sq=package:chromium&l=1012>.
I think Zea understood this, and maybe Peter too, but I didn't realize that.

We propose the following solution:

(1) If it's the DSE, do not send out SE sync events.
(2) If it's the DSE, send out Pref sync events.
(3) If it's the DSE and we _receive_ an SE sync update event, drop it
(4) If it's the DSE and we _receive_ an SE sync delete event, send an
undelete event

Zea is right that pushing back on sync updates is dangerous, although we
had been thinking about it here too.

We did also consider the idea of sending out our "current state" of the SE
after it stops being the DSE. This could lead to an old machine receiving a
DSE change overriding that change depending on whether it receives the Pref
event or the SE event first.

On Mon, May 5, 2014 at 8:15 PM, <zea@chromium.org> wrote:

> On 2014/05/05 23:25:11, Peter Kasting wrote:
>
>> The DSE prefs take priority over the web database.  But we have to try and
>>
> keep
>
>> the two in sync, because if the user changes to a different DSE, and the
>> outdated data is in the web database, then the now-not-default engine in
>> the
>> dialog will reflect the old data.
>>
>
>  Which means maybe if the client gets a sync update to the same engine
>> that's
>> currently default, and the data from sync doesn't match, the client should
>>
> throw
>
>> that data away and send the server back an update with the contents of the
>> prefs?  This way only synced pref changes, and not synced search engine
>> data,
>> can change the contents of a user's local DSE.
>>
>
> Hmm, that makes me nervous. If for some reason one of the clients stops
> receiving preference updates, this will basically result in a ping pong
> effect,
> where the two clients both think they have the most recent DSE data, and
> are
> constantly rejecting the search engine changes from the other.
>
> That said, if we only perform the update to the search engine based on a
> user
> event, rather than automatically when a sync change arrives, then that
> might be
> acceptable. One option would be that when the DSE changes from A to B
> (assuming
> A was the one that had DSE data that didn't match the web data), we push
> A's
> data into web db/search engine sync. We're basically saying that the
> previous
> default data is the "correct" data, while at the same time protecting
> against
> misbehaving clients.
>
> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On 2014/05/06 00:38:01, erikwright wrote: > We propose the following solution: > > (1) If ...

6 years, 7 months ago (2014-05-06 00:45:51 UTC) #46

Peter Kasting

(Since the sync stuff here still seems pretty up in the air, and it's the ...

6 years, 7 months ago (2014-05-06 02:05:11 UTC) #47

erikwright (departed)

On Mon, May 5, 2014 at 8:45 PM, <pkasting@chromium.org> wrote: > On 2014/05/06 00:38:01, erikwright ...

6 years, 7 months ago (2014-05-06 13:33:50 UTC) #48

On Mon, May 5, 2014 at 8:45 PM, <pkasting@chromium.org> wrote:

> On 2014/05/06 00:38:01, erikwright wrote:
>
>> We propose the following solution:
>>
>
>  (1) If it's the DSE, do not send out SE sync events.
>> (2) If it's the DSE, send out Pref sync events.
>> (3) If it's the DSE and we _receive_ an SE sync update event, drop it
>> (4) If it's the DSE and we _receive_ an SE sync delete event, send an
>> undelete event
>>
>
> It seems like if a user changes the DSE in client A from engine 1 to
> engine 2,
> and then deletes 1, and client B receives the search engine sync event
> (for the
> delete) before the pref event (for the DSE change), client B will undelete
> engine 1, resulting in client A then undeleting engine 1.  That's not
> right.
>

That is the current implementation. See the code comment
here<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/search_engines/template_url_service.cc&q=ideal%20file:template_url_service.cc&sq=package:chromium&l=1012>
.

Note that we could choose to not send the "undelete" event and just leave
it in an inconsistent state (engine 1 present in client B but not in client
A). But it seems that consistency is better - at least the user can now fix
this problem by re-deleting engine 1 from whichever client is most
convenient.

Why don't we just mark the DSE prefs as unsynced?  At that point everything
> goes
> back to the way it was before: all the sync data happens under one
> datatype.
> When we get sync events, we reflect them into the prefs like you were
> originally
> planning to do.  (We should also stop syncing the "DSE ID", I didn't look
> to see
> if your change is already doing that.)
>

I assume you are suggesting continuing to sync the GUID for indicating the
DSE. That's the current implementation and suffers from the same undelete
issue described above. There are also other issues associated with
protecting the value separate from syncing it. For example, in the case of
a crash after applying a sync event to the DSE in Web Data but before
updating the value in prefs, we are compelled on the next startup to treat
the Prefs value as authoritative. At that point we would push the stale
value into Web Data and then out to the other clients.

> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

erikwright (departed)

-brettw -phajdan.jr Removed the files that you were here for!

6 years, 7 months ago (2014-05-06 16:06:21 UTC) #49

erikwright (departed)

pkasting, zea: PTAL. This is the implementation of my most recent proposal. I need to ...

6 years, 7 months ago (2014-05-06 16:27:21 UTC) #50

Peter Kasting

zea: Within a single sync datatype, is ordering guaranteed? So, if a client, say, edits ...

6 years, 7 months ago (2014-05-06 18:58:09 UTC) #51

zea: Within a single sync datatype, is ordering guaranteed?  So, if a client,
say, edits engine 1, and then edits engine 2, and then deletes engine 3, will
all other clients see those search engine sync updates in that order?

I ask because Erik and I were discussing over IM how we could make all this work
with syncing only the search engines datatype, and avoiding any kind of synced
prefs.  If ordering within a datatype is guaranteed, I think we're good:

(1) Mark the prefs we're using to store the DSE details as unsyncable.
(2) Remove the "default search provider GUID" synced pref entirely.
(3) Add to search engine sync entries a "make this engine the default" boolean.

When a user edits the default engine or changes the default to another engine,
that client should send up a sync message with the newly-default engine's
details, and the "make default" bit set.  Clients who receive search engine sync
data should first write that data into the DSE prefs if the "make default" bit
is set, then write the data to the web DB.

I think this avoids the most common weird ordering scenarios.  For example, if
client A changes the default from engine 1 to engine 2 and then deletes engine
1, other clients are guaranteed to get the "make 2 default" message before the
attempt to modify 1, so they won't be told to delete the current default engine,
so they won't need to undelete it or have a ghost entry.

It's still theoretically possible to get weird orderings.  For example, let's
say the following happen simultaneously:
* Client A changes the default from 1 to 2
* Client B deletes 2
Now when A gets B's message, it probably has to send back an undelete for 2.

I don't think it's possible to ever eliminate all cases like these.  But I think
this system of not syncing any search-related prefs at least banishes most of
them to the realm of the theoretical, and makes them very rare in practice.  And
I think the code is probably simpler than the previous proposal here.  Also, if
some error happens and we try to disable either pref sync or search engine sync
on some client, we don't wind up in a weird scenario where we're syncing half
the data related to default search engines but not the other half -- if we turn
of search engine sync on some client, it's all the way off.

WDYT?

erikwright (departed)

On Tue, May 6, 2014 at 2:58 PM, <pkasting@chromium.org> wrote: > zea: Within a single ...

6 years, 7 months ago (2014-05-06 19:15:57 UTC) #52

On Tue, May 6, 2014 at 2:58 PM, <pkasting@chromium.org> wrote:

> zea: Within a single sync datatype, is ordering guaranteed?  So, if a
> client,
> say, edits engine 1, and then edits engine 2, and then deletes engine 3,
> will
> all other clients see those search engine sync updates in that order?
>
> I ask because Erik and I were discussing over IM how we could make all
> this work
> with syncing only the search engines datatype, and avoiding any kind of
> synced
> prefs.  If ordering within a datatype is guaranteed, I think we're good:
>
> (1) Mark the prefs we're using to store the DSE details as unsyncable.
> (2) Remove the "default search provider GUID" synced pref entirely.
> (3) Add to search engine sync entries a "make this engine the default"
> boolean.
>
> When a user edits the default engine or changes the default to another
> engine,
> that client should send up a sync message with the newly-default engine's
> details, and the "make default" bit set.  Clients who receive search
> engine sync
> data should first write that data into the DSE prefs if the "make default"
> bit
> is set, then write the data to the web DB.
>

We should never write this to the Web DB. It's a synthesized bit that comes
from Prefs. It is only used to indicate the "user selected DSE." If we
receive a sync event with that contains the bit, we set the user pref to
that SE. If effective DSE changes, and the origin is user prefs, we send a
sync event for that SE with the bit set.

I think this avoids the most common weird ordering scenarios.  For example,
> if
> client A changes the default from engine 1 to engine 2 and then deletes
> engine
> 1, other clients are guaranteed to get the "make 2 default" message before
> the
> attempt to modify 1, so they won't be told to delete the current default
> engine,
> so they won't need to undelete it or have a ghost entry.
>
> It's still theoretically possible to get weird orderings.  For example,
> let's
> say the following happen simultaneously:
> * Client A changes the default from 1 to 2
> * Client B deletes 2
> Now when A gets B's message, it probably has to send back an undelete for
> 2.
>

Options:
(1) Undelete 2. Results in a consistent state where 2 is the DSE.
(2) Locally clear the user-selected DSE on client A. Respects the delete
action, but leaves the DSE inconsistent (B->1, A->fallback).

I think the undelete option is best. This is better, though, than the
current implementation because it actually requires legitimately
conflicting actions on two clients, whereas the current implementation (and
the current patch iteration) can do undeletes due to network/timing with
only a single client accessed.


> I don't think it's possible to ever eliminate all cases like these.  But I
> think
> this system of not syncing any search-related prefs at least banishes most
> of
> them to the realm of the theoretical, and makes them very rare in
> practice.  And
> I think the code is probably simpler than the previous proposal here.
>  Also, if
> some error happens and we try to disable either pref sync or search engine
> sync
> on some client, we don't wind up in a weird scenario where we're syncing
> half
> the data related to default search engines but not the other half -- if we
> turn
> of search engine sync on some client, it's all the way off.
>
> WDYT?
>
> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On 2014/05/06 19:15:57, erikwright wrote: > > Clients who receive search > > engine sync ...

6 years, 7 months ago (2014-05-06 19:30:12 UTC) #53

On 2014/05/06 19:15:57, erikwright wrote:
> > Clients who receive search
> > engine sync
> > data should first write that data into the DSE prefs if the "make default"
> > bit
> > is set, then write the data to the web DB.
> >
> 
> We should never write this to the Web DB. It's a synthesized bit that comes
> from Prefs. It is only used to indicate the "user selected DSE." If we
> receive a sync event with that contains the bit, we set the user pref to
> that SE. If effective DSE changes, and the origin is user prefs, we send a
> sync event for that SE with the bit set.

Sorry, I didn't word things clearly.  I didn't mean to write the "is default"
bit to the web DB.  (Indeed, ultimately an end goal of you guys' changes is to
remove that notion from the web DB entirely, right?)  I just meant, when I
client receives a sync change, if the engine details have this bit set, the
client should first write the new data to the prefs, before then proceeding on
to write whatever data to the web data service that would normally be written.

> > It's still theoretically possible to get weird orderings.  For example,
> > let's
> > say the following happen simultaneously:
> > * Client A changes the default from 1 to 2
> > * Client B deletes 2
> > Now when A gets B's message, it probably has to send back an undelete for
> > 2.
> >
> 
> Options:
> (1) Undelete 2. Results in a consistent state where 2 is the DSE.
> (2) Locally clear the user-selected DSE on client A. Respects the delete
> action, but leaves the DSE inconsistent (B->1, A->fallback).
> 
> I think the undelete option is best. This is better, though, than the
> current implementation because it actually requires legitimately
> conflicting actions on two clients, whereas the current implementation (and
> the current patch iteration) can do undeletes due to network/timing with
> only a single client accessed.

I agree.  If we took option (2), the clients would stay in disagreement
indefintely, until hopefully someday something caused them to be in agreement
again.  That's not a state we want to be in.  Better to do option (1).

I also concur with your analysis of this proposal versus the current patch.

Nicolas Zea

In practice, order is either preserved or squashed for actions within a datatype. By squash, ...

6 years, 7 months ago (2014-05-06 20:54:40 UTC) #54

Peter Kasting

On 2014/05/06 20:54:40, Nicolas Zea wrote: > In practice, order is either preserved or squashed ...

6 years, 7 months ago (2014-05-06 21:20:30 UTC) #55

On 2014/05/06 20:54:40, Nicolas Zea wrote:
> In practice, order is either preserved or squashed for actions within a
> datatype. By squash, I mean a set of changes are batched together, and other
> clients will receive them as part of the same ProcessSyncChanges batch. Within
> the SyncChangeList though, items are ordered arbitrarily, but they should be
> part of the same change list.

That seems bad.  We need a way to take these squashed changes and place them
back into the order in which they originally happened, in case one change in the
set depends on the results of another, as in the example I gave with "change
default from engine 1 to 2, delete engine 1", where the delete cannot be handled
correctly until after the "change default" has been handled.

> Having an "is default" bit in the search engine sync data is certainly an
> option. The issue then is what happens when more than one search engine has
the
> default bit set (possible whenever clients make changes based on stale data).

Can you describe more about what the last parenthetical here means?

> And then, do you automatically unset the default bit on the loser(s)?

Note that this bit is only part of the sync data; it's a processing instruction,
not an actual piece of a search engine's data from the client's perspective.  If
we receive multiple changes which say "make default" and process them in order,
the result is that we set a succession of different engines as default.  There's
no need to resolve or unset anything.

Again, this relies on being able to process all changes in a precise order.

> An alternative is don't use a bit, use some sort of ranking (e.g. timestamp or
> some other monotonically increasing position resistant to ties, similar to the
> uniquepositions class we have in sync). That way you don't have to unset the
> loser. You would still have the undelete logic you mentioned above, but you're
> now more resistant to the case where an old default search engine somehow
comes
> back from the dead.

Given my comments above, I don't know whether this paragraph even makes sense.

Peter Kasting

I might as well mail this comment that's sitting around. https://codereview.chromium.org/268643002/diff/390001/chrome/browser/search_engines/util.cc File chrome/browser/search_engines/util.cc (right): https://codereview.chromium.org/268643002/diff/390001/chrome/browser/search_engines/util.cc#newcode219 ...

6 years, 7 months ago (2014-05-06 21:20:57 UTC) #56

Nicolas Zea

On 2014/05/06 21:20:30, Peter Kasting wrote: > On 2014/05/06 20:54:40, Nicolas Zea wrote: > > ...

6 years, 7 months ago (2014-05-06 21:48:02 UTC) #57

On 2014/05/06 21:20:30, Peter Kasting wrote:
> On 2014/05/06 20:54:40, Nicolas Zea wrote:
> > In practice, order is either preserved or squashed for actions within a
> > datatype. By squash, I mean a set of changes are batched together, and other
> > clients will receive them as part of the same ProcessSyncChanges batch.
Within
> > the SyncChangeList though, items are ordered arbitrarily, but they should be
> > part of the same change list.
> 
> That seems bad.  We need a way to take these squashed changes and place them
> back into the order in which they originally happened, in case one change in
the
> set depends on the results of another, as in the example I gave with "change
> default from engine 1 to 2, delete engine 1", where the delete cannot be
handled
> correctly until after the "change default" has been handled.

This can be handled easily enough by the SE sync logic. While iterating over the
SyncChangeList, save to the side any deletes until adds/updates have been
handled, and then process the deletes.

> 
> > Having an "is default" bit in the search engine sync data is certainly an
> > option. The issue then is what happens when more than one search engine has
> the
> > default bit set (possible whenever clients make changes based on stale
data).
> 
> Can you describe more about what the last parenthetical here means?

Client 1 is not syncing for some time (e.g. user hasn't reauthed credentials).
While it wasn't syncing, it had an old default X, which underwent some change
(e.g. user modified keyword). Some time later, on a Client 2, which is syncing,
the user changed the default to Y, and deletes X.

When client 1 comes back online, it'll commit the locally modified snapshot of X
(in addition to downloading Y). This will automatically undelete X, and sync it
back around. At this point, clients 1 and 2 have both X and Y with the default
bit set.

> 
> > And then, do you automatically unset the default bit on the loser(s)?
> 
> Note that this bit is only part of the sync data; it's a processing
instruction,
> not an actual piece of a search engine's data from the client's perspective. 
If
> we receive multiple changes which say "make default" and process them in
order,
> the result is that we set a succession of different engines as default. 
There's
> no need to resolve or unset anything.
> 
> Again, this relies on being able to process all changes in a precise order.

Note that Sync is not transferring around events/changes, but transferring
around snapshots of data. It's possible to build an ordered change system on top
of this, but it would require more control over the sync internals than the API
currently exposes. We're working on improving this, by for example letting the
datatype decide how to handle conflicts like the one mentioned above, but we're
not there yet.

> 
> > An alternative is don't use a bit, use some sort of ranking (e.g. timestamp
or
> > some other monotonically increasing position resistant to ties, similar to
the
> > uniquepositions class we have in sync). That way you don't have to unset the
> > loser. You would still have the undelete logic you mentioned above, but
you're
> > now more resistant to the case where an old default search engine somehow
> comes
> > back from the dead.
> 
> Given my comments above, I don't know whether this paragraph even makes sense.

Peter Kasting

On 2014/05/06 21:48:02, Nicolas Zea wrote: > On 2014/05/06 21:20:30, Peter Kasting wrote: > > ...

6 years, 7 months ago (2014-05-06 22:01:21 UTC) #58

On 2014/05/06 21:48:02, Nicolas Zea wrote:
> On 2014/05/06 21:20:30, Peter Kasting wrote:
> > On 2014/05/06 20:54:40, Nicolas Zea wrote:
> > > In practice, order is either preserved or squashed for actions within a
> > > datatype. By squash, I mean a set of changes are batched together, and
other
> > > clients will receive them as part of the same ProcessSyncChanges batch.
> Within
> > > the SyncChangeList though, items are ordered arbitrarily, but they should
be
> > > part of the same change list.
> > 
> > That seems bad.  We need a way to take these squashed changes and place them
> > back into the order in which they originally happened, in case one change in
> the
> > set depends on the results of another, as in the example I gave with "change
> > default from engine 1 to 2, delete engine 1", where the delete cannot be
> handled
> > correctly until after the "change default" has been handled.
> 
> This can be handled easily enough by the SE sync logic. While iterating over
the
> SyncChangeList, save to the side any deletes until adds/updates have been
> handled, and then process the deletes.

This ensures we don't try (and fail) to delete the current default, but it still
means we could set the wrong engine as default:

Client A:
* Changes default from engine 1 to 2
* Changes default from engine 2 to 3

Now client B:
* Receives a squashed set of changes with two engine snapshots, one each for
engines 2 and 3, both marked "make default".
* If engine 3 gets processed first, client B will end up with engine 2 as
default, client A will have engine 3 as default.

This is why we need all changes to be ordered, rather than just having one type
of change processed last.

We could perhaps enforce this ourselves by ensuring that every change sent to
sync contains an updated last_modified time, and each client locally ensures all
such times are unique and monotonically increasing for changes from that client.
 Also, if a client receives a sync change with timestamp N and processes it, all
future changes this client sends back up should be stamped later than N.  If
both of these are done, then we can sort the changes within a squashed set by
increasing timestamp and ensure we don't have any conflicts other than those
that happen because of simultaneous conflicting changes on separate clients.

> > > Having an "is default" bit in the search engine sync data is certainly an
> > > option. The issue then is what happens when more than one search engine
has
> > the
> > > default bit set (possible whenever clients make changes based on stale
> data).
> > 
> > Can you describe more about what the last parenthetical here means?
> 
> Client 1 is not syncing for some time (e.g. user hasn't reauthed credentials).
> While it wasn't syncing, it had an old default X, which underwent some change
> (e.g. user modified keyword). Some time later, on a Client 2, which is
syncing,
> the user changed the default to Y, and deletes X.
> 
> When client 1 comes back online, it'll commit the locally modified snapshot of
X
> (in addition to downloading Y). This will automatically undelete X, and sync
it
> back around. At this point, clients 1 and 2 have both X and Y with the default
> bit set.

Remember that the "default bit" is not a bit stored on search engines locally,
it's just a processing instruction included with a sync change.  So when client
1 sends the server its snapshot of X, it will set this bit, since X is the local
default.  When client 2 receives this, it will make X default.  The worrisome
bit is whether during or after this, client 1 receives the synced "change to Y;
delete X" actions from 2.  Because if so, then basically the clients will swap
places.

My hope is that somehow rather than passing in flight these changes get resolved
against each other by the server somewhere.  I don't really care which one wins,
as long as both clients are told the same state of the world.

> > If
> > we receive multiple changes which say "make default" and process them in
> order,
> > the result is that we set a succession of different engines as default. 
> There's
> > no need to resolve or unset anything.
> > 
> > Again, this relies on being able to process all changes in a precise order.
> 
> Note that Sync is not transferring around events/changes, but transferring
> around snapshots of data.

Right, I understand that.  Feel free to substitute "snapshots" in for "changes"
in my writing.

erikwright (departed)

The plan that Peter and I proposed earlier (AIUI) did not include sending an update ...

6 years, 7 months ago (2014-05-07 01:19:36 UTC) #59

The plan that Peter and I proposed earlier (AIUI) did not include sending
an update to unset the default bit. We assumed we would treat whatever we
were last told was the default as the default.

But it seems like there is no guarantee that the last thing that a client
said was the default will be the last one that we are told is the default,
because of the squashing that was described.

For example (3 clients transmit and receive events in the following order):

Client 1: Tx SE A is default
Client 2: Rx SE A is default
Client 1: Tx SE B is default
Client 2: Tx SE A keyword update
Client 3: Rx SE B is default
Client 3: Rx SE A is default + keyword update (squashed)

That is assuming that events are squashed "forwards" (i.e. the combined
event occurs in the position of the last event). If they are squashed
backwards the result could be the same.

We could consider sending a "not default" but that seems unreliable too.

I think I inferred that Peter was suggesting syncing "change objects" (each
representing a mutation of an SE). And Nicolas seems to be suggesting that
the Sync protocol technically supports sending/receiving an ordered stream
of events like we want. But from what I understand, while the protocol
might support it, the client API does not expose that support.

Either way it seems like the "is default bit" either definitely doesn't
work or we are not sure it works.

And the transaction log approach, while it seems like it might be the most
formally correct approach, seems like it requires its own design document
and engineering effort.

The CL that is uploaded now uses Prefs to sync the user-selected DSE and
Web Data to sync everything else. It seems to work correctly except for the
undelete scenario, which the user can easily correct when it occurs. That
problem already exists today. It also affects an extremely small proportion
of our users (as very few users actually change DSEs and SEs).

Many users, today, are affected by DSE hijacking. It seems the only viable
solution to that hijacking in the M36 timeframe is the CL that is uploaded
now.

I propose that we move forward with that approach.


On Tue, May 6, 2014 at 6:01 PM, <pkasting@chromium.org> wrote:

> On 2014/05/06 21:48:02, Nicolas Zea wrote:
>
>> On 2014/05/06 21:20:30, Peter Kasting wrote:
>> > On 2014/05/06 20:54:40, Nicolas Zea wrote:
>> > > In practice, order is either preserved or squashed for actions within
>> a
>> > > datatype. By squash, I mean a set of changes are batched together, and
>>
> other
>
>> > > clients will receive them as part of the same ProcessSyncChanges
>> batch.
>> Within
>> > > the SyncChangeList though, items are ordered arbitrarily, but they
>> should
>>
> be
>
>> > > part of the same change list.
>> >
>> > That seems bad.  We need a way to take these squashed changes and place
>> them
>> > back into the order in which they originally happened, in case one
>> change in
>> the
>> > set depends on the results of another, as in the example I gave with
>> "change
>> > default from engine 1 to 2, delete engine 1", where the delete cannot be
>> handled
>> > correctly until after the "change default" has been handled.
>>
>
>  This can be handled easily enough by the SE sync logic. While iterating
>> over
>>
> the
>
>> SyncChangeList, save to the side any deletes until adds/updates have been
>> handled, and then process the deletes.
>>
>
> This ensures we don't try (and fail) to delete the current default, but it
> still
> means we could set the wrong engine as default:
>
> Client A:
> * Changes default from engine 1 to 2
> * Changes default from engine 2 to 3
>
> Now client B:
> * Receives a squashed set of changes with two engine snapshots, one each
> for
> engines 2 and 3, both marked "make default".
> * If engine 3 gets processed first, client B will end up with engine 2 as
> default, client A will have engine 3 as default.
>
> This is why we need all changes to be ordered, rather than just having one
> type
> of change processed last.
>
> We could perhaps enforce this ourselves by ensuring that every change sent
> to
> sync contains an updated last_modified time, and each client locally
> ensures all
> such times are unique and monotonically increasing for changes from that
> client.
>  Also, if a client receives a sync change with timestamp N and processes
> it, all
> future changes this client sends back up should be stamped later than N.
>  If
> both of these are done, then we can sort the changes within a squashed set
> by
> increasing timestamp and ensure we don't have any conflicts other than
> those
> that happen because of simultaneous conflicting changes on separate
> clients.
>
>
>  > > Having an "is default" bit in the search engine sync data is
>> certainly an
>> > > option. The issue then is what happens when more than one search
>> engine
>>
> has
>
>> > the
>> > > default bit set (possible whenever clients make changes based on stale
>> data).
>> >
>> > Can you describe more about what the last parenthetical here means?
>>
>
>  Client 1 is not syncing for some time (e.g. user hasn't reauthed
>> credentials).
>> While it wasn't syncing, it had an old default X, which underwent some
>> change
>> (e.g. user modified keyword). Some time later, on a Client 2, which is
>>
> syncing,
>
>> the user changed the default to Y, and deletes X.
>>
>
>  When client 1 comes back online, it'll commit the locally modified
>> snapshot of
>>
> X
>
>> (in addition to downloading Y). This will automatically undelete X, and
>> sync
>>
> it
>
>> back around. At this point, clients 1 and 2 have both X and Y with the
>> default
>> bit set.
>>
>
> Remember that the "default bit" is not a bit stored on search engines
> locally,
> it's just a processing instruction included with a sync change.  So when
> client
> 1 sends the server its snapshot of X, it will set this bit, since X is the
> local
> default.  When client 2 receives this, it will make X default.  The
> worrisome
> bit is whether during or after this, client 1 receives the synced "change
> to Y;
> delete X" actions from 2.  Because if so, then basically the clients will
> swap
> places.
>
> My hope is that somehow rather than passing in flight these changes get
> resolved
> against each other by the server somewhere.  I don't really care which one
> wins,
> as long as both clients are told the same state of the world.
>
>
>  > If
>> > we receive multiple changes which say "make default" and process them in
>> order,
>> > the result is that we set a succession of different engines as default.
>> There's
>> > no need to resolve or unset anything.
>> >
>> > Again, this relies on being able to process all changes in a precise
>> order.
>>
>
>  Note that Sync is not transferring around events/changes, but transferring
>> around snapshots of data.
>>
>
> Right, I understand that.  Feel free to substitute "snapshots" in for
> "changes"
> in my writing.
>
> https://codereview.chromium.org/268643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

(Aside: Please don't quote the entire previous reply in your reply, I get a huge ...

6 years, 7 months ago (2014-05-07 01:28:25 UTC) #60

(Aside: Please don't quote the entire previous reply in your reply, I get a huge
email, and also have to delete the whole thing when replying in return.)

On 2014/05/07 01:19:36, erikwright wrote:
> The plan that Peter and I proposed earlier (AIUI) did not include sending
> an update to unset the default bit. We assumed we would treat whatever we
> were last told was the default as the default.

"Unset the default bit" doesn't even make sense as a phrase.  There is no
persistent, set default bit on something.

> But it seems like there is no guarantee that the last thing that a client
> said was the default will be the last one that we are told is the default,
> because of the squashing that was described.
> 
> For example (3 clients transmit and receive events in the following order):
> 
> Client 1: Tx SE A is default
> Client 2: Rx SE A is default
> Client 1: Tx SE B is default
> Client 2: Tx SE A keyword update
> Client 3: Rx SE B is default
> Client 3: Rx SE A is default + keyword update (squashed)

This can't happen the way you describe.  Client 1 transmits SE A default, then
SE B default, so client 3 has to receieve them either in that order, or squashed
together.

Furthermore, as long as clients do not set the "make this the default" bit
unless the default engine is actually changing (I mean, we are making a
different search engine the default engine, not "the user is editing the default
engine"), I don't think there are any problems along the lines you're thinking.

> We could consider sending a "not default" but that seems unreliable too.

This is not necessary.

> I think I inferred that Peter was suggesting syncing "change objects" (each
> representing a mutation of an SE). And Nicolas seems to be suggesting that
> the Sync protocol technically supports sending/receiving an ordered stream
> of events like we want. But from what I understand, while the protocol
> might support it, the client API does not expose that support.

I don't understand the distinction you're making here.

We send sync updates when something changes.  A sync update is a snapshot of a
search engine, coupled with a small amount of other data, e.g. in our case a
"when processing this engine, you should also make it the default".

> Either way it seems like the "is default bit" either definitely doesn't
> work or we are not sure it works.

Think through it again.  You're being needlessly pessimistic.

> And the transaction log approach, while it seems like it might be the most
> formally correct approach, seems like it requires its own design document
> and engineering effort.

I don't know what approach this is.

> The CL that is uploaded now uses Prefs to sync the user-selected DSE and
> Web Data to sync everything else. It seems to work correctly except for the
> undelete scenario, which the user can easily correct when it occurs. That
> problem already exists today. It also affects an extremely small proportion
> of our users (as very few users actually change DSEs and SEs).

The CL as uploaded today seems likely to introduce duplicates frequently -- much
more frequently than the existing codebase.  I am not willing to proceed with
it.

> Many users, today, are affected by DSE hijacking. It seems the only viable
> solution to that hijacking in the M36 timeframe is the CL that is uploaded
> now.
> 
> I propose that we move forward with that approach.

Sorry, I can't support this.

erikwright (departed)

OK, I had completely misinterpreted zea's comment about squashing. I read it in the 'git' ...

6 years, 7 months ago (2014-05-07 02:45:58 UTC) #61

OK, I had completely misinterpreted zea's comment about squashing. I read
it in the 'git' sense but now I see it differently.

And I understand that the "timestamp" that peter is talking about is
basically a sequence number. Using time seems problematic because a single
client with an off clock could force everyone to use times much greater
than "now", in which case the timestamp devolves to a sequence number,
anyway, but one that will wrap much sooner than a pure sequence number.

Peter says "if both of these are done, then we can sort the changes within
a squashed set by increasing timestamp and ensure we don't have any
conflicts *other than those that happen because of simultaneous conflicting
changes on separate clients.*"

But "simultaneous" is in the eyes of the beholder. Imagine the following
three client scenario:

Client 1 sets a DSE with sequence number N.
Client 2 receives DSE with sequence number N before going "offline".
Client 2 makes 10 DSE changes. The last one has DSE sequence number N+10.
None of them are sent up to sync since this client is offline.
Client 1 sets a DSE with sequence number N+1.
Client 3 receives DSEs N and N+1, and uses N+1.
Client 2 goes online and sends its data.
Client 3 receives DSE N+10, which is stale, and uses it.

This can lead to an arbitrarily stale DSE replacing a much more recent DSE.

But I think we can make this work:

(1) Define a "DSE sequence" field on SEs.
(2) If we receive a sync add/update with a "DSE sequence" number that is
higher than our current user-selected DSE, we replace our current
user-selected DSE with that value.
(3) If we sync, and do _not_ receive a DSE sequence number higher than our
current user-selected DSE, we check to see if our current user-selected DSE
is "pending sync". If so, we send a sync update with our current
user-selected DSE and a sequence number that is one greater than the last
DSE sequence number we saw.
(4) When we change the user-selected DSE locally, we don't send it up right
away. Instead we mark the DSE as "pending sync". When the next sync
transaction occurs (and we are as up-to-date as is possible), step (3) will
do the sending.
(5) If we receive conflicting DSEs with the same DSE sequence number, sort
them by their Sync GUID and pick the winner.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On Tue, May 6, 2014 at 7:45 PM, Erik Wright <erikwright@chromium.org> wrote: > Client 1 ...

6 years, 7 months ago (2014-05-07 02:57:10 UTC) #62

On Tue, May 6, 2014 at 7:45 PM, Erik Wright <erikwright@chromium.org> wrote:

> Client 1 sets a DSE with sequence number N.
> Client 2 receives DSE with sequence number N before going "offline".
> Client 2 makes 10 DSE changes. The last one has DSE sequence number N+10.
> None of them are sent up to sync since this client is offline.
> Client 1 sets a DSE with sequence number N+1.
> Client 3 receives DSEs N and N+1, and uses N+1.
> Client 2 goes online and sends its data.
> Client 3 receives DSE N+10, which is stale, and uses it.
>
> This can lead to an arbitrarily stale DSE replacing a much more recent DSE.
>

I think that's fine.  In this case, I actually think client 2's 10 changes
_should_ take effect.

Trying to make these sorts of sequencing changes globally ordered and
unique basically devolves to the sort of global timekeeping problem Spanner
tries to solve, which is much more complex than we want to try to do here.
 I really only proposed it for sequencing within a client, and for some
simple best-effort sequencing between clients, and we could drop the latter
without too much negative effect.

But I think we can make this work:
>
> (1) Define a "DSE sequence" field on SEs.
> (2) If we receive a sync add/update with a "DSE sequence" number that is
> higher than our current user-selected DSE, we replace our current
> user-selected DSE with that value.
> (3) If we sync, and do _not_ receive a DSE sequence number higher than our
> current user-selected DSE, we check to see if our current user-selected DSE
> is "pending sync". If so, we send a sync update with our current
> user-selected DSE and a sequence number that is one greater than the last
> DSE sequence number we saw.
> (4) When we change the user-selected DSE locally, we don't send it up
> right away. Instead we mark the DSE as "pending sync". When the next sync
> transaction occurs (and we are as up-to-date as is possible), step (3) will
> do the sending.
> (5) If we receive conflicting DSEs with the same DSE sequence number, sort
> them by their Sync GUID and pick the winner.
>

I think this is more complexity than the situation warrants.

I would just ensure sync events from each particular client have a
timestamp or other value that allows ordering them (within-client) when
received by other clients; that we set the "make the relevant engine
default" bit on the sync event if and only if the client did some kind of
SetDefaultSearchProvider()-type call; and that clients which receive events
order any squashed events, then process them in order, updating both prefs
and web data store for all affected engines, but only calling
SetDefaultSearchProvider() or the like when the "make default" bit is set.

If we do this, I think we basically get reasonable behavior.

PK

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

erikwright (departed)

On Tue, May 6, 2014 at 10:57 PM, Peter Kasting <pkasting@chromium.org>wrote: > On Tue, May ...

6 years, 7 months ago (2014-05-07 03:49:18 UTC) #63

On Tue, May 6, 2014 at 10:57 PM, Peter Kasting <pkasting@chromium.org>wrote:

> On Tue, May 6, 2014 at 7:45 PM, Erik Wright <erikwright@chromium.org>wrote:
>
>> Client 1 sets a DSE with sequence number N.
>> Client 2 receives DSE with sequence number N before going "offline".
>> Client 2 makes 10 DSE changes. The last one has DSE sequence number N+10.
>> None of them are sent up to sync since this client is offline.
>> Client 1 sets a DSE with sequence number N+1.
>> Client 3 receives DSEs N and N+1, and uses N+1.
>> Client 2 goes online and sends its data.
>> Client 3 receives DSE N+10, which is stale, and uses it.
>>
>> This can lead to an arbitrarily stale DSE replacing a much more recent
>> DSE.
>>
>
> I think that's fine.  In this case, I actually think client 2's 10 changes
> _should_ take effect.
>

I can't see why they "should" take effect other than according to the
implementation rules. From a user's point of view, this would be annoying
and arbitrary. Basically we divided the world into online and offline. When
the two worlds join, we have to pick a winner. And we do it by which one
has made the most transactions in that time?

I would just ensure sync events from each particular client have a
> timestamp or other value that allows ordering them (within-client) when
> received by other clients; that we set the "make the relevant engine
> default" bit on the sync event if and only if the client did some kind of
> SetDefaultSearchProvider()-type call; and that clients which receive events
> order any squashed events, then process them in order, updating both prefs
> and web data store for all affected engines, but only calling
> SetDefaultSearchProvider() or the like when the "make default" bit is set.
>
> If we do this, I think we basically get reasonable behavior.
>

I asked Peter this question by IM: Is the sequence number only used for
selecting a winner within a batch, or do we ignore incoming DSEs that have
sequence numbers lower than the last one received in a prior batch?

Peter's feeling is that we are only using the sequence number within a
batch, and we are not comparing sequence numbers from different clients.

My response:

In a 3 client scenario. Clients 1 and 2 have both set DSEs.
Client 3 receives both in the same batch, you need to pick one.
And you need to pick the same one that clients 1 and 2 will ultimately pick.

Peter felt that some kind of conflict resolution would occur on the server.
But I believe this is a misunderstanding - conflict resolution AFAIUI is
only done on a per-entity basis. Since the "is DSE" bit is being set on
different entities, the server is not going to decide which one should win.

We discussed some possibilities involving server-side changes to support
that. Those are far enough in the future that they would seriously clash
with our objectives.

Peter proposed a solution that involves having a single "DSE" entity that
is part of the SE dataset. I guess it would basically be a search engine,
but it would have a special well-known sync GUID that allows all clients to
refer to it reliably.

It could either have all of the properties of the DSE, or it could just
have a reference to the DSE. I think that duplicating the properties of the
DSE re-introduces a lot of the inconsistency issues, so I will assume that
it just has a field that contains the sync GUID of the real SE entry that
happens to be the DSE.

If that's the case, I think it still suffers from all of the same
"undelete" issues that we have already been struggling with.

The options that seem implementable in less than a month are:

(1) The currently uploaded approach (sync the DSE via prefs, and everything
else via SE).
(2) The sequence number approach I outlined (make a best effort to
synchronize the latest sequence number with a server, resolve clashes by
ranking sync GUIDs)
(3) Peter's sequence number approach, but with considering sequences
between clients (when a client comes back online its stale value still wins
if there have been more local updates than remote updates).
(4) A "special" search engine that contains a reference to the normal SE
that happens to be the DSE.

I include (1) even though Peter is concerned that it introduces more
duplicates than the current implementation. I don't understand how, so I
would appreciate it if Peter could share his perspective on that.

In any case, I'm happy to implement any of the above options. I have a
preference for (1) because it's already implemented. I would like to
understand more concretely why it might be bad.

Of the others, (3) seems like the easiest to implement, so I would make it
my second choice.

Thanks for chiming in with your own thoughts.

> PK
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On Tue, May 6, 2014 at 8:49 PM, Erik Wright <erikwright@chromium.org> wrote: > On Tue, ...

6 years, 7 months ago (2014-05-07 06:32:47 UTC) #64

On Tue, May 6, 2014 at 8:49 PM, Erik Wright <erikwright@chromium.org> wrote:

> On Tue, May 6, 2014 at 10:57 PM, Peter Kasting <pkasting@chromium.org>wrote:
>
>> On Tue, May 6, 2014 at 7:45 PM, Erik Wright <erikwright@chromium.org>wrote:
>>
>>> Client 1 sets a DSE with sequence number N.
>>> Client 2 receives DSE with sequence number N before going "offline".
>>> Client 2 makes 10 DSE changes. The last one has DSE sequence number
>>> N+10. None of them are sent up to sync since this client is offline.
>>> Client 1 sets a DSE with sequence number N+1.
>>> Client 3 receives DSEs N and N+1, and uses N+1.
>>> Client 2 goes online and sends its data.
>>> Client 3 receives DSE N+10, which is stale, and uses it.
>>>
>>> This can lead to an arbitrarily stale DSE replacing a much more recent
>>> DSE.
>>>
>>
>> I think that's fine.  In this case, I actually think client 2's 10
>> changes _should_ take effect.
>>
>
> I can't see why they "should" take effect other than according to the
> implementation rules. From a user's point of view, this would be annoying
> and arbitrary. Basically we divided the world into online and offline. When
> the two worlds join, we have to pick a winner. And we do it by which one
> has made the most transactions in that time?
>

If, for example, you suddenly enable sync on a client, then the current
state of that client is what should sync to other clients.  It would be
weird if instead somehow sync took into account the time at which you last
made various changes to the client's search engines, relative to when you
made changes on other clients.

Another, different argument: the goal of this algorithm is a consistent
worldview and reasonable behavior in the common case.  I really don't care
if the fallout is that in some obscure edge case you get a consistent but
suboptimal worldview, which is easily correctable.

> I would just ensure sync events from each particular client have a
>> timestamp or other value that allows ordering them (within-client) when
>> received by other clients; that we set the "make the relevant engine
>> default" bit on the sync event if and only if the client did some kind of
>> SetDefaultSearchProvider()-type call; and that clients which receive events
>> order any squashed events, then process them in order, updating both prefs
>> and web data store for all affected engines, but only calling
>> SetDefaultSearchProvider() or the like when the "make default" bit is set.
>>
>> If we do this, I think we basically get reasonable behavior.
>>
>
> I asked Peter this question by IM: Is the sequence number only used for
> selecting a winner within a batch, or do we ignore incoming DSEs that have
> sequence numbers lower than the last one received in a prior batch?
>
> Peter's feeling is that we are only using the sequence number within a
> batch, and we are not comparing sequence numbers from different clients.
>

Note that I previously assumed comparing numbers cross clients, but as of
the IM conversation Erik references, I'm concerned that that might cause
more problems than it solves.

We discussed some possibilities involving server-side changes to support
> that. Those are far enough in the future that they would seriously clash
> with our objectives.
>

Well, it depends.  If conflict resolution implementation is extremely
simple, it's potentially feasible to implement somewhere.  Really, the sync
folks need to tell us how to proceed here.

Peter proposed a solution that involves having a single "DSE" entity that
> is part of the SE dataset. I guess it would basically be a search engine,
> but it would have a special well-known sync GUID that allows all clients to
> refer to it reliably.
>

This isn't what I was thinking.

Basically, I want the equivalent of the synced DSE GUID pref, except I want
it not as a pref that gets synced as part of prefs, but as some identifier
that gets synced as part of search engines.  The reason to request this is
that the server could presumably resolve conflicts in updates to this value
and ensure all clients ultimately saw the same value.

I will assume that it just has a field that contains the sync GUID of the
> real SE entry that happens to be the DSE.
>

This is sort of equivalent to what I was suggesting, I guess.

If that's the case, I think it still suffers from all of the same
> "undelete" issues that we have already been struggling with.
>

This doesn't have any more problems than the "make default" bit.  Yes, you
can have undeletion if client A deletes an engine and then client B edits
it, but that's basically unavoidable.  Undeletion is not something I'm
worried about.

(1) The currently uploaded approach (sync the DSE via prefs, and everything
> else via SE).
>
> I include (1) even though Peter is concerned that it introduces more
> duplicates than the current implementation. I don't understand how, so I
> would appreciate it if Peter could share his perspective on that.
>

Lemme hold off on that until we get done discussing the stuff above to see
if it's feasible to begin with, since I may be wrong about my concerns.

PK

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

erikwright (departed)

> >>> Client 1 sets a DSE with sequence number N. > >>> Client 2 ...

6 years, 7 months ago (2014-05-07 12:53:16 UTC) #65

> >>> Client 1 sets a DSE with sequence number N.
> >>> Client 2 receives DSE with sequence number N before going "offline".
> >>> Client 2 makes 10 DSE changes. The last one has DSE sequence number
> >>> N+10. None of them are sent up to sync since this client is offline.
> >>> Client 1 sets a DSE with sequence number N+1.
> >>> Client 3 receives DSEs N and N+1, and uses N+1.
> >>> Client 2 goes online and sends its data.
> >>> Client 3 receives DSE N+10, which is stale, and uses it.
> >>>
> >>> This can lead to an arbitrarily stale DSE replacing a much more recent
> >>> DSE.

...

> If, for example, you suddenly enable sync on a client, then the current
> state of that client is what should sync to other clients.  It would be
> weird if instead somehow sync took into account the time at which you last
> made various changes to the client's search engines, relative to when you
> made changes on other clients.

That might be sensible if we are talking about enabling sync for the first time
on a device. I'm not sure if that works exactly the same way as when a device
that is already sync'ing comes back online after a long period of disconnection.
Whether that is correct UX for a new device seems debatable but I'm willing to
go along with it for the sake of argument.

If I have a device that was sync'ing, but is then offline for a very long time
(ex. an old cellphone or laptop that gets put away for 18 months until suddenly
I realize it contains important photos/documents) it seems clear to me that I
don't want to suddenly turn it on and have its values override my current
values.

> Another, different argument: the goal of this algorithm is a consistent
> worldview and reasonable behavior in the common case.  I really don't care
> if the fallout is that in some obscure edge case you get a consistent but
> suboptimal worldview, which is easily correctable.

I agree that consistency is very important, perhaps behind fault tolerance.
After that comes striking a balance between predictability and simplicity.

I think this is the order of priorities we care about:

Fault tolerance: If any part or parts of the system fail, we don't bring down
the rest of the system (i.e., prefs stop syncing, we don't flood the SE sync
servers).
Consistency: Clients always eventually end up with the right state if they are
connected to the backend and the backend is working.
Predictability: For most reasonable things a user might do, when the backend is
working properly, the results are what they expect (consistent data that
reflects their most recent actions).
Simplicity: We can understand it, rationalize about the extent to which it meets
the above criteria, and implement it with low risk of regressions.

In other words, although I don't necessarily agree about what you think is
desirable in that edge case, I agree that it's relatively unlikely and the user
has a way to correct it. And I agree that other solutions may be less complex.

> > I would just ensure sync events from each particular client have a
> >> timestamp or other value that allows ordering them (within-client) when
> >> received by other clients; that we set the "make the relevant engine
> >> default" bit on the sync event if and only if the client did some kind of
> >> SetDefaultSearchProvider()-type call; and that clients which receive events
> >> order any squashed events, then process them in order, updating both prefs
> >> and web data store for all affected engines, but only calling
> >> SetDefaultSearchProvider() or the like when the "make default" bit is set.
> >>
> >> If we do this, I think we basically get reasonable behavior.

...
 
> Note that I previously assumed comparing numbers cross clients, but as of
> the IM conversation Erik references, I'm concerned that that might cause
> more problems than it solves.

I don't think that it introduces any problems worse than what we previously
discussed (a client with N+10 DSE that happens to be old due to a long
disconnect ends up wiping an N+1 DSE that is newer).

> We discussed some possibilities involving server-side changes to support
> > that. Those are far enough in the future that they would seriously clash
> > with our objectives.
> >
> 
> Well, it depends.  If conflict resolution implementation is extremely
> simple, it's potentially feasible to implement somewhere.  Really, the sync
> folks need to tell us how to proceed here.

If it's possible to implement something with deficiencies comparable to
(ideally, identical or a strict subset of) what we currently have, I believe
that should be our first priority. Focusing on the user will lead us to see that
users are very vocally concerned about unwanted manipulation of their DSE. Much,
much more so than they are concerned about the existing deficiencies in sync.
That doesn't mean that we shouldn't explore opportunities to resolve
deficiencies in the long term.

I definitely do not propose accepting any new deficiencies.

> Basically, I want the equivalent of the synced DSE GUID pref, except I want
> it not as a pref that gets synced as part of prefs, but as some identifier
> that gets synced as part of search engines.  The reason to request this is
> that the server could presumably resolve conflicts in updates to this value
> and ensure all clients ultimately saw the same value.

Sure, but from my understanding each stream consists of a single entity type. So
while you can treat it as a different object locally, it will appear as an SE to
sync.

> This doesn't have any more problems than the "make default" bit.  Yes, you
> can have undeletion if client A deletes an engine and then client B edits
> it, but that's basically unavoidable.  Undeletion is not something I'm
> worried about.

I agree that undeletion is acceptable. I just want to compare it clearly to the
other proposals in terms of deficiencies. If there are multiple proposals with
the same deficiencies, the one that is easiest to implement is the one I would
choose.

erikwright (departed)

> >>> Client 1 sets a DSE with sequence number N. > >>> Client 2 ...

6 years, 7 months ago (2014-05-07 12:53:17 UTC) #66

> >>> Client 1 sets a DSE with sequence number N.
> >>> Client 2 receives DSE with sequence number N before going "offline".
> >>> Client 2 makes 10 DSE changes. The last one has DSE sequence number
> >>> N+10. None of them are sent up to sync since this client is offline.
> >>> Client 1 sets a DSE with sequence number N+1.
> >>> Client 3 receives DSEs N and N+1, and uses N+1.
> >>> Client 2 goes online and sends its data.
> >>> Client 3 receives DSE N+10, which is stale, and uses it.
> >>>
> >>> This can lead to an arbitrarily stale DSE replacing a much more recent
> >>> DSE.

...

> If, for example, you suddenly enable sync on a client, then the current
> state of that client is what should sync to other clients.  It would be
> weird if instead somehow sync took into account the time at which you last
> made various changes to the client's search engines, relative to when you
> made changes on other clients.

That might be sensible if we are talking about enabling sync for the first time
on a device. I'm not sure if that works exactly the same way as when a device
that is already sync'ing comes back online after a long period of disconnection.
Whether that is correct UX for a new device seems debatable but I'm willing to
go along with it for the sake of argument.

If I have a device that was sync'ing, but is then offline for a very long time
(ex. an old cellphone or laptop that gets put away for 18 months until suddenly
I realize it contains important photos/documents) it seems clear to me that I
don't want to suddenly turn it on and have its values override my current
values.

> Another, different argument: the goal of this algorithm is a consistent
> worldview and reasonable behavior in the common case.  I really don't care
> if the fallout is that in some obscure edge case you get a consistent but
> suboptimal worldview, which is easily correctable.

I agree that consistency is very important, perhaps behind fault tolerance.
After that comes striking a balance between predictability and simplicity.

I think this is the order of priorities we care about:

Fault tolerance: If any part or parts of the system fail, we don't bring down
the rest of the system (i.e., prefs stop syncing, we don't flood the SE sync
servers).
Consistency: Clients always eventually end up with the right state if they are
connected to the backend and the backend is working.
Predictability: For most reasonable things a user might do, when the backend is
working properly, the results are what they expect (consistent data that
reflects their most recent actions).
Simplicity: We can understand it, rationalize about the extent to which it meets
the above criteria, and implement it with low risk of regressions.

In other words, although I don't necessarily agree about what you think is
desirable in that edge case, I agree that it's relatively unlikely and the user
has a way to correct it. And I agree that other solutions may be less complex.

> > I would just ensure sync events from each particular client have a
> >> timestamp or other value that allows ordering them (within-client) when
> >> received by other clients; that we set the "make the relevant engine
> >> default" bit on the sync event if and only if the client did some kind of
> >> SetDefaultSearchProvider()-type call; and that clients which receive events
> >> order any squashed events, then process them in order, updating both prefs
> >> and web data store for all affected engines, but only calling
> >> SetDefaultSearchProvider() or the like when the "make default" bit is set.
> >>
> >> If we do this, I think we basically get reasonable behavior.

...
 
> Note that I previously assumed comparing numbers cross clients, but as of
> the IM conversation Erik references, I'm concerned that that might cause
> more problems than it solves.

I don't think that it introduces any problems worse than what we previously
discussed (a client with N+10 DSE that happens to be old due to a long
disconnect ends up wiping an N+1 DSE that is newer).

> We discussed some possibilities involving server-side changes to support
> > that. Those are far enough in the future that they would seriously clash
> > with our objectives.
> >
> 
> Well, it depends.  If conflict resolution implementation is extremely
> simple, it's potentially feasible to implement somewhere.  Really, the sync
> folks need to tell us how to proceed here.

If it's possible to implement something with deficiencies comparable to
(ideally, identical or a strict subset of) what we currently have, I believe
that should be our first priority. Focusing on the user will lead us to see that
users are very vocally concerned about unwanted manipulation of their DSE. Much,
much more so than they are concerned about the existing deficiencies in sync.
That doesn't mean that we shouldn't explore opportunities to resolve
deficiencies in the long term.

I definitely do not propose accepting any new deficiencies.

> Basically, I want the equivalent of the synced DSE GUID pref, except I want
> it not as a pref that gets synced as part of prefs, but as some identifier
> that gets synced as part of search engines.  The reason to request this is
> that the server could presumably resolve conflicts in updates to this value
> and ensure all clients ultimately saw the same value.

Sure, but from my understanding each stream consists of a single entity type. So
while you can treat it as a different object locally, it will appear as an SE to
sync.

> This doesn't have any more problems than the "make default" bit.  Yes, you
> can have undeletion if client A deletes an engine and then client B edits
> it, but that's basically unavoidable.  Undeletion is not something I'm
> worried about.

I agree that undeletion is acceptable. I just want to compare it clearly to the
other proposals in terms of deficiencies. If there are multiple proposals with
the same deficiencies, the one that is easiest to implement is the one I would
choose.

erikwright (departed)

Peter's concerns with changing the sync protocol are valid in that any new system is ...

6 years, 7 months ago (2014-05-07 16:10:09 UTC) #67

erikwright (departed)

pkasting, zea: PTAL. The latest and greatest is up and running through the try bots. ...

6 years, 7 months ago (2014-05-07 19:05:46 UTC) #68

gab

Just driving by here as I was reading some of this CL; didn't nearly have ...

6 years, 7 months ago (2014-05-07 20:25:07 UTC) #69

Nicolas Zea

Mostly LG from a sync perspective https://codereview.chromium.org/268643002/diff/600001/chrome/browser/search_engines/template_url_service.cc File chrome/browser/search_engines/template_url_service.cc (left): https://codereview.chromium.org/268643002/diff/600001/chrome/browser/search_engines/template_url_service.cc#oldcode1391 chrome/browser/search_engines/template_url_service.cc:1391: SetUserSelectedDefaultSearchProvider(pending_default); So I ...

6 years, 7 months ago (2014-05-07 20:30:05 UTC) #70

erikwright (departed)

engedy,zea,gab: Responded to all comments so far. https://codereview.chromium.org/268643002/diff/390001/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/268643002/diff/390001/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h#newcode72 chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:72: // and ...

6 years, 7 months ago (2014-05-07 22:26:45 UTC) #71

Peter Kasting

My mind kind of glazed over trying to look at this so this is more ...

6 years, 7 months ago (2014-05-07 23:38:27 UTC) #73

erikwright (departed)

I've pulled out a pre-CL (http://crrev.com/270533007/), and I've also reverted some unnecessary code moves. That ...

6 years, 7 months ago (2014-05-08 12:46:23 UTC) #74

I've pulled out a pre-CL (http://crrev.com/270533007/), and I've also reverted
some unnecessary code moves. That brings us down from (+891 lines, -1705 lines)
to (+677 lines, -1441 lines).

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/profile_...
File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc
(right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/profile_...
chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:89:
tree->SetInteger("usage_count", template_url->usage_count());
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: This list of keys is in a seemingly random order -- can it be made to
match
> the member order in TemplateURL?

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/profile_...
File
chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc
(right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/profile_...
chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:120:
// differences.
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Move the comments in both these arms above the conditional, then reverse
> the conditional to eliminate this empty arm.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_manager.cc (right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_manager.cc:133: :
fallback_default_search_.get();
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Please format as
> 
>   return TemplateURLService::FallbackSearchEnginesDisabled() ?
>       NULL : fallback_default_search_.get();

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_policy_handler_unittest.cc
(right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_policy_handler_unittest.cc:133:
const base::ListValue* list_value;
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Declare these locals just above the first line that uses each, rather
than
> all in this block.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
File chrome/browser/search_engines/default_search_pref_migration.cc (right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/default_search_pref_migration.cc:29: return
scoped_ptr<TemplateURLData>();
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Another possibility would be to declare |default_provider_data| atop the
> function and then always return it on all these cases.  Up to you.

Reverted this code move for simplicity.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:259: bool
TemplateURLService::fallback_search_engines_disabled_ = false;
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: This belongs below the subsequent divider, not above it

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:485: return result;
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Eliminates unnecessary temp:
> 
>   if (!AddNoNotify(template_url, true))
>     return false;
>   NotifyObservers();
>   return true;

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:655: (url->GetType() ==
TemplateURL::NORMAL);
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Weird indenting... normally, lines after the first should normally be
> indented 4, not even, though in this case I'd probably do:
> 
>   return
>       ((default_search_provider_source_ == DefaultSearchManager::FROM_USER) ||
>        (default_search_provider_source_ ==
>            DefaultSearchManager::FROM_FALLBACK)) &&
>       (url != GetDefaultSearchProvider()) &&
>       url->url_ref().SupportsReplacement() &&
>       (url->GetType() == TemplateURL::NORMAL);

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:685:
DefaultSearchManager::FROM_EXTENSION;
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: This should be indented 4, not 7

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:693:
default_search_provider_source_ == DefaultSearchManager::FROM_FALLBACK) {
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Parens around binary subexprs (file style)  (many places)

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:695: // prepopulated
engine). No matter what we will reset it at the end anyway.
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: How about:
> 
> Clear |default_search_provider_| in case we want to remove the engine it
points
> to.  This will get reset at the end of the function anyway.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:760: OnFailedLoad();
On 2014/05/07 23:38:29, Peter Kasting wrote:
> This should no longer cause a load failure (see latest version of my patch).

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1514: : NULL,
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Format as:
> 
>   ApplyDefaultSearchChange(
>       initial_default_search_provider_ ?
>           initial_default_search_provider_->data() : NULL,
>       default_search_provider_source_);

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1524: // just return
|initial_default_search_provider_|.
On 2014/05/07 23:38:29, Peter Kasting wrote:
> This comment is no longer meaningful.
> 
> This whole function can go away, see latest version of my patch.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1626:
default_search_provider_source_ ==
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Crazy indenting!  (11 instead of 4?)

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1658:
GetPrefs()->GetString(prefs::kSyncedDefaultSearchProviderGUID))) {
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Indent an additional 4, not 1

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1659:
default_search_manager_.SetUserSelectedDefaultSearchEngine(
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Indent 4 from "if", not 6

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1802: // Note that we
exclude if data == NULL because that could cause a false
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: if -> the case of

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1804: // due to policy. No
recursion case could lead to recursion with NULL.
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: "...We'll never actually get recursion with data == NULL." instead?

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1812: // We do this both
to clear up removed policy-defined DSE as well as to add
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: clear up removed -> remove any no-longer-applicable?

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1826: if (source ==
DefaultSearchManager::FROM_EXTENSION) {
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Rather than several successive "if (source == ...)", it might make sense
to
> have a switch statement here, to make it clear to the reader that these are
all
> parallel, mutually exclusive handler blocks.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1855: delete new_dse;
On 2014/05/07 23:38:29, Peter Kasting wrote:
> This isn't right.  AddNoNotify() always takes ownership of its arg, even when
> addition fails.  You're double-freeing.  (3 places)

That's disconcerting. I already noticed this myself and I'm sure I fixed it. I
must be losing my mind.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1873: new_dse->data_.id =
kInvalidTemplateURLID;
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Since TemplateURLData is supposed to basically be immutable once inside
> TemplateURLs, a more kosher way of doing this would probably be:
> 
>   TemplateURLData new_data(*data);
>   new_data.show_in_default_list = true;
>   new_data.id = kInvalidTemplateURLID;
>   TemplateURL* new_dse = new TemplateURL(profile_, new_data);
> 
> Then these first two lines could be pulled out of both arms and moved above
the
> conditional.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:1894: // TODO(erikwright):
should this only be if user-selected?
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Don't ask me -- but do be sure to get the RLZ pings right, we care a great
deal
> about them.  I don't know who owns this stuff, cpu might know.

I verified the prior implementation as well as checking with the relevant
people.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:375: // some tests,
specifically TemplateURLServiceSyncTests).
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: This is not a complete sentence.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:376: static bool
FallbackSearchEnginesDisabled() {
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: This should be named unix_hacker()-style if it's cheap and inlined.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:381: static void
DisableFallbackSearchEngines();
On 2014/05/07 23:38:29, Peter Kasting wrote:
> The implementation of this is just one line.  This could be inlined, and after
> that, could be ifdefed to prevent non-test use:
> 
> #if defined(UNIT_TEST)
>   static void set_fallback_search_engines_disabled() {
>     fallback_search_engines_disabled_ = true;
>   }
> #endif

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:448: // before and after
the attempted load.
On 2014/05/07 23:38:29, Peter Kasting wrote:
> This commentary looks to be from an out-of-date version of my code, and
wouldn't
> be accurate to describe your patch anyway.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:451: // Receives notice
that the DSE has changed.
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Receives from where?  Prefer to phrase such things as "Called by Foo() when". 
> (2 places)

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:456: // loaded state.
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: loaded -> the loaded

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:728: static bool
fallback_search_engines_disabled_;
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: This name shouldn't have a trailing '_' as it's not a nonstatic member. 
> The Google style guide suggests prefixing with "g_", which seems OK to me.  I
> don't think Chromium code has a consistent style with these sorts of things.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_unittest.cc:1212:
ASSERT_TRUE(old_default);
On 2014/05/07 23:38:29, Peter Kasting wrote:
> This can still be EXPECT_TRUE, we don't deref it.

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
File chrome/browser/search_engines/util.cc (right):

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/util.cc:109: i->second->prepopulate_id()) &&
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Indent an additional 4, not 1

Done.

https://codereview.chromium.org/268643002/diff/660001/chrome/browser/search_e...
chrome/browser/search_engines/util.cc:310:
default_search_provider->prepopulate_id()) ||
On 2014/05/07 23:38:29, Peter Kasting wrote:
> Nit: Indent an additional 4, not 1

Done.

erikwright (departed)

jochen: PTAL for chrome/browser/chrome_notification_types.h

6 years, 7 months ago (2014-05-08 12:51:11 UTC) #75

erikwright (departed)

Pulled out some cleanups into a follow-up CL. This one is now down to 13 ...

6 years, 7 months ago (2014-05-08 13:29:22 UTC) #77

Peter Kasting

We must be getting close if this is all the feedback I can muster. https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_engines/default_search_pref_migration_unittest.cc ...

6 years, 7 months ago (2014-05-08 22:46:45 UTC) #78

Peter Kasting

https://codereview.chromium.org/268643002/diff/900001/chrome/browser/search_engines/template_url_service_sync_unittest.cc File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/268643002/diff/900001/chrome/browser/search_engines/template_url_service_sync_unittest.cc#newcode248 chrome/browser/search_engines/template_url_service_sync_unittest.cc:248: TemplateURLService::set_fallback_search_engines_disabled(false); Hmmm. I wonder if, instead of having the ...

6 years, 7 months ago (2014-05-08 22:51:48 UTC) #79

erikwright (departed)

pkasting: PTAL. I will respond to the nits in the meantime. https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_engines/template_url_service.cc File chrome/browser/search_engines/template_url_service.cc (left): ...

6 years, 7 months ago (2014-05-09 18:00:20 UTC) #80

pkasting: PTAL. I will respond to the nits in the meantime.

https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (left):

https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:481: if (!prefs ||
load_failed_)
On 2014/05/08 22:46:46, Peter Kasting wrote:
> The functionality that was here -- not persisting state to anywhere when in
the
> load failure case -- doesn't seem to be implemented anywhere in your change
that
> I can see.  I still think we should go into fully-nonpersistent mode when the
> web DB load goes bad.  I'm worried about, for example, a user adding a new
> search engine, that is given a low ID number since no other engines are
loaded,
> and making that engine default -- which then gets stored in prefs, and causes
> dataloss when the web DB can once again load, because we blow away whatever
> engine had that ID (and maybe also have dupes, if a user was trying to re-add
an
> existing engine).

OK - I added some new code in SetUserSelectedDefaultSearchEngine that handles
this case.

https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:140: static void
SaveDefaultSearchProviderToPrefs(const TemplateURL* url,
On 2014/05/08 22:46:46, Peter Kasting wrote:
> Bad rebase?  This is the same way the code looks on trunk today, I think.
> 
> Also, is this function only called by tests now?  May want to note that if so.

> Unless you're just going to delete this in a followup change soon.

In your CL you made it non-static in order to not save when in a failed state.
This CL reverts that since it is now only used externally.

I deferred moving this function to the sole client (a test) in order to
facilitate your review of this CL. You have already LGTM'd the CL that contains
the move (I'll land it ASAP after this CL).

erikwright (departed)

OK, I replied to all nits. The only issue not replied to is the fallback ...

6 years, 7 months ago (2014-05-09 19:05:35 UTC) #81

Peter Kasting

LGTM https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_engines/template_url_service.h File chrome/browser/search_engines/template_url_service.h (right): https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_engines/template_url_service.h#newcode140 chrome/browser/search_engines/template_url_service.h:140: static void SaveDefaultSearchProviderToPrefs(const TemplateURL* url, On 2014/05/09 18:00:22, ...

6 years, 7 months ago (2014-05-09 19:12:39 UTC) #82

LGTM

https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/268643002/diff/880001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.h:140: static void
SaveDefaultSearchProviderToPrefs(const TemplateURL* url,
On 2014/05/09 18:00:22, erikwright wrote:
> On 2014/05/08 22:46:46, Peter Kasting wrote:
> > Bad rebase?  This is the same way the code looks on trunk today, I think.
> > 
> > Also, is this function only called by tests now?  May want to note that if
so.
> 
> > Unless you're just going to delete this in a followup change soon.
> 
> In your CL you made it non-static in order to not save when in a failed state.
> This CL reverts that since it is now only used externally.

Ah, you're right.  Sorry, at one point I locally reverted and then un-reverted
this change, and I forgot I'd unreverted it.

https://codereview.chromium.org/268643002/diff/960001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/268643002/diff/960001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:796: // the effective DSE
changes.
This comment doesn't make it clear what, precisely, is going to get "pushed
back" in the DSM cases, and whether that's equivalent to the call to
ApplyDefaultSearchChange() or not.  Might want to update it.

https://codereview.chromium.org/268643002/diff/960001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:798: if
((default_search_provider_source_ == DefaultSearchManager::FROM_USER) ||
Consider a comment here about what this does differently than the calls below
and why it makes sense to do it in the failure case, unless the updated comment
above covers this.

https://codereview.chromium.org/268643002/diff/960001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service.cc:805:
default_search_manager_.SetUserSelectedDefaultSearchEngine(url->data());
Nit: Indent 2, not 4

erikwright (departed)

The CQ bit was checked by erikwright@chromium.org

6 years, 7 months ago (2014-05-09 19:22:47 UTC) #83

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/268643002/980001

6 years, 7 months ago (2014-05-09 19:26:53 UTC) #84

erikwright (departed)

Committed patchset #55 manually as r269396 (tree was closed).

6 years, 7 months ago (2014-05-09 20:30:34 UTC) #85

Alpha Left Google

A revert of this CL has been created in https://codereview.chromium.org/280113002/ by hclam@chromium.org. The reason for ...

6 years, 7 months ago (2014-05-10 20:37:47 UTC) #86

erikwright (departed)

On 2014/05/10 20:37:47, Alpha wrote: > A revert of this CL has been created in ...

6 years, 7 months ago (2014-05-12 00:54:29 UTC) #87

erikwright (departed)

The CQ bit was checked by erikwright@chromium.org

6 years, 7 months ago (2014-05-12 00:54:50 UTC) #88

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/268643002/980001

6 years, 7 months ago (2014-05-12 00:55:05 UTC) #89

Mark P

As near as I can tell, this change removed the UMA histograms for Search.DefaultSearchProviderType. Was ...

6 years, 7 months ago (2014-05-19 18:05:29 UTC) #91

erikwright (departed)

We are looking into this and will either restore it with identical semantics or propose ...

6 years, 7 months ago (2014-05-20 20:20:14 UTC) #92

Cait (Slow)

6 years, 7 months ago (2014-05-21 19:06:38 UTC) #93

Looks like this was being logged once per Chrome run, just after loading.
https://codereview.chromium.org/295093007/  should restore this behavior.

-Cait

On Tue, May 20, 2014 at 4:20 PM, Erik Wright <erikwright@chromium.org>wrote:

> We are looking into this and will either restore it with identical
> semantics or propose something similar if the new implementation no longer
> offers exactly the same semantics.
>
>
> On Mon, May 19, 2014 at 2:05 PM, <mpearson@chromium.org> wrote:
>
>>
>> As near as I can tell, this change removed the UMA histograms for
>> Search.DefaultSearchProviderType.  Was this intentional?  I object:
>> people,
>> myself included, find it interesting and occasionally useful.
>>
>> --mark
>>
>>
>> https://codereview.chromium.org/268643002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Issue 268643002: Use the DefaultSearchManager as the exclusive authority on DSE, ignoring Web Data. (Closed)