1113811 - Remove unnecessary null-check of "new css::Declaration" in nsCSSParser

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

CSSParserImpl::ParseDeclarationBlock has an unnecessary null-check after new (unnecessary because new is infallible).

Filing this bug on cleaning that up.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1, "diff -w" version

Comment 3

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a3f2525323

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2b933356daf7 for apparently breaking all tests:

https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=f22bb012bf58 

(Builds were broken on dholbert's push, so this is the most recent push that built with dholbert's changes.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like the patch moved the declaration->CompressFrom() line into the loop, when it shouldn't have.  (Should have un-indented it instead of leaving it at its current indent.)

Comment 6

[Daniel Holbert [:dholbert]]

Yup -- looks like I mistakenly deleted the wrong closing-brace. (not the one that corresponded to the "if" condition that I'm removing). (This  is equivalent to what dbaron said in comment 5)

I think I probably did this because my merge too, "meld", highlighted the wrong one for me, because it was confused about bracing-level by an extra (quoted!) closing-brace in this code: "ExpectSymbol('}', true)"

Anyway, I'm going to rebuild & briefly test with that locally, and if all is well, I'll re-push with that fixed.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v2: now with the correct closing-brace removed

Here's the fixed patch. I verified that I can open a browser and open/close tabs, with this patch applied. (I could not, with the previous patch; we crash/abort before that.)

I think that's sufficient testing for a patch of this sort (removing an unnecessary null-check).   Sorry for not performing it on the earlier patch & catching that mistake earlier. :)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2, "diff -w" version

Comment 9

[Daniel Holbert [:dholbert]]

Re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a28c173dbd61

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a28c173dbd61