Closed Bug 1442559 Opened 6 years ago Closed 6 years ago

browser_parsable_css.js will permafail for opt builds of beta 60

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: aryx, Assigned: xidorn)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1442559 part 1 - Make it clearer what whitelist item filters the error.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1442559 part 2 - Use a separate whitelist for prop names.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1442559 part 3 - Detect whitelist item which doesn't have any condition.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Xidorn, you fixed this in bug 1333175. Any reason why this is failing again?
Flags: needinfo?(jaws) → needinfo?(xidorn+moz)
Oh, are we shipping this now in 60 so we can remove it from the whitelist? I'm not sure why this isn't failing on Nightly builds though.
We are not shipping unprefixed fullscreen in 60, so I don't expect it to become useless. There must be some other reason...
So the reason here is that the propName items added in bug 1441882 whitelists all warnings, and thus any whitelist item appended after them would be reported unused.

Sounds like bug 1441882 is basically making the whole whitelist meaningless. You can remove everything before the first {propName} item and the test would still pass.
Flags: needinfo?(xidorn+moz)
The reason is that function ignoredError would always return true if there is a whitelist item contains neither sourceName nor errorMessage.
To avoid this kind of issue happens again, we probably should really add a catch all whitelist item and ensure that it never touched.
Comment on attachment 8955699 [details]
Bug 1442559 - Remove unused CSS whitelist entry for fullscreen.

https://reviewboard.mozilla.org/r/224784/#review230814

r- per bug comments that this isn't the right fix.
Attachment #8955699 - Flags: review?(gijskruitbosch+bugs) → review-
Working on a patch.
Assignee: nobody → xidorn+moz
Comment on attachment 8955728 [details]
Bug 1442559 part 1 - Make it clearer what whitelist item filters the error.

https://reviewboard.mozilla.org/r/224804/#review230840

::: browser/base/content/test/static/browser_parsable_css.js:164
(Diff revision 1)
> +function dumpWhitelistItem(item) {
> +  let props = [];
> +  for (let prop in item) {
> +    props.push(`${prop}: ${item[prop]}`);
> +  }
> +  return `{${props.join(", ")}}`;
> +}

Nit:

```js
return JSON.stringify(item, (key, value) => {
  return (value instanceof RegExp) ? value.toString() : value;
});
```

::: browser/base/content/test/static/browser_parsable_css.js:192
(Diff revision 1)
>        }
>      }
>      if (matches) {
>        whitelistItem.used = true;
> +      let {sourceName, errorMessage} = aErrorObject;
> +      info(`Ignored error "${errorMessage}" on ${sourceName}. ` +

Nit: add `because of whitelist item` to the string before dumping the whitelist item, please.
Attachment #8955728 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8955729 [details]
Bug 1442559 part 2 - Use a separate whitelist for prop names.

https://reviewboard.mozilla.org/r/224806/#review230842

::: browser/base/content/test/static/browser_parsable_css.js:488
(Diff revision 1)
>                  " file: " + item.file +
>                  " from: " + item.from);

Looks like this could also use `dumpWhitelistItem`?

::: browser/base/content/test/static/browser_parsable_css.js:499
(Diff revision 1)
> +  for (let item of propNameWhitelist) {
> +    if (!item.used &&
> +        (!item.platforms || item.platforms.includes(AppConstants.platform)) &&
> +        isDevtools == item.isFromDevTools &&
> +        !item.intermittent) {
> +      ok(false, `Unused whitelist item. propName: ${item.propName}`);

Ditto.

More generally, can you factor these 3 loops out into a function that just takes the whitelist in question as an argument, with perhaps an argument to use in the error message string to clarify which whitelist this came from - though if we use `dumpWhitelistItem` everywhere I don't think it'll be much of an issue figuring out what it is anyway.
Attachment #8955729 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8955730 [details]
Bug 1442559 part 3 - Detect whitelist item which doesn't have any condition.

https://reviewboard.mozilla.org/r/224808/#review230844
Attachment #8955730 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2bf42743feb
part 1 - Make it clearer what whitelist item filters the error. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6371120b7c1c
part 2 - Use a separate whitelist for prop names. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d540c55c96c5
part 3 - Detect whitelist item which doesn't have any condition. r=Gijs
Backed out for eslint failure at /builds/worker/checkouts/gecko/browser/base/content/test/static/browser_parsable_css.js:474

Push that caused the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d540c55c96c5e4fdab1434032628fa876152b8e1

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=165884240&repo=autoland&lineNumber=240

Backout: https://hg.mozilla.org/integration/autoland/rev/53f082b32ba9eb0b9febdc97373c89b9ac500bd3
Flags: needinfo?(xidorn+moz)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d6a3eb93a00
part 1 - Make it clearer what whitelist item filters the error. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/756d5b77b540
part 2 - Use a separate whitelist for prop names. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/39e3ac3b1194
part 3 - Detect whitelist item which doesn't have any condition. r=Gijs
Flags: needinfo?(xidorn+moz)
Comment on attachment 8955729 [details]
Bug 1442559 part 2 - Use a separate whitelist for prop names.

https://reviewboard.mozilla.org/r/224806/#review230918


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/static/browser_parsable_css.js:472
(Diff revision 2)
>    // as the ok(false) in messageIsCSSError.
>    let errors = messages.filter(messageIsCSSError);
>    is(errors.length, 0, "All the styles (" + allPromises.length + ") loaded without errors.");
>  
>    // Confirm that all whitelist rules have been used.
> +  function checkWhitelist(whitelist) {

Error: 'whitelist' is already declared in the upper scope. [eslint: no-shadow]
Comment on attachment 8955730 [details]
Bug 1442559 part 3 - Detect whitelist item which doesn't have any condition.

https://reviewboard.mozilla.org/r/224808/#review230920


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/static/browser_parsable_css.js:480
(Diff revision 2)
>    // as the ok(false) in messageIsCSSError.
>    let errors = messages.filter(messageIsCSSError);
>    is(errors.length, 0, "All the styles (" + allPromises.length + ") loaded without errors.");
>  
>    // Confirm that all whitelist rules have been used.
>    function checkWhitelist(whitelist) {

Error: 'whitelist' is already declared in the upper scope. [eslint: no-shadow]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: