Closed
Bug 1147946
Opened 10 years ago
Closed 10 years ago
Remove use of nsIProgrammingLanguage in XULContentSinkImpl::OpenScript()
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(8 files)
2.62 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.82 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
4.69 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
4.42 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
3.95 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1015 bytes,
patch
|
Details | Diff | Splinter Review | |
12.62 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f2a75f20f9
None of these patches are very complex.
Attachment #8584791 -
Flags: review?(amarchesini)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8584792 -
Flags: review?(amarchesini)
Assignee | ||
Comment 3•10 years ago
|
||
langID is either be UNKNOWN or JAVASCRIPT so we never run the code here.
Attachment #8584793 -
Flags: review?(amarchesini)
Assignee | ||
Comment 4•10 years ago
|
||
I'll attach a -w patch for this.
Attachment #8584794 -
Flags: review?(amarchesini)
Assignee | ||
Comment 5•10 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.
Attachment #8584795 -
Flags: review?(amarchesini)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8584796 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•10 years ago
|
||
Updated•10 years ago
|
Attachment #8584791 -
Flags: review?(amarchesini) → review+
Updated•10 years ago
|
Attachment #8584792 -
Flags: review?(amarchesini) → review+
Updated•10 years ago
|
Attachment #8584793 -
Flags: review?(amarchesini) → review+
Updated•10 years ago
|
Attachment #8584794 -
Flags: review?(amarchesini) → review+
Comment 8•10 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.
Attachment #8584795 -
Flags: review?(amarchesini) → review+
Comment 9•10 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.
Attachment #8584796 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 10•10 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|.
Assignee | ||
Comment 12•10 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
Keywords: leave-open
Comment 13•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Can I just leave it as is?
sure.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 14•10 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.
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8587683 -
Flags: review?(amarchesini)
Assignee | ||
Comment 16•10 years ago
|
||
(The diff -w is empty.)
Updated•10 years ago
|
Attachment #8587683 -
Flags: review?(amarchesini) → review+
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Keywords: leave-open
Comment 20•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•