Closed Bug 1465628 Opened 6 years ago Closed 6 years ago

Looks like we need to update devtools css db

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: dholbert, Assigned: xidorn)

Details

Attachments

(5 files)

(here's the diff that ./mach devtools-css-db generates on dholbert's machine)
1.98 KB, patch
Details | Diff | Splinter Review
Bug 1465628 part 1 - Use a static table for property preferences.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1465628 part 2 - Add InspectorUtils API for getting property preferences.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1465628 part 3 - Generate preference list from inspector utils.
59 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1465628 part 4 - Update test to check more stuff.
59 bytes, text/x-review-board-request
tromey
: review+
Details 
Right now in my mozilla-inbound clone, ./mach devtools-css-db generates a non-empty patch, which means we need to run it & land whatever changes it makes.

(The diffs are around scrollbar color stuff, plus a seemingly-arbitrary reordering of "cursor" through "caret-color".)

Tentatively needinfo'ing xidorn since he's working on caret color and has "hg blame" on the reordered chunk.
Flags: needinfo?(xidorn+moz)
Just for illustration purposes, here's the diff that it generates for me, though maybe I'll defer to xidorn to generate this himself (since it's trivial & in case he gets something different, & since these changes are likely tied to work he's done recently).
FWIW, mreschenberg got the same "cursor...caret-color" reordering in a tentative "./mach devtools-css-db" trial-run that she did earlier today (and didn't end up needing to land because it turns out her patch shouldn't impact that file after all). That (discarded) patch is here, for reference:
 https://reviewboard.mozilla.org/r/247930/diff/1#index_header

She's on Mac, and I'm on Linux, so that means this same reordering happens on those two platforms at least.  So, there doesn't seem to be a per-platform dependence here, FWIW (which is good, but which is also mysterious about why the ordering is "wrong" in the first place).
OK, so I checked the related test test_css-properties-db.js which is supposed to catch this kind of issues, and it turns out that it happens to not catch them.

The reordering is not caught because it's content of all property, which according to the comment
> The "all" property can contain information that can be turned on and off by
> preferences. These values can be different between OSes, so ignore the equality
> check for this property, since this is likely to fail.
so the check is intentionally disabled there.

I doubt this reasoning, though. At very least, it seems we got the same result among all the major systems (macOS for mreschenberg, Linux for dholbert, and Windows for me). And it feels to me that we ought to ship web features in among platforms in general. So probably we should have the special check removed.

The second part of preferences is not caught because of the design of this test. It checks every properties present in the database and check whether their preferences match. However, it seems properties which are not enabled don't have items in the database, and thus they are not checked.

I think we should change how the test work to catch this kind of change.
Flags: needinfo?(xidorn+moz)
Hmmm... so we don't check preferences because there is no runtime way to check that at all. We don't have an API to get the list of preferences, which is probably fine.

I guess we should just accept the situation of preferences, then.
Comment on attachment 8982125 [details]
Bug 1465628 part 3 - Generate preference list from inspector utils.

https://reviewboard.mozilla.org/r/248154/#review254408

Thank you for the patch.  I think this looks good.
Attachment #8982125 - Flags: review?(ttromey) → review+
Comment on attachment 8982126 [details]
Bug 1465628 part 4 - Update test to check more stuff.

https://reviewboard.mozilla.org/r/248156/#review254410

Thanks.

I guess the reason that css-properties-db.js changed in this patch and not in the previous patch is that first the test had to be updated?  That seems fine.

I don't think I understand what causes the differences in cursor..caret-color that were pointed out in comment #0.  Perhaps it's not the intent of this bug to fix that problem, but if it is still not understood then a follow-up bug would be good to have.  Maybe it's as simple as sorting the list somewhere.
Attachment #8982126 - Flags: review?(ttromey) → review+
(In reply to Tom Tromey :tromey from comment #10)
> Comment on attachment 8982126 [details]
> Bug 1465628 part 4 - Update test to check more stuff.
> 
> https://reviewboard.mozilla.org/r/248156/#review254410
> 
> Thanks.
> 
> I guess the reason that css-properties-db.js changed in this patch and not
> in the previous patch is that first the test had to be updated?  That seems
> fine.

It can happen in any patch, actually. I just don't think it logically belongs to any other change, and I think having a separate commit for it is probably not very useful, so I included it in the last one.

> I don't think I understand what causes the differences in
> cursor..caret-color that were pointed out in comment #0.  Perhaps it's not
> the intent of this bug to fix that problem, but if it is still not
> understood then a follow-up bug would be good to have.  Maybe it's as simple
> as sorting the list somewhere.

The reason of the move of cursor..caret-color is bug 1460192 which renames "pointing" into "inherited_ui", and changes its position in the longhand mod.

The order of subproperties of "all" shorthand is rather arbitrary, depending on where we define them in Servo code.

Since it is indeed a web-exposed behavior, we may want to give it a more stable order at some point...
Attachment #8982123 - Flags: review?(cam) → review?(emilio)
Attachment #8982124 - Flags: review?(cam) → review?(emilio)
Comment on attachment 8982123 [details]
Bug 1465628 part 1 - Use a static table for property preferences.

https://reviewboard.mozilla.org/r/248150/#review255316
Attachment #8982123 - Flags: review?(emilio) → review+
Comment on attachment 8982124 [details]
Bug 1465628 part 2 - Add InspectorUtils API for getting property preferences.

https://reviewboard.mozilla.org/r/248152/#review255320

::: layout/inspector/InspectorUtils.h:117
(Diff revision 1)
>    // property aliases included.
>    static void GetCSSPropertyNames(GlobalObject& aGlobal,
>                                    const PropertyNamesOptions& aOptions,
>                                    nsTArray<nsString>& aResult);
>  
> +  // Get a list of all property preferences.

nit: This comment doesn't seem very useful. Maybe expand a bit or remove?
Attachment #8982124 - Flags: review?(emilio) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5ac0c7a9fb4
part 1 - Use a static table for property preferences. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6832baa28e3f
part 2 - Add InspectorUtils API for getting property preferences. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e9cdff122793
part 3 - Generate preference list from inspector utils. r=tromey
https://hg.mozilla.org/integration/autoland/rev/04f276eb2f0e
part 4 - Update test to check more stuff. r=tromey
Backed out 4 changesets (bug 1465628) for Eslint failure. CLOSED TREE

Log:
Backed out 4 changesets (bug 1465628) for Eslint failure. CLOSED TREE

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=04f276eb2f0ea509ded9f185c0f3287ba3b91b87&selectedJob=181818130

Backout:
https://hg.mozilla.org/integration/autoland/rev/a9e6e98ff2c2abe8986d24221cb2c4ae7204a5ba
Flags: needinfo?(xidorn+moz)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc6993f44edd
part 1 - Use a static table for property preferences. r=emilio
https://hg.mozilla.org/integration/autoland/rev/017d65a99eab
part 2 - Add InspectorUtils API for getting property preferences. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2ce03f7e1d5b
part 3 - Generate preference list from inspector utils. r=tromey
https://hg.mozilla.org/integration/autoland/rev/425391ffe743
part 4 - Update test to check more stuff. r=tromey
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: