Closed Bug 1309065 Opened 8 years ago Closed 7 years ago

./mach devtools-css-db depends on the file that it's attempting to generate (and as a result, it doesn't do the right thing after a property's been renamed)

Categories

(DevTools :: Framework, defect, P1)

Product:
DevTools
Component:
Framework
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: dholbert, Assigned: gregtatum)

References

Details

Attachments

(1 file)

Bug 1309065 - Be smarter about filling in the client-side css properties db;
58 bytes, text/x-review-board-request
tromey
: review+
Details 
STR:
 1. Delete devtools/shared/css/generated/properties-db.js
 2. Run ./mach devtools-css-db

ACTUAL RESULTS:
{
Error running mach:

    ['devtools-css-db']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

CalledProcessError: Command '[u'/scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/obj/dist/bin/xpcshell', '-g', u'/scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/obj/dist/bin', '-a', u'/scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/obj/dist/bin/browser', '/scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/devtools/shared/css/generated/generate-properties-db.js']' returned non-zero exit status 3

  File "/scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/devtools/shared/css/generated/mach_commands.py", line 46, in generate_css_db
    db = self.get_properties_db_from_xpcshell()
  File "/scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/devtools/shared/css/generated/mach_commands.py", line 105, in get_properties_db_from_xpcshell
    env = sub_env)
}

EXPECTED RESULTS:
The command should complete successfully (and regenerate properties-db.js from scratch).  i.e. it should not depend on the file that it generates.


As far as I can tell, this is the partial cause of the latest set of woes on bug 1308651 -- ./mach devtools-css-db doesn't seem to be updating CSS_PROPERTIES, and I think the reason is partly because it's using the current state of this .js file as a Source Of Truth when determining the final state of this same .js file.
(In reply to Daniel Holbert [:dholbert] from comment #0) 
> As far as I can tell, this is the partial cause of the latest set of woes on
> bug 1308651 -- ./mach devtools-css-db doesn't seem to be updating
> CSS_PROPERTIES

Sorry, bug mis-link -- I meant to link to "bug 1300895" there.

My STR in comment 0 may perhaps a bit too draconian. Alternate STR:
 1. Update your tree to revision b7c0df58a2f4 (currently on autoland) and rebuild.
 2. Run ./mach xpcshell-test devtools/shared/tests/unit/test_css-properties-db.js
  --> Notice that the test fails, with a warning about agreement on "column-count"
 3. Run ./mach devtools-css-db (as the test failure instructs you to)
 4. Examine the changes to your working directory.

EXPECTED RESULTS:
You should end up with "column-count" (and other unprefixed CSS column-* properties) in the CSS_PROPERTIES section of your  your properties-db.js file.

ACTUAL RESULTS:
"column-count" does not exist in the CSS_PROPERTIES section of your properties-db.js file.  Its old now-removed prefixed version is there, but the newly-unprefixed version is still missing.


From a bit more local testing, it seems we can work around this problem by *deleting* the contents of properties-db.js' CSS_PROPERTIES map entirely -- if I clean it out so it's just CSS_PROPERTIES = {}; and then run ./mach devtools-css-db, I get different output (with the expected *unprefixed* column-* css properties listed now).  That is...also unexpected, but it seems to be perhaps the best workaround for this issue right now.
Depends on: 1290988
Summary: ./mach devtools-css-db depends on the file that it's attempting to generate → ./mach devtools-css-db depends on the file that it's attempting to generate (and as a result, it doesn't do the right thing after a property's been renamed)
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Framework
Priority: -- → P1
Yup, looks like I'm self-referential in this. Let me fix it.
Thanks, Greg!
Comment on attachment 8799960 [details]
Bug 1309065 - Be smarter about filling in the client-side css properties db;

https://reviewboard.mozilla.org/r/85010/#review83566

::: devtools/server/actors/css-properties.js:68
(Diff revision 1)
> -    // In order to maintain any backwards compatible changes when debugging older
> +    properties[name] = {
> -    // clients, take the definition from the static CSS properties database, and fill it
> -    // in with the most recent property definition from the server.
> -    const clientDefinition = CSS_PROPERTIES[name] || {};
> -    const serverDefinition = {

I really don't know why I was doing this here...

::: devtools/shared/fronts/css-properties.js
(Diff revision 1)
> -    // Ensure the database was returned in a format that is understood.
> -    // Older versions of the protocol could return a blank database.

This was only true if you sent the client version number and it matched the platform version number. This won't actually happen in practice. I was just being overly cautious when I wrote this.
Comment on attachment 8799960 [details]
Bug 1309065 - Be smarter about filling in the client-side css properties db;

https://reviewboard.mozilla.org/r/85010/#review83590

Thanks.  This looks good.
Attachment #8799960 - Flags: review?(ttromey) → review+
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2aef437d23b7
Be smarter about filling in the client-side css properties db; r=tromey
Thanks, will rebase and take a look.
Flags: needinfo?(gtatum)
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b20660535269
Be smarter about filling in the client-side css properties db; r=tromey
https://hg.mozilla.org/mozilla-central/rev/b20660535269
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: