Closed Bug 1468754 Opened Last year Closed 11 months ago

Track Changes - construct an Actor to hold tracked changes

Categories

(DevTools :: Inspector, enhancement, P2)

enhancement

Tracking

(firefox65 fixed)

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 15 obsolete files)

46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
Export Changes primary artifact will be a "punch list" of changes made with devtools. The intent is to capture enough detail that each change could be reported and applied separately (eventually supporting undo/redo) and could be processed for re-importing or exported in a human-readable form.

This bug intends to cover the data storage definition of the punch list. Other bugs cover the generation of the data.
Summary: Build punch list data structure → Export Changes - build punch list data structure
Assignee: nobody → bwerth
Summary: Export Changes - build punch list data structure → Track Changes - build punch list data structure
As I understand the problem better, I'm refining the title of the bug.
Summary: Track Changes - build punch list data structure → Track Changes - construct an Actor to hold tracked changes
Severity: normal → enhancement
Priority: -- → P2
Component: General → Inspector
Attachment #8986923 - Flags: review?(gl)
Attachment #8986924 - Flags: review?(gl)
Attachment #8986925 - Flags: review?(gl)
Comment on attachment 8986923 [details]
Bug 1468754 Part 1: Add a ChangesActor to devtools.

https://reviewboard.mozilla.org/r/252154/#review258662

::: devtools/server/actors/changes.js:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const protocol = require("devtools/shared/protocol");
> +

Remove this line

::: devtools/server/actors/changes.js:9
(Diff revision 2)
> +
> +"use strict";
> +
> +const protocol = require("devtools/shared/protocol");
> +
> +const {changesSpec} = require("devtools/shared/specs/changes");

s/{changesSpec}/{ changesSpec }

