Closed Bug 1147946 Opened 5 years ago Closed 5 years ago

Remove use of nsIProgrammingLanguage in XULContentSinkImpl::OpenScript()


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: mccr8, Assigned: mccr8)




(8 files)

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.
Blocks: 1147947
try run:

None of these patches are very complex.
Attachment #8584791 - Flags: review?(amarchesini)
langID is either be UNKNOWN or JAVASCRIPT so we never run the code here.
Attachment #8584793 - Flags: review?(amarchesini)
I'll attach a -w patch for this.
Attachment #8584794 - Flags: review?(amarchesini)
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.
Attachment #8584795 - Flags: review?(amarchesini)
Attached patch part 4 with -wSplinter Review
Attachment #8584791 - Flags: review?(amarchesini) → review+
Attachment #8584792 - Flags: review?(amarchesini) → review+
Attachment #8584793 - Flags: review?(amarchesini) → review+
Attachment #8584794 - Flags: review?(amarchesini) → review+
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.
Attachment #8584795 - Flags: review?(amarchesini) → review+
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 ****/

Remove this extra space too.
Attachment #8584796 - Flags: review?(amarchesini) → review+
(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|.
Can I just leave it as is?
Flags: needinfo?(amarchesini)
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Can I just leave it as is?

Flags: needinfo?(amarchesini)

I forgot to remove that bit of trailing whitespace, but I'll just attach a patch to remove all of it in a moment.
(The diff -w is empty.)
Attachment #8587683 - Flags: review?(amarchesini) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.