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

RESOLVED FIXED in mozilla37



CSS Parsing and Computation
3 years ago
3 years ago


(Reporter: dholbert, Assigned: dholbert)


Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(3 attachments, 1 obsolete attachment)

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

Filing this bug on cleaning that up.
OS: Linux → All
Hardware: x86_64 → All
Created attachment 8539487 [details] [diff] [review]
fix v1
Attachment #8539487 - Flags: review?(cam)
Created attachment 8539488 [details] [diff] [review]
fix v1, "diff -w" version
Attachment #8539487 - Flags: review?(cam) → review+
Flags: in-testsuite-
Backed out in for apparently breaking all tests: 

(Builds were broken on dholbert's push, so this is the most recent push that built with dholbert's changes.)
Flags: needinfo?(dholbert)
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.)
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.
Flags: needinfo?(dholbert)
Created attachment 8540482 [details] [diff] [review]
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. :)
Attachment #8540482 - Flags: review+
Created attachment 8540483 [details] [diff] [review]
fix v2, "diff -w" version
Attachment #8539487 - Attachment is obsolete: true


3 years ago
Depends on: 1114871
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1116087
No longer depends on: 1116087
You need to log in before you can comment on or make changes to this bug.