Track Changes - replace custom hash with actorID for identifying rule and source
Categories
(DevTools :: Inspector: Changes, enhancement, P3)
Tracking
(firefox67 fixed)
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: rcaliman, Assigned: rcaliman)
References
Details
Attachments
(1 file)
Replaces the custom getSourceHash() and getRuleHash() for uniquely identifying CSS rules and stylesheets with the rule and stylesheet actor IDs. These are stable throughout the session.
This saves on needless string-building work and helps avoid excessively long ids which will soon be used in the DOM for marking the elements for rendered rules and stylesheets.
|
Assignee | |
Comment 1•5 years ago
|
||
Replaces custom generated hashes with the actorIDs which are stable
during the editing session enough to use as unique identifiers.
For future restore / persistence, we still have the metadata about each
rule and stylesheet to attempt to identify them again.
Pushed by rcaliman@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/89ba5f983e61 Use actorID for tracked rules and stylesheets. r=pbro
|
||
Comment 3•5 years ago
|
||
Backed out changeset 89ba5f983e61 (bug 1525238) for dt failures at browser_changes_rule_selector.js and browser_inspector_highlighter-eyedropper-xul.js
Backout: https://hg.mozilla.org/integration/autoland/rev/5c37f9a1114c1de97b8aa20719ddd46d4e471f86
Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&selectedJob=226454332&revision=89ba5f983e613a1aea267c72b21915115782dbd9
Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226454332&repo=autoland&lineNumber=4865
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226454313&repo=autoland&lineNumber=5001
|
|
Assignee | |
Comment 4•5 years ago
|
||
Upon closer inspection of the test failures it occurred that refactoring to use the actorID is not feasible at this time. I will shelve this patch for later. The fix to get this through involves a substantial structural change for little benefit. Leaving the code untouched doesn't impede other work from happening.
The important failing test is for tracking renamed selectors. (The other seems to be XUL-related)
Context:
When using a hashing function which includes the old or new selector, the reducer which aggregates changes treats the operation as happening on two rules: one whole rule removal (which uses the old selector), followed by a whole rule addition (which uses the new selector).
When using the actorID as an identifier instead of the custom hash, the rule identifier stays the same over those two operations. To the reducer, it looks like the contents of a single rule are changing: all declarations removed + all declarations added. The selector change is not tracked atomically.
This triggers another part of the reducer meant to improve the UX: changes which cancel each other out get cleared from the store. The consequence is that selector changes no longer show up in the Changes panel. This is bad.
Tracking the selector changes atomically (instead of whole rule removal / whole rule addition) involves structural changes to the Redux store and custom handling in the Changes panel. This work seems unnecessary at this stage. Using the actorID instead of the hashing functions is meant to be an optimization. The code works fine as is.
Since both the hashing methods and the actorID identifiers need to stay in for at least 2-3 releases to account for backwards compatibility with old servers, the additional structural changes required to get this patch to land look unfeasible. We'll revisit this issue when/if restructuring the Redux store for more meaningful gains.
|
|
Assignee | |
Comment 5•5 years ago
|
||
The eternity that is usually "we'll revisit this later" was shorter than anticipated. :)
Having access to the rule's actorID became a requirement for Bug 1524547. We need it in order to identify a StyleRuleActor to get the full authored text of its rule for the "Copy Rule" action of the Changes panel.
I'm reopening this bug and getting back to work.
The issues blocking this patch are addressed by:
- updating the patch to account for nonexistent NodeActors for XUL nodes (failing test).
- Bug 1527924: refactoring the tracking of changes to selectors (failing test)
|
|
Assignee | |
Comment 6•5 years ago
|
||
Green try with latest fix for XUL test and disabled test for selector rename (to be re-enabled by Bug 1527924):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf84b00053dcba3e12619a6db1f0f7e6c815103
|
|
Assignee | |
Updated•5 years ago
|
Pushed by rcaliman@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eadeff4ae694 Use actorID for tracked rules and stylesheets. r=pbro
|
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
Description
•