[HTML5] Re-implement text/plain loading using the HTML5 tokenizer

RESOLVED FIXED in mozilla10

Status

()

Core
HTML: Parser
P3
normal
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

(Blocks: 2 bugs, {html5})

Other Branch
mozilla10
html5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
http://www.whatwg.org/specs/web-apps/current-work/#read-text

Comment 1

9 years ago
Whoever is going to work on this could try to fix bug 369301 (or the issue it's a dupe of) at the same time.
(Assignee)

Updated

9 years ago
Priority: -- → P3
(Assignee)

Updated

8 years ago
Assignee: hsivonen → nobody
(Assignee)

Updated

8 years ago
Blocks: 553999
(Assignee)

Updated

7 years ago
Blocks: 563890
(Assignee)

Comment 2

6 years ago
Created attachment 530600 [details] [diff] [review]
Reimplement text/plain

What test cases should I write for this (if any)?
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #530600 - Flags: review?(jonas)

Updated

6 years ago
Blocks: 655434
(Assignee)

Comment 3

6 years ago
Created attachment 531031 [details] [diff] [review]
Reimplement text/plain, v2

This patch fixes a wrong logical operator and increases the timeout on a test that uses setTimeout to poke a document loaded from a data: URL.
Attachment #531031 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Blocks: 659763
(Assignee)

Comment 4

6 years ago
Created attachment 567087 [details] [diff] [review]
Reimplement text/plain, rebased
Attachment #530600 - Attachment is obsolete: true
Attachment #531031 - Attachment is obsolete: true
Attachment #567087 - Flags: review?(Olli.Pettay)
Attachment #530600 - Flags: review?(jonas)
Attachment #531031 - Flags: review?(jonas)

Comment 5

6 years ago
Comment on attachment 567087 [details] [diff] [review]
Reimplement text/plain, rebased

># HG changeset patch
># Parent e4bbf1e529e9249dc01a6810e769fbf462514894
>Bug 479959 - Reimplement text/plain loading using the HTML5 parser. r=NOT_REVIEWED.
>
>diff --git a/content/base/test/chrome/test_bug430050.xul b/content/base/test/chrome/test_bug430050.xul
>--- a/content/base/test/chrome/test_bug430050.xul
>+++ b/content/base/test/chrome/test_bug430050.xul
>@@ -31,17 +31,17 @@ https://bugzilla.mozilla.org/show_bug.cg
>     document.documentElement.addEventListener('DOMAttrModified',
>       function(evt) {
>         if (evt.target == evt.currentTarget) {
>           document.getElementById('b').setAttribute("src",
>                                                     "data:text/plain,failed");
>           document.getElementById('b').loadURI('data:text/plain,succeeded',
>                                                null,
>                                                'UTF-8');
>-          setTimeout(endTest, 0);
>+          setTimeout(endTest, 100);
Why this change? Looks pretty error prone.
Perhaps the test should use load event listener and not timeout at all.

> nsHTMLDocument::StartDocumentLoad(const char* aCommand,
>                                   nsIChannel* aChannel,
>                                   nsILoadGroup* aLoadGroup,
>                                   nsISupports* aContainer,
>                                   nsIStreamListener **aDocListener,
>                                   bool aReset,
>                                   nsIContentSink* aSink)
> {
>+  nsCAutoString contentType;
>+  aChannel->GetContentType(contentType);
>+
>   bool viewSource = aCommand && !nsCRT::strcmp(aCommand, "view-source") &&
>     NS_USE_NEW_VIEW_SOURCE;
>-  bool loadAsHtml5 = nsHtml5Module::sEnabled || viewSource;
>+  bool plainText = (contentType.EqualsLiteral(TEXT_PLAIN) ||
>+    contentType.EqualsLiteral(TEXT_CSS) ||
>+    contentType.EqualsLiteral(APPLICATION_JAVASCRIPT) ||
>+    contentType.EqualsLiteral(APPLICATION_XJAVASCRIPT) ||
>+    contentType.EqualsLiteral(TEXT_ECMASCRIPT) ||
>+    contentType.EqualsLiteral(APPLICATION_ECMASCRIPT) ||
>+    contentType.EqualsLiteral(TEXT_JAVASCRIPT));
I was wondering whether this should be in a helper method in nsContentUtils, but since the
old parser is going away, there shouldn't be any need.


>@@ -878,17 +884,17 @@ nsHtml5StreamParser::OnStartRequest(nsIR
>   NS_ASSERTION(!mLastBuffer, "How come we have the last buffer set?");
>   mFirstBuffer = mLastBuffer = newBuf;
> 
>   nsresult rv = NS_OK;
> 
>   // The line below means that the encoding can end up being wrong if
>   // a view-source URL is loaded without having the encoding hint from a
>   // previous normal load in the history.
>-  mReparseForbidden = !(mMode == NORMAL);
>+  mReparseForbidden = !(mMode == NORMAL || mMode == PLAIN_TEXT);
I'd write this mMode != NORMAL && mMode != PLAIN_TEXT. But either way is ok.

>+void StartPlainText();
>+
So there is no EndPlainText(). StartPlainText() sounds a bit strange.
Could you add some comment about that, or rename StartPlainText().


r=me if the test is fixed. I think adding a load event listener to the <browser> element should work there.
Attachment #567087 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 6

6 years ago
Thanks. Landed with the test changed to use a load listener on <browser>.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0ef50b48d3
Target Milestone: --- → mozilla10

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/9a0ef50b48d3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 702104

Updated

6 years ago
Depends on: 702212
You need to log in before you can comment on or make changes to this bug.