MD History: update (instead of re-create) data sources on sign in change

MD History: update (instead of re-create) data sources on sign in change

BUG=659487
Committed: https://crrev.com/25af85bdf26ac7dbfa4a3ba1b408da2634c94345
Cr-Commit-Position: refs/heads/master@{#437451}

[Dan Beam, 2016-11-03 03:38:04]

Description was changed from

==========
MD History: update (instead of re-create) data sources on sign in change

BUG=659487
R=tsergeant@chromium.org
==========

to

==========
MD History: update (instead of re-create) data sources on sign in change

BUG=659487
==========

[Dan Beam, 2016-11-03 03:38:04]

dbeam@chromium.org changed reviewers:
- tsergeant@chromium.org

[Dan Beam, 2016-11-15 04:58:19]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2016-11-15 04:58:49]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-11-15 05:01:05]

dbeam@chromium.org changed reviewers:
+ calamity@chromium.org

[Dan Beam, 2016-11-15 05:01:05]

+calamity@ for review

/cc tsergeant@ / lshang@ as FYI (feedback welcome!)

useful ideas for testing would also be nice...

[Dan Beam, 2016-11-15 05:01:11]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 05:02:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2016-11-15 05:20:32]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 05:20:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 06:34:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 06:34:54]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-11-16 04:19:20]

ping calamity@

[calamity, 2016-11-16 04:26:50]

On 2016/11/16 04:19:20, Dan Beam wrote:
> ping calamity@

Ack. Looking.

[calamity, 2016-11-16 05:36:13]

Looks sane enough to me, but I'm not too familiar with all these systems.

https://codereview.chromium.org/2475593002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/2475593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:228:
CreateMdHistoryUIHTMLSource(profile, use_test_title_);
Is there a reason we don't disable data source replacement here? Why do we need
a new data source per tab?

https://codereview.chromium.org/2475593002/diff/60001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2475593002/diff/60001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:658: if (it ==
data_sources_.end() || !it->second->IsWebUIDataSourceImpl()) {
Why can't non WebUI data sources be updated?

[Dan Beam, 2016-11-16 06:02:02]

dbeam@chromium.org changed reviewers:
+ creis@chromium.org

[Dan Beam, 2016-11-16 06:02:03]

+creis@ for content/public and general review as well

/cc jam@ and groby@ as FYI

https://codereview.chromium.org/2475593002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/md_history_ui.cc (right):

https://codereview.chromium.org/2475593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/md_history_ui.cc:228:
CreateMdHistoryUIHTMLSource(profile, use_test_title_);
On 2016/11/16 05:36:13, calamity wrote:
> Is there a reason we don't disable data source replacement here? Why do we
need
> a new data source per tab?

so, right now these data sources live in the resource context on the IO thread.

WebUIs and associated code are created (and generally used from) the UI thread.

there's currently no retrieval API (and if there was, it might need to be
async?) so it'd be slightly hard to tell from most WebUI code whether an
existing data source is around (to not replace it).

fwiw: there is a TODO about this -
https://cs.chromium.org/chromium/src/content/public/browser/url_data_source.h...

we could attempt to just merge when Add() is called with an existing source. 
this would allow callers to retain references as the pointers to the objects
would stay the same, but be updated.

https://codereview.chromium.org/2475593002/diff/60001/content/browser/webui/u...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/2475593002/diff/60001/content/browser/webui/u...
content/browser/webui/url_data_manager_backend.cc:658: if (it ==
data_sources_.end() || !it->second->IsWebUIDataSourceImpl()) {
On 2016/11/16 05:36:13, calamity wrote:
> Why can't non WebUI data sources be updated?

we're only sending updates to the localized "strings".  other URLDataSources
don't have that, only WebUIDataSources.

[calamity, 2016-11-16 06:43:20]

lgtm

[Dan Beam, 2016-11-16 06:54:06]

just to note some things calamity@ and I talked about over chat:

1) all of this code seems to be in need of some touch ups.  create a whole data
source just to throw it away (when replacements are disabled)?  that doesn't
make much sense to me.

2) staleness or data races seems to be a fairly pervasive issue.  right now
re-creating data sources potentially papers over the instances in which an input
to a dynamic value has changed.  for data sources that combine dynamic and
static stuff, turning off replacements may result in staleness problems.

3) we probably shouldn't be combining static and dynamic resources, because
re-doing any work for static resources is unnecessary, and NOT correctly
recomputing dynamic resources on data is also a bummer.

other approaches that were [kind of] considered:

a) make Add() + Add() merge more recent values into an existing data source,
allows callers to retain weak references safely (but changes semantics and
potentially encourages updates on UI thread, which may not be cool)

b) disable replacements for this data source and figure out how to do discrete
updates, e.g. recomputing whether a user is signed in.  this is similar to the
approach implemented currently, but without the safety net of recreating.

[Charlie Reis, 2016-11-16 18:38:45]

I'm not familiar with data sources, so I can only offer a rubber stamp to
calamity's approval.  That said, content/ LGTM with nits.

Please expand the CL description-- it's not clear what the problem was.  Is this
just a more efficient way to do what we did before, or does it affect
correctness?

https://codereview.chromium.org/2475593002/diff/60001/content/public/browser/...
File content/public/browser/web_ui_data_source.h (right):

https://codereview.chromium.org/2475593002/diff/60001/content/public/browser/...
content/public/browser/web_ui_data_source.h:9: 
nit: No blank line?

https://codereview.chromium.org/2475593002/diff/60001/content/public/browser/...
content/public/browser/web_ui_data_source.h:39: CONTENT_EXPORT static void
Update(
Please include comments for all methods in the public interface.  (It's probably
worth also explaining why some cases should use Update rather than Add, given
the bug it's fixing.)

[Dan Beam, 2016-11-19 02:44:27]

i'm not super happy with how this is implemented, and it'll be a blueprint for
others.

i'm thinking maybe there's a simpler way to merge a newer data source with an
existing one when Add() is called again.  i don't really like how this works all
too well.

[Dan Beam, 2016-12-09 02:11:23]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 02:11:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2016-12-09 02:12:18]

alright, i've shopped this by a bunch more people and nobody's given me any
other amazingly good ideas (including some sustained thought from myself). 
let's give it a go

[Dan Beam, 2016-12-09 02:12:26]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2016-12-09 02:12:31]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-12-09 02:13:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 03:34:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481249551307060,
"parent_rev": "54ebee9ceccbbacbc25ddf8c05020f2d1116dfa0", "commit_rev":
"cd97e055f062ac4157357a7911331aab231b0207"}

[commit-bot: I haz the power, 2016-12-09 03:35:10]

Description was changed from

==========
MD History: update (instead of re-create) data sources on sign in change

BUG=659487
==========

to

==========
MD History: update (instead of re-create) data sources on sign in change

BUG=659487
==========

[commit-bot: I haz the power, 2016-12-09 03:35:11]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-12-09 03:37:34]

Description was changed from

==========
MD History: update (instead of re-create) data sources on sign in change

BUG=659487
==========

to

==========
MD History: update (instead of re-create) data sources on sign in change

BUG=659487

Committed: https://crrev.com/25af85bdf26ac7dbfa4a3ba1b408da2634c94345
Cr-Commit-Position: refs/heads/master@{#437451}
==========

[commit-bot: I haz the power, 2016-12-09 03:37:35]

Patchset 2 (id:??) landed as
https://crrev.com/25af85bdf26ac7dbfa4a3ba1b408da2634c94345
Cr-Commit-Position: refs/heads/master@{#437451}

[Dan Beam, 2016-12-13 01:52:41]

Patchset #3 (id:80001) has been deleted