923251 - Cleanup some code in layout/style/

Status: RESOLVED FIXED

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Make ParseMediaList return void;

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Make Loader::PrepareSheet return void;

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Make nsCSSStyleSheet::AppendAllChildSheets return void;

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d: Remove nsCSSStyleSheet::eUniqueInner_CloneFailed;

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part e: Make nsPresContext::EnsureSafeToHandOutCSSRules return void;

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part f: Pass an Element to inDOMUtils::GetRuleNodeForContent and rename it;

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part g: Make nsCSSStyleSheet::WillDirty return void;

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part h: Don't handle allocation failure in nsMediaQuery::Clone;

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part i: Outparamdel nsMediaList::Clone;

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

heycam, feel free to steal these reviews if you have time.

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Cameron McCormack (:heycam)]

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.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.

Comment 14

[Cameron McCormack (:heycam)]

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.

Comment 15

[Cameron McCormack (:heycam)]

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.

Comment 16

[Cameron McCormack (:heycam)]

(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

[Cameron McCormack (:heycam)]

*precedent

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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