Apply object updates from the network without blowing away object identity or UI attributes.

Apply object updates from the network without blowing away object identity or UI attributes.

After this change, a data update will no longer collapse any expanded sections.

BUG=408364
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181405

[Jeffrey Yasskin, 2014-08-30 01:55:16]

jyasskin@chromium.org changed reviewers:
+ ojan@chromium.org

[Jeffrey Yasskin, 2014-08-30 01:55:16]

I'm missing a test checking that updates preserve the 'expanded' state, and I
may have broken the ct-test-list.html:testsChanged handler since the tests
object doesn't really change anymore. When that's fixed, you'll probably need to
add another uiStateProperties to mark the next 'expanded' property as not
needing updates.

[Jeffrey Yasskin, 2014-08-30 03:07:24]

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
File Tools/GardeningServer/lib/util.html (right):

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:18: // If they're arrays, true if the
elements have a |fingerprint| property.
Other possible names for the |fingerprint| property are |identityKey|,
|identity|, |key|. I didn't pick |hash| because these must not collide.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:47: util.updateLeft = function(target,
source)
Other names for |updateLeft| include |assign|, |inplaceAssign|,
|updatePreservingIdentity|.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:101: var uiStateProperties =
target.constructor.uiStateProperties;
|uiStateProperties| could be |transientProperties| or |ephemeralProperties|.
"transient" matches Java in that these aren't propagated from the
deserialization.

[ojan, 2014-09-02 03:48:11]

ojan@chromium.org changed reviewers:
+ esprehn@chromium.org

[ojan, 2014-09-02 04:08:03]

esprehn, mind taking a quick glance at the approach this patch takes? I expect
you'll have strong opinions on the right way to do this. Alternately, you'll
like this approach and we should consider putting it somewhere reusable
eventually.

lgtm. I'm not too fussed about the names. Just suggested the ones I had a slight
leaning towards.

On 2014/08/30 at 01:55:16, jyasskin wrote:
> I'm missing a test checking that updates preserve the 'expanded' state

It's a bummer not to have any tests of this since that's the user-visible
behavior the patch is fixing. Mind taking a stab at it?

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
File Tools/GardeningServer/lib/util.html (right):

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:11: var util = util || {};
I have an allergy to generic util namespaces. They become too much of a dumping
ground for random junk. It's a bit redundant, but how about calling this
updateUtil?

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:18: // If they're arrays, true if the
elements have a |fingerprint| property.
On 2014/08/30 at 03:07:24, Jeffrey Yasskin wrote:
> Other possible names for the |fingerprint| property are |identityKey|,
|identity|, |key|. I didn't pick |hash| because these must not collide.