::: devtools/server/actors/changes.js:12
(Diff revision 2)
> +const protocol = require("devtools/shared/protocol");
> +
> +const {changesSpec} = require("devtools/shared/specs/changes");
> +
> +/**
> + * The ChangesActor stores a stack of changes made by devtools on

made by the inspector

::: devtools/server/actors/changes.js:15
(Diff revision 2)
> +
> +/**
> + * The ChangesActor stores a stack of changes made by devtools on
> + * the document in the associated tab.
> + */
> +var ChangesActor = protocol.ActorClassWithSpec(changesSpec, {

s/var/const

::: devtools/server/actors/changes.js:19
(Diff revision 2)
> + */
> +var ChangesActor = protocol.ActorClassWithSpec(changesSpec, {
> +  /**
> +   * Create a ChangesActor.
> +   *
> +   * @param inspector

@param  {InspectorActor} inspector

::: devtools/server/actors/changes.js:19
(Diff revision 2)
> + */
> +var ChangesActor = protocol.ActorClassWithSpec(changesSpec, {
> +  /**
> +   * Create a ChangesActor.
> +   *
> +   * @param inspector

I can see this is using some old actors as a template. You can just see how we format some of the JSDocs like the @param and code in https://searchfox.org/mozilla-central/source/devtools/server/actors/layout.js.

::: devtools/server/actors/changes.js:21
(Diff revision 2)
> +  /**
> +   * Create a ChangesActor.
> +   *
> +   * @param inspector
> +   *    The InspectorActor that owns this ChangesActor.
> +   *

Remove this line

::: devtools/server/actors/changes.js:22
(Diff revision 2)
> +   * Create a ChangesActor.
> +   *
> +   * @param inspector
> +   *    The InspectorActor that owns this ChangesActor.
> +   *
> +   * @constructor

Remove this line. We don't really do @constructor these days. I think this was an ancient pattern.

::: devtools/server/actors/changes.js:41
(Diff revision 2)
> +  },
> +
> +  form: function(detail) {
> +    if (detail === "actorid") {
> +      return this.actorID;
> +    }

Add a new line after this if statement block.

::: devtools/shared/fronts/changes.js:4
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";

Add a new line before "use strict"

::: devtools/shared/fronts/changes.js:21
(Diff revision 2)
> + * ChangesFront, the front object for the ChangesActor
> + */
> +const ChangesFront = FrontClassWithSpec(changesSpec, {
> +  initialize: function(conn, form, ctx, detail) {
> +    Front.prototype.initialize.call(this, conn, form, ctx, detail);
> +    this.inspector = this.parent();

We can actually remove the initialize() function if we don't need this.inspector, which currently is unused.

::: devtools/shared/fronts/changes.js:32
(Diff revision 2)
> +      return;
> +    }
> +    this._form = form;
> +  },
> +
> +  destroy: function() {

We can also remove this since we aren't doing anything extra with this destroy method. See https://searchfox.org/mozilla-central/source/devtools/shared/fronts/layout.js as an example.

::: devtools/shared/specs/changes.js:4
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";

Add a new line before "use strict"

::: devtools/shared/specs/changes.js:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const {
> +  Arg,
> +  Option,

You are gonna get eslint errors if you leave this in because it's not being used.

::: devtools/shared/specs/changes.js:12
(Diff revision 2)
> +  Arg,
> +  Option,
> +  RetVal,
> +  generateActorSpec,
> +  types
> +} = require("devtools/shared/protocol");

I prefer to see the list alphabeticalized.

::: devtools/shared/specs/changes.js:18
(Diff revision 2)
> +
> +const changesSpec = generateActorSpec({
> +  typeName: "changes",
> +
> +  methods: {
> +    getChangeCount: {

Would prefer to see the list of methods alphabetically sorted.

::: devtools/shared/specs/changes.js:21
(Diff revision 2)
> +
> +  methods: {
> +    getChangeCount: {
> +      request: {},
> +      response: { count: RetVal("number") }
> +    },

I prefer to see a new line after every method to separate every method.
Attachment #8986923 - Flags: review?(gl)
Comment on attachment 8986923 [details]
Bug 1468754 Part 1: Add a ChangesActor to devtools.

https://reviewboard.mozilla.org/r/252154/#review258764

::: devtools/server/actors/changes.js:47
(Diff revision 2)
> +    return {
> +      actor: this.actorID,
> +    };
> +  },
> +
> +  getChangeCount: function() {

Thinking more about this. I don't think it's a good idea to land empty functions like these until they are fully implemented. 

I am not convinced that we should be tracking the changes in the server so I would like to see more of the code fleshed out before reviewing further instead of landing a skeleton actor, which might not be used in the future. I would prefer to wait for more patches with a semi-working prototype before proceeding.
I do find the methods a bit weird as well. Typically, we would rather do getChanges() and get an array of all the changes instead of just asking for a single at a given index. The server is typically for trying to query something from platform or the content page, which you wouldn't otherwise be able to do in the client. I am imagining you are trying to hook into the PageStyleActor with this solution.

It is still perhaps too early to tell what the use case of the actor will be, since I can also imagine a client side solution. So, I would prefer to do a batch feedback/review of all the patches that shows a working prototype.
Comment on attachment 8986924 [details]
Bug 1468754 Part 2: Make the ChangesActor be managed by the InspectorActor.

Clearing reviews for now
Attachment #8986924 - Flags: review?(gl)
Attachment #8986925 - Flags: review?(gl)
Comment on attachment 8986923 [details]
Bug 1468754 Part 1: Add a ChangesActor to devtools.

https://reviewboard.mozilla.org/r/252154/#review258662

> made by the inspector

I did not make this change, since changes may also come from the Style Editor. As devtools evolves, there may be other ways to modify the document.

> Would prefer to see the list of methods alphabetically sorted.

I'm not making this change. That sort of change would put all "get" methods together separate from their corresponding "set" methods (which don't exist today, but it's a common pattern), etc. There seems very little value in doing this.
Comment on attachment 8986923 [details]
Bug 1468754 Part 1: Add a ChangesActor to devtools.

https://reviewboard.mozilla.org/r/252154/#review258764

> Thinking more about this. I don't think it's a good idea to land empty functions like these until they are fully implemented. 
> 
> I am not convinced that we should be tracking the changes in the server so I would like to see more of the code fleshed out before reviewing further instead of landing a skeleton actor, which might not be used in the future. I would prefer to wait for more patches with a semi-working prototype before proceeding.

That's fine. I'll build the patch stack for the next part (Bug 1468758) on top of this locally, to prove out the concept.
(In reply to Gabriel [:gl] (ΦωΦ) from comment #10)
> I do find the methods a bit weird as well. Typically, we would rather do
> getChanges() and get an array of all the changes instead of just asking for
> a single at a given index. The server is typically for trying to query
> something from platform or the content page, which you wouldn't otherwise be
> able to do in the client. I am imagining you are trying to hook into the
> PageStyleActor with this solution.

Yes, returning an array of all changes would probably be better, and would reduce server/client traffic. I'll make that change now. Since I'll build the first implementation on top of this before landing it, I'll sort out the practicality issues as I go.

> It is still perhaps too early to tell what the use case of the actor will
> be, since I can also imagine a client side solution. So, I would prefer to
> do a batch feedback/review of all the patches that shows a working prototype.

Yes, precisely. With my limited understanding of the behavior of the server/client distinction in devtools, this was my effort to take advantage of the server-side setRuleText method at https://searchfox.org/mozilla-central/source/devtools/server/actors/styles.js#1298. If there's a good place to hook this in to client, I'd appreciate a pointer to where that should go. That might be the right move since it would probably help us with changes surviving a page reload (which is a planned feature). Can you advise how to build this solution on the client?
Flags: needinfo?(gl)
(In reply to Brad Werth [:bradwerth] from comment #14)
> https://searchfox.org/mozilla-central/source/devtools/server/actors/styles.
> If there's a good place to hook this in to client, I'd appreciate a
> pointer to where that should go. That might be the right move since it would
> probably help us with changes surviving a page reload (which is a planned
> feature). Can you advise how to build this solution on the client?

Replying to myself... looks like what I need to do is hook into RuleRewriter. I could still use an example of how to store and manage data on the client.
Though this will likely be re-done as client-side storage, I've pushed the updated server-side patches with review changes complete.
I am gonna hold off reviews for now. I think the best way to go forward about this is to start a doc with some architectural proposals that we can share with the team. jryans would be a good reviewer for any server protocol proposals. pbro, rcaliman, tromey and myself would be good reviewers for general knowledge around the inspector. 

It is possible that the best way of knowing which way to go forward is to implement a prototype server and client side implementations so we can evaluate the merits of each approach. I haven't looked at the style editor myself for awhile now so I will need to re-familiarize myself with that bit.
Flags: needinfo?(gl)
Attachment #8986923 - Flags: review?(gl)
Attachment #8986924 - Flags: review?(gl)
Attachment #8986925 - Flags: review?(gl)
Alright, I'm pursuing the high-level architectural discussion. On the assumption that server-side is still in consideration, I'll keep this bug as the server-side proof of concept, and push this a bit further.
(In reply to Brad Werth [:bradwerth] from comment #21)
> Alright, I'm pursuing the high-level architectural discussion. On the
> assumption that server-side is still in consideration, I'll keep this bug as
> the server-side proof of concept, and push this a bit further.

Yes, I think both approaches are up for consideration. It is possible one approach will be better and easier than the other, but I would say at this stage it is an architectural exploration before committing to server vs client.
See Also: → 1476366
See Also: 1476366
The uploaded files have only server-side methods and tests -- there's no UI, and no testing that the devtools UI will trigger detection of changes.

It does look like a server-side approach will make it fairly simple to add DOM attribute change tracking by adding hooks in NodeActor.modifyAttributes[1].

[1]: https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/node.js#616
Comment on attachment 8986923 [details]
Bug 1468754 Part 1: Add a ChangesActor to devtools.

https://reviewboard.mozilla.org/r/252152/#review266040
Comment on attachment 8986923 [details]
Bug 1468754 Part 1: Add a ChangesActor to devtools.

https://reviewboard.mozilla.org/r/252154/#review266038

::: devtools/server/actors/changes.js:54
(Diff revision 4)
> +  },
> +
> +  change: function(index) {
> +    if (index >= 0 && index < this.changes.length) {
> +      // Return a copy of the change at index.
> +      return Object.assign({}, this.changes[index]);

This only does a shallow copy of the object. The first level of key/values will be copied, but the nested objects (stylesheetChange, attributeChange) will be passed by reference and will therefore be succeptible to  mutations.
Comment on attachment 8986923 [details]
Bug 1468754 Part 1: Add a ChangesActor to devtools.

https://reviewboard.mozilla.org/r/252154/#review266038

> This only does a shallow copy of the object. The first level of key/values will be copied, but the nested objects (stylesheetChange, attributeChange) will be passed by reference and will therefore be succeptible to  mutations.

Good point. For similar reasons, the changes() method is also affected by this issue. I'll resolve this either by flattening the change structure, or by implementing a deep copy.
MozReview-Commit-ID: 1Y8esljnLk9
MozReview-Commit-ID: D71oBN8MSgU

Depends on D4399
Instead of firing events to the client, the StyleRuleActor now sends
changes to the ChangesActor, which will arrange them into a stack of
changes for undo/redo and for coalescing (to be implemented).

Depends on D4400
Now that StyleRuleActor no longer fires the events, ChangesActor fires them.

Depends on D4401
Attachment #8986923 - Attachment is obsolete: true
Attachment #8986924 - Attachment is obsolete: true
Attachment #8993526 - Attachment is obsolete: true
Attachment #8994985 - Attachment is obsolete: true
Attachment #8994986 - Attachment is obsolete: true
Attachment #8986925 - Attachment is obsolete: true
Attachment #8993527 - Attachment is obsolete: true
Attachment #8997236 - Attachment is obsolete: true
Attachment #8997237 - Attachment is obsolete: true
Attachment #9004406 - Attachment is obsolete: true
Attachment #9004402 - Attachment is obsolete: true
My understanding is that instantiating a Front should trigger instantiation
of the corresponding actor. If that's not the case, then surely the Actor
will be created upon the first call of a method of the Front. That is not
working here and I'm posting this in the hope of getting some feedback on
why it is not working.

Depends on D9050
Attachment #9018093 - Attachment is obsolete: true
Attachment #9014193 - Attachment is obsolete: true
Attachment #9004409 - Attachment is obsolete: true
Attachment #9004410 - Attachment is obsolete: true
Attachment #9014193 - Attachment is obsolete: false
Attachment #9014193 - Attachment is obsolete: true
Attachment #9018092 - Attachment is obsolete: true
Attachment #9018092 - Attachment is obsolete: false
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/817018fb5aa4
Part 1: Add a ChangesActor to devtools. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e6ff3400a4f6
Part 2: Make client-side Inspector, on open, ensure the existence of the ChangesActor. r=pbro
https://hg.mozilla.org/integration/autoland/rev/ddd043a258e0
Part 3: Add a TrackChangesEmitter helper object, and make ChangesActor listen to it. r=pbro
https://hg.mozilla.org/integration/autoland/rev/c98ac7503111
Part 4: Make ChangesActor fire events. r=pbro
https://hg.mozilla.org/integration/autoland/rev/c36e8383aa3a
Part 5: Make ChangesView respond to change events. r=rcaliman
Backed out 5 changesets (bug 1468754) for linting failure at checkouts/gecko/devtools/server/actors/utils/actor-registry.js on a CLOSED TREE

Backout link:  https://hg.mozilla.org/integration/autoland/rev/7f6a5b37d2ca21ea84379c6edf675f15b99d4e5e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&searchStr=linting%2Copt%2Csource-test-mozlint-eslint%2C%28es%29&revision=c36e8383aa3ac580f99c69011eebca5fce6b0830

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=207022554&repo=autoland&lineNumber=276

Log snippet: [task 2018-10-22T16:45:53.177Z] building 'psutil._psutil_posix' extension
[task 2018-10-22T16:45:53.177Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-10-22T16:45:53.177Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-10-22T16:45:53.177Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-10-22T16:45:53.177Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-10-22T16:45:53.177Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-10-22T16:45:53.177Z] 
[task 2018-10-22T16:45:53.178Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-10-22T16:50:44.138Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/server/actors/utils/actor-registry.js:262:29 | Missing trailing comma. (comma-dangle)
[task 2018-10-22T16:50:44.138Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/shared/fronts/changes.js:14:23 | Unexpected space before function parentheses. (space-before-function-paren)
Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71f45c79522e
Part 1: Add a ChangesActor to devtools. r=pbro
https://hg.mozilla.org/integration/autoland/rev/da447a45603e
Part 2: Make client-side Inspector, on open, ensure the existence of the ChangesActor. r=pbro
https://hg.mozilla.org/integration/autoland/rev/0d44a0523525
Part 3: Add a TrackChangesEmitter helper object, and make ChangesActor listen to it. r=pbro
https://hg.mozilla.org/integration/autoland/rev/55ba74fa5c95
Part 4: Make ChangesActor fire events. r=pbro
https://hg.mozilla.org/integration/autoland/rev/67f969f5bdba
Part 5: Make ChangesView respond to change events. r=rcaliman
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a9f1d46b0a9
Part 1: Add a ChangesActor to devtools. r=pbro
https://hg.mozilla.org/integration/autoland/rev/17e7f300cc64
Part 2: Make client-side Inspector, on open, ensure the existence of the ChangesActor. r=pbro
https://hg.mozilla.org/integration/autoland/rev/a1bc85d3577f
Part 3: Add a TrackChangesEmitter helper object, and make ChangesActor listen to it. r=pbro
https://hg.mozilla.org/integration/autoland/rev/2919bc5cd4c6
Part 4: Make ChangesActor fire events. r=pbro
https://hg.mozilla.org/integration/autoland/rev/194a4baf87a5
Part 5: Make ChangesView respond to change events. r=rcaliman
Flags: needinfo?(bwerth)
You need to log in before you can comment on or make changes to this bug.