Closed
Bug 923251
Opened 9 years ago
Closed 9 years ago
Cleanup some code in layout/style/
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: Ms2ger, Assigned: Ms2ger)
Details
(Whiteboard: [qa-])
Attachments
(9 files)
7.45 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
6.38 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
2.91 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
5.87 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
3.38 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
4.06 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
16.82 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
2.61 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
3.26 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #813254 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #813255 -
Flags: review?(dbaron)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #813256 -
Flags: review?(dbaron)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #813257 -
Flags: review?(dbaron)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #813259 -
Flags: review?(dbaron)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #813260 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #813261 -
Flags: review?(dbaron)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #813262 -
Flags: review?(dbaron)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #813263 -
Flags: review?(dbaron)
Assignee | ||
Comment 10•9 years ago
|
||
heycam, feel free to steal these reviews if you have time.
Updated•9 years ago
|
Summary: Cleanup some code in layout → Cleanup some code in layout/style/
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cam)
Updated•9 years ago
|
Attachment #813254 -
Flags: review?(dbaron) → review+
Updated•9 years ago
|
Attachment #813255 -
Flags: review?(dbaron) → review+
Comment 11•9 years ago
|
||
Comment on attachment 813256 [details] [diff] [review] Part c: Make nsCSSStyleSheet::AppendAllChildSheets return void; Is there much point to using MOZ_ALWAYS_TRUE? Seems unlikely the infallible nsTArray will ever accidentally start returning null without being noticed.
Attachment #813256 -
Flags: review?(dbaron) → review+
Comment 12•9 years ago
|
||
Comment on attachment 813257 [details] [diff] [review] Part d: Remove nsCSSStyleSheet::eUniqueInner_CloneFailed; + MOZ_ASSUME_UNREACHABLE("All cases handled."); The comments for MOZ_ASSUME_UNREACHABLE say * In Gecko, you probably should not use this macro outside of performance- or * size-critical code, because it's unsafe. If you don't care about code size * or performance, you should probably use MOZ_ASSERT or MOZ_CRASH. and I don't think this is a particularly performance sensitive bit of code, so maybe use MOZ_ASSERT instead. Same wondering about the use of MOZ_ALWAYS_TRUE as the previous patch. r=me with MOZ_ASSERT instead of MOZ_ASSUME_UNREACHABLE and a justification for MOZ_ALWAYS_TRUE being worth it, or it being removed.
Attachment #813257 -
Flags: review?(dbaron) → review+
Updated•9 years ago
|
Attachment #813259 -
Flags: review?(dbaron) → review+
Updated•9 years ago
|
Attachment #813260 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #11) > Comment on attachment 813256 [details] [diff] [review] > Part c: Make nsCSSStyleSheet::AppendAllChildSheets return void; > > Is there much point to using MOZ_ALWAYS_TRUE? Seems unlikely the infallible > nsTArray will ever accidentally start returning null without being noticed. I think it's somewhat useful as documentation; can go either way though. (In reply to Cameron McCormack (:heycam) from comment #12) > Comment on attachment 813257 [details] [diff] [review] > Part d: Remove nsCSSStyleSheet::eUniqueInner_CloneFailed; > > + MOZ_ASSUME_UNREACHABLE("All cases handled."); > > The comments for MOZ_ASSUME_UNREACHABLE say > > * In Gecko, you probably should not use this macro outside of performance- > or > * size-critical code, because it's unsafe. If you don't care about code > size > * or performance, you should probably use MOZ_ASSERT or MOZ_CRASH. > > and I don't think this is a particularly performance sensitive bit of code, > so maybe use MOZ_ASSERT instead. That comment is overly pessimistic, IMO. I think I tried MOZ_ASSERT, but then it didn't compile on non-debug builds because I didn't return anything, so it'd have to be MOZ_CRASH, I guess.
Updated•9 years ago
|
Attachment #813261 -
Flags: review?(dbaron) → review+
Comment 14•9 years ago
|
||
Comment on attachment 813262 [details] [diff] [review] Part h: Don't handle allocation failure in nsMediaQuery::Clone; > nsresult > nsMediaList::Clone(nsMediaList** aResult) > { > nsRefPtr<nsMediaList> result = new nsMediaList(); > if (!result || !result->mArray.AppendElements(mArray.Length())) > return NS_ERROR_OUT_OF_MEMORY; >- for (int32_t i = 0, i_end = mArray.Length(); i < i_end; ++i) { >- if (!(result->mArray[i] = mArray[i]->Clone())) { >- return NS_ERROR_OUT_OF_MEMORY; >- } >+ for (uint32_t i = 0, i_end = mArray.Length(); i < i_end; ++i) { >+ MOZ_ALWAYS_TRUE(result->mArray[i] = mArray[i]->Clone()); > } > NS_ADDREF(*aResult = result); > return NS_OK; > } Again I wonder whether it's worth having the MOZ_ALWAYS_TRUE in here. If you really want to keep an assertion, maybe instead: result->mArray[i] = mArray[i]->Clone(); MOZ_ASSERT(result->mArray[i]); I think putting the assignment inside the MOZ_ALWAYS_TRUE makes things look a bit less clear. r=me with that.
Attachment #813262 -
Flags: review?(dbaron) → review+
Comment 15•9 years ago
|
||
Comment on attachment 813263 [details] [diff] [review] Part i: Outparamdel nsMediaList::Clone; >-nsresult >-nsMediaList::Clone(nsMediaList** aResult) >+already_AddRefed<nsMediaList> >+nsMediaList::Clone() > { > nsRefPtr<nsMediaList> result = new nsMediaList(); >- if (!result || !result->mArray.AppendElements(mArray.Length())) >- return NS_ERROR_OUT_OF_MEMORY; >+ MOZ_ALWAYS_TRUE(result->mArray.AppendElements(mArray.Length())); > for (uint32_t i = 0, i_end = mArray.Length(); i < i_end; ++i) { > MOZ_ALWAYS_TRUE(result->mArray[i] = mArray[i]->Clone()); > } >- NS_ADDREF(*aResult = result); >- return NS_OK; >+ return result.forget(); > } Again, don't think the MOZ_ALWAYS_TRUE on an infallible nsTArray memory allocating method is worth it.
Attachment #813263 -
Flags: review?(dbaron) → review+
Updated•9 years ago
|
Flags: needinfo?(cam)
Comment 16•9 years ago
|
||
(In reply to :Ms2ger from comment #13) > I think it's somewhat useful as documentation; can go either way though. I think the visual impact outweighs the usefulness. > That comment is overly pessimistic, IMO. I think I tried MOZ_ASSERT, but > then it didn't compile on non-debug builds because I didn't return anything, > so it'd have to be MOZ_CRASH, I guess. Especially with only two values to test for, I tend to write it like: if (one case) { ... } else { MOZ_ASSERT(the other case); ... } choosing the least worst case for the MOZ_ASSERT one to be in if we happen to get a bogus value. I worry a bit that the compiler is going could generate code that is worse if it encounters the "unreachable" branch than if we either assert and do nothing, or assert and do one of the branches for the values we do handle. I think MOZ_CRASH would be fine; there's precedence for that in nsFlexContainerFrame.cpp, at least.
Comment 17•9 years ago
|
||
*precedent
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/168bfdfdd3c4 https://hg.mozilla.org/mozilla-central/rev/be85e2f0ba72 https://hg.mozilla.org/mozilla-central/rev/def772f9b44a https://hg.mozilla.org/mozilla-central/rev/9f50f1e05bfd https://hg.mozilla.org/mozilla-central/rev/9bbf4d1c1bb9 https://hg.mozilla.org/mozilla-central/rev/1e10073501d4 https://hg.mozilla.org/mozilla-central/rev/c98c087abf2c https://hg.mozilla.org/mozilla-central/rev/53ea7f67aee5 https://hg.mozilla.org/mozilla-central/rev/13460edc7f57
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•9 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•