I probalby would go with key, but fingerprint is fine with me too.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:21: util.canUpdateLeft = function(target,
source) {
This is only used in side this file, so no need to expose it on the public
interface, right?

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:47: util.updateLeft = function(target,
source)
On 2014/08/30 at 03:07:24, Jeffrey Yasskin wrote:
> Other names for |updateLeft| include |assign|, |inplaceAssign|,
|updatePreservingIdentity|.

updateLeft sgtm

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:67: function indicesByFingerprint(array) {
Since this isn't using any closed variables, it could be defined outside of
updateLeft. Would make it a bit easier to read this function.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:76: var sourceIndices =
indicesByFingerprint(source);
This would be a bit easier to read if you move the contents of the if and the
else blocks here to helper functions.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:101: var uiStateProperties =
target.constructor.uiStateProperties;
On 2014/08/30 at 03:07:24, Jeffrey Yasskin wrote:
> |uiStateProperties| could be |transientProperties| or |ephemeralProperties|.
"transient" matches Java in that these aren't propagated from the
deserialization.

transient sgtm.

[esprehn, 2014-09-02 06:09:45]

Sure I'll look tomorrow.

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

[Jeffrey Yasskin, 2014-09-02 21:45:51]

I'm working on the test now.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
File Tools/GardeningServer/lib/util.html (right):

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:11: var util = util || {};
On 2014/09/02 04:08:02, ojan-only-code-yellow-reviews wrote:
> I have an allergy to generic util namespaces. They become too much of a
dumping
> ground for random junk. It's a bit redundant, but how about calling this
> updateUtil?

Done.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:18: // If they're arrays, true if the
elements have a |fingerprint| property.
On 2014/09/02 04:08:02, ojan-only-code-yellow-reviews wrote:
> On 2014/08/30 at 03:07:24, Jeffrey Yasskin wrote:
> > Other possible names for the |fingerprint| property are |identityKey|,
> |identity|, |key|. I didn't pick |hash| because these must not collide.
> 
> I probalby would go with key, but fingerprint is fine with me too.

Done.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:21: util.canUpdateLeft = function(target,
source) {
On 2014/09/02 04:08:02, ojan-only-code-yellow-reviews wrote:
> This is only used in side this file, so no need to expose it on the public
> interface, right?

Right, although it made it a little easier to describe updateLeft. Done.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:67: function indicesByFingerprint(array) {
On 2014/09/02 04:08:02, ojan-only-code-yellow-reviews wrote:
> Since this isn't using any closed variables, it could be defined outside of
> updateLeft. Would make it a bit easier to read this function.

Done.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:76: var sourceIndices =
indicesByFingerprint(source);
On 2014/09/02 04:08:02, ojan-only-code-yellow-reviews wrote:
> This would be a bit easier to read if you move the contents of the if and the
> else blocks here to helper functions.

Done. I decided to pull these all the way out of updateLeft(), which pulled
updateByIndices() out too. It is easier to read now.

https://codereview.chromium.org/526633002/diff/20001/Tools/GardeningServer/li...
Tools/GardeningServer/lib/util.html:101: var uiStateProperties =
target.constructor.uiStateProperties;
On 2014/09/02 04:08:02, ojan-only-code-yellow-reviews wrote:
> On 2014/08/30 at 03:07:24, Jeffrey Yasskin wrote:
> > |uiStateProperties| could be |transientProperties| or |ephemeralProperties|.
> "transient" matches Java in that these aren't propagated from the
> deserialization.
> 
> transient sgtm.

Done.

[esprehn, 2014-09-02 23:00:28]

This seems really complicated, I don't think you want to be creating objects and
then comparing them like this. Instead I would suggest splitting the data
parsing and the object creation code, for example the code review tool does:

function Thing(id) {
  this.id = id;
  this.prop = default;
}

Thing.prototype.parseData = function(data) {
  this.prop = data.prop;
};

where parseData takes the JSON from the server and merges the data into the
object. Then when you want to refresh the data you refetch from the server and
call parseData again, which won't overwrite the transient properties and
correctly handles computed and merged values. This makes the fetch logic deal
with the comparisons as well:

var self = this;
this.things = serverData.map(function(data) {
  var o = self.thingsById[data.id] || new Thing(data.id);
  o.parseData(data);
  return o;
});

Having a generic "compare these things" system using all the slow v8 reflection
stuff doesn't seem good.

[esprehn, 2014-09-03 00:44:01]

In talking about this with Ojan we can go ahead and try this and see how it
goes. Maybe this is better than what I suggested, it's not clear the right way
to do this, we really need to get something built into TemplateBinding itself.

[Jeffrey Yasskin, 2014-09-03 00:50:43]

I think it's not going to be too hard to see what your suggestion
looks like, so I'll try that. If it does turn out to be a lot of work
I might give up.

On Tue, Sep 2, 2014 at 5:44 PM,  <esprehn@chromium.org> wrote:
> In talking about this with Ojan we can go ahead and try this and see how it
> goes. Maybe this is better than what I suggested, it's not clear the right
> way
> to do this, we really need to get something built into TemplateBinding
> itself.
>
> https://codereview.chromium.org/526633002/

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

[esprehn, 2014-09-03 06:18:57]

On 2014/09/03 at 00:50:43, jyasskin wrote:
> I think it's not going to be too hard to see what your suggestion
> looks like, so I'll try that. If it does turn out to be a lot of work
> I might give up.
> 

In talking with Ojan I think the big confusion here is that template binding
won't destroy objects that are stable, even if the array changes, so you don't
need all that complexity in updateUtil to add and remove things or do no-op
merges or any of this, just rebuild the entire array every time.

Specifically that whole library can just be:

function updateList(oldObjects, dataList, makeKey, makeObject) {
  var map = {};
  oldObjects.forEach(function(object) {
    map[object.key] = object;
  });
  return dataList.map(function(data) {
    var key = makeKey(data);
    var object = map[key] || makeObject(key);
    object.parseData(data);
    return object;
  });
}

used like:

model.objects = updateList(model.objects, dataList, function(data) {
  return data.foo;
}, function(key) {
  return new Thing(key);
});

Also don't use hasOwnProperty() like that, it's just making your code slow. I
also don't understand why you need generic object (not array) merging, that
shouldn't be needed.

[Jeffrey Yasskin, 2014-09-04 00:30:29]

Patchset #5 (id:100001) has been deleted

[Jeffrey Yasskin, 2014-09-04 00:38:28]

I've rebased this on top of https://codereview.chromium.org/536163002/, and I
think everything's done except the rewrite Elliott asked for.

On 2014/09/03 06:18:57, esprehn wrote:
> On 2014/09/03 at 00:50:43, jyasskin wrote:
> > I think it's not going to be too hard to see what your suggestion
> > looks like, so I'll try that. If it does turn out to be a lot of work
> > I might give up.
> > 
> 
> In talking with Ojan I think the big confusion here is that template binding
> won't destroy objects that are stable, even if the array changes, so you don't
> need all that complexity in updateUtil to add and remove things or do no-op
> merges or any of this, just rebuild the entire array every time.

Yep, I hadn't realized this about Polymer. I double-checked with
http://jsbin.com/moniyanoxani/4/edit.

> Specifically that whole library can just be:
> 
> function updateList(oldObjects, dataList, makeKey, makeObject) {
>   var map = {};
>   oldObjects.forEach(function(object) {
>     map[object.key] = object;
>   });
>   return dataList.map(function(data) {
>     var key = makeKey(data);
>     var object = map[key] || makeObject(key);
>     object.parseData(data);

This parseData() function doesn't currently exist. All the parsing happens in
ct-failures.html rather than having each class know how to parse its fraction of
the data.

I'm going to try spreading the parsing across the codebase instead of writing
this generic object-updating library, to see if that winds up simpler.

>     return object;
>   });
> }
> 
> used like:
> 
> model.objects = updateList(model.objects, dataList, function(data) {
>   return data.foo;
> }, function(key) {
>   return new Thing(key);
> });
> 
> Also don't use hasOwnProperty() like that, it's just making your code slow.

That is, assume that {} works well as a map, by checking whether [] returns
undefined?

I'm still using hasOwnProperty in the generic object updating code because I
don't think I want to merge inherited properties.

> I
> also don't understand why you need generic object (not array) merging, that
> shouldn't be needed.

[ojan, 2014-09-04 17:58:16]

This lgtm.

I'm thinking we should just go ahead and land this so we can get the critical
bug fixed. We can evaluate elliott's rewrite async from that. Most of these
changes will be needed for the rewrite version anyways. jyasskin, WDYT?

[Jeffrey Yasskin, 2014-09-04 18:14:12]

On 2014/09/04 17:58:16, ojan-only-code-yellow-reviews wrote:
> This lgtm.
> 
> I'm thinking we should just go ahead and land this so we can get the critical
> bug fixed. We can evaluate elliott's rewrite async from that. Most of these
> changes will be needed for the rewrite version anyways. jyasskin, WDYT?

SGTM. This does need to wait for https://codereview.chromium.org/536163002/.

[Jeffrey Yasskin, 2014-09-04 21:01:56]

On 2014/09/04 18:14:12, Jeffrey Yasskin wrote:
> On 2014/09/04 17:58:16, ojan-only-code-yellow-reviews wrote:
> > This lgtm.
> > 
> > I'm thinking we should just go ahead and land this so we can get the
critical
> > bug fixed. We can evaluate elliott's rewrite async from that. Most of these
> > changes will be needed for the rewrite version anyways. jyasskin, WDYT?
> 
> SGTM. This does need to wait for https://codereview.chromium.org/536163002/.

I've updated the documentation a bit and simplified the implementation using the
fact that updateLeft now always requires the caller to assign the target. I'll
check the commit box once the NetworkSimulator patch makes it through the CQ. (I
could NOTRY it, but I wanted to see if the CQ would notice it didn't need to do
anything. It didn't so notice.)

[Jeffrey Yasskin, 2014-09-04 22:02:55]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:03:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/526633002/140001

[commit-bot: I haz the power, 2014-09-04 22:05:13]

Committed patchset #6 (id:140001) as 181405