Cleanup some code in layout/style/

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

unspecified
mozilla28
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(9 attachments)

No description provided.
heycam, feel free to steal these reviews if you have time.
Summary: Cleanup some code in layout → Cleanup some code in layout/style/
Assignee

Updated

6 years ago
Flags: needinfo?(cam)
Attachment #813254 - Flags: review?(dbaron) → review+
Attachment #813255 - Flags: review?(dbaron) → review+
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 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+
Attachment #813259 - Flags: review?(dbaron) → review+
Attachment #813260 - Flags: review?(dbaron) → review+
(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.
Attachment #813261 - Flags: review?(dbaron) → review+
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 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+
Flags: needinfo?(cam)
(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.
*precedent

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.