Andrew McCreight [:mccr8] Assignee
	Description • 9 years ago

This method uses nsIProgrammingLanguage, but there are only two possible languages, JAVASCRIPT or UNKNOWN.

The logic here is weird, where the order of the attributes matters in terms of whether we decide something is actually JavaScript, but I'm going to leave that alone.

	Andrew McCreight [:mccr8] Assignee
	Comment 1 • 9 years ago

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f2a75f20f9

None of these patches are very complex.

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 9 years ago

langID is either be UNKNOWN or JAVASCRIPT so we never run the code here.

	Andrew McCreight [:mccr8] Assignee
	Comment 4 • 9 years ago

I'll attach a -w patch for this.

	Andrew McCreight [:mccr8] Assignee
	Comment 5 • 9 years ago

Use a boolean isJavaScript rather than an enum which is either UNKNOWN or JAVASCRIPT.

Note that the existing behavior is odd, where encountering some kinds
of failures is immediately fatal, which other kinds are not, and the
order we see attributes in affects whether or not we decide something
is JavaScript. This behavior should be wholly preserved by this patch.

	Andrea Marchesini [:baku]
	Comment 8 • 9 years ago

Comment on attachment 8584795 [details] [diff] [review]
part 5 - Don't use the generic nsIProgrammingLanguage enum in XULContentSinkImpl::OpenScript().

Review of attachment 8584795 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xul/nsXULContentSink.cpp
@@ +857,5 @@
>  nsresult
>  XULContentSinkImpl::OpenScript(const char16_t** aAttributes,
>                                 const uint32_t aLineNumber)
>  {
> +  bool isJavaScript = true;

I would set it to false here.

@@ +896,5 @@
>                } else if (rv != NS_ERROR_INVALID_ARG) {
>                    return rv;
>                }
>            } else {
> +              isJavaScript = false;

Then you can remove this else stmt.

	Andrea Marchesini [:baku]
	Comment 9 • 9 years ago

Comment on attachment 8584796 [details] [diff] [review]
part 6 - Remove some useless null checks on infallible new in XULContentSinkImpl.

Review of attachment 8584796 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xul/nsXULContentSink.cpp
@@ +443,5 @@
>  
>  /**** BEGIN NEW APIs ****/
>  
>  
>  NS_IMETHODIMP 

Remove this extra space too.

	Andrew McCreight [:mccr8] Assignee
	Comment 10 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #8)
> I would set it to false here.

I don't think that will preserve the existing behavior, unless there are some guarantees on aAttributes.  for instance, if aAttributes is an empty list, then currently isJavaScript will end up as |true|, but with your patch it will end up with |false|.

	Andrew McCreight [:mccr8] Assignee
	Comment 11 • 9 years ago

Can I just leave it as is?

	Andrew McCreight [:mccr8] Assignee
	Comment 12 • 9 years ago

Landed the preliminary cleanup patches.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f421dc3eefe8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f23b648a2970
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/226747de554e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/afe401fb6901

	Andrea Marchesini [:baku]
	Comment 13 • 9 years ago

(In reply to Andrew McCreight [:mccr8] from comment #11)
> Can I just leave it as is?

sure.

	Andrew McCreight [:mccr8] Assignee
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6ebc72b1b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6800c24bb92

I forgot to remove that bit of trailing whitespace, but I'll just attach a patch to remove all of it in a moment.

	Andrew McCreight [:mccr8] Assignee
	Comment 16 • 9 years ago

(The diff -w is empty.)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/f421dc3eefe8
https://hg.mozilla.org/mozilla-central/rev/f23b648a2970
https://hg.mozilla.org/mozilla-central/rev/226747de554e
https://hg.mozilla.org/mozilla-central/rev/afe401fb6901

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7f6ebc72b1b8
https://hg.mozilla.org/mozilla-central/rev/d6800c24bb92

	Andrew McCreight [:mccr8] Assignee
	Comment 19 • 9 years ago

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

	Phil Ringnalda (:philor)
	Comment 20 • 9 years ago

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