Last Comment Bug 479959 - [HTML5] Re-implement text/plain loading using the HTML5 tokenizer
: [HTML5] Re-implement text/plain loading using the HTML5 tokenizer
Status: RESOLVED FIXED
: html5
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Other Branch
: All All
: P3 normal with 1 vote (vote)
: mozilla10
Assigned To: Henri Sivonen (:hsivonen)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 702104 702212
Blocks: 563890 655434 553999 659763
  Show dependency treegraph
 
Reported: 2009-02-24 05:59 PST by Henri Sivonen (:hsivonen)
Modified: 2011-11-14 01:50 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reimplement text/plain (10.63 KB, patch)
2011-05-06 05:50 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Reimplement text/plain, v2 (11.57 KB, patch)
2011-05-09 06:30 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Reimplement text/plain, rebased (11.64 KB, patch)
2011-10-14 07:55 PDT, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review

Description Henri Sivonen (:hsivonen) 2009-02-24 05:59:01 PST
http://www.whatwg.org/specs/web-apps/current-work/#read-text
Comment 1 Daniel.S 2009-02-27 09:35:38 PST
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.
Comment 2 Henri Sivonen (:hsivonen) 2011-05-06 05:50:55 PDT
Created attachment 530600 [details] [diff] [review]
Reimplement text/plain

What test cases should I write for this (if any)?
Comment 3 Henri Sivonen (:hsivonen) 2011-05-09 06:30:34 PDT
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.
Comment 4 Henri Sivonen (:hsivonen) 2011-10-14 07:55:06 PDT
Created attachment 567087 [details] [diff] [review]
Reimplement text/plain, rebased
Comment 5 Olli Pettay [:smaug] 2011-10-14 09:30:03 PDT
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.
Comment 6 Henri Sivonen (:hsivonen) 2011-11-01 08:37:32 PDT
Thanks. Landed with the test changed to use a load listener on <browser>.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0ef50b48d3
Comment 7 Ed Morley [:emorley] 2011-11-02 06:36:41 PDT
https://hg.mozilla.org/mozilla-central/rev/9a0ef50b48d3

Note You need to log in before you can comment on or make changes to this bug.