Last Comment Bug 710175 - Revert to old View Source back end for Firefox 10
: Revert to old View Source back end for Firefox 10
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: 10 Branch
: All All
: -- normal (vote)
: mozilla10
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-13 04:18 PST by Henri Sivonen (:hsivonen)
Modified: 2012-04-18 00:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Go back to using the old View Source back end (7.22 KB, patch)
2011-12-13 04:27 PST, Henri Sivonen (:hsivonen)
bugs: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Henri Sivonen (:hsivonen) 2011-12-13 04:18:40 PST
The new View Source implementation landed so that it went into Aurora with the Firefox 10 release train. Afterwards, regressions were fixed on trunk (for Firefox 11). The choice whether to land the fixes on a branch or whether to go back to the old code on the branch is a call for the release drivers, but this bug is a placeholder for going back to the old code on the branch.
Comment 1 Henri Sivonen (:hsivonen) 2011-12-13 04:27:49 PST
Created attachment 581228 [details] [diff] [review]
Go back to using the old View Source back end

The #defines I had put in place didn't support the configuration where the new parser is used for non-View Source plain text processing but the old parser is used for all kind of View Source. That's why I removed the #defines and hard-coded the desired behavior for this branch-only patch. Note that nsHtml5Module::sEnabled is always true anyway.
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-12-13 04:29:33 PST
Just curious, has there been any major problems in the new view source?
I don't recall any. All the patches I've reviewed should just go to Aurora.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-12-13 04:33:22 PST
Comment on attachment 581228 [details] [diff] [review]
Go back to using the old View Source back end


>   waitForExplicitFinish();
>   openViewSourceWindow(source, function(aWindow) {
>     let gBrowser = aWindow.gBrowser;
> 
>-    todo(gBrowser.contentDocument.title == source, "Correct document title");
>+    ok(gBrowser.contentDocument.title == source, "Correct document title");
Or is this still something to fix in the new view source. Doesn't look too hard.
Comment 4 Henri Sivonen (:hsivonen) 2011-12-13 04:37:13 PST
(In reply to Olli Pettay [:smaug] from comment #2)
> Just curious, has there been any major problems in the new view source?
> I don't recall any. All the patches I've reviewed should just go to Aurora.

Nothing that I'd consider major, but there's one unfixed minor regression still: bug 710142. (I agree that technically the fixes could be landed on Aurora. And the last unfixed regression could probably be fixed on Beta if it doesn't get fixed before Dec 20th.)

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 581228 [details] [diff] [review]
> Go back to using the old View Source back end
> 
> 
> >   waitForExplicitFinish();
> >   openViewSourceWindow(source, function(aWindow) {
> >     let gBrowser = aWindow.gBrowser;
> > 
> >-    todo(gBrowser.contentDocument.title == source, "Correct document title");
> >+    ok(gBrowser.contentDocument.title == source, "Correct document title");
> Or is this still something to fix in the new view source. Doesn't look too
> hard.

This is something that's fixed in the new View Source on trunk but the patch landed after the Aurora uplift.

Thank you for the r+.
Comment 5 Henri Sivonen (:hsivonen) 2011-12-13 05:02:47 PST
Comment on attachment 581228 [details] [diff] [review]
Go back to using the old View Source back end

(This comment has been posted on all the bugs mentioned in this comment, except bug 710142, so that the release drivers see it regardless of the order in which they process approval requests.)

The new View Source implementation landed before Firefox 10 moved to Aurora. Afterwards, a bunch of regressions were identified. Many of the regression fixes didn't land before Firefox 10 moved to Aurora but they have now been fixed on trunk except for bug 710142.

To avoid shipping with regressions, we either need to land all the regression fixes on Aurora for Firefox 10 (followed by a fix for bug 710142 in Beta if it doesn't make it before Dec 20th) or switch back to the old View Source implementation on Aurora. The new View Source implementation provides much better diagnostics for Web developers than the old View Source implementation.

So I'd like to ask the release drivers to either approve bug 535530, bug 699356, bug 699365, bug 700034, bug 700361, bug 703965, bug 704667 and bug 705473 plus bug 695640, which is a non-regression tweak, or to approve bug 710175 for reverting to the old View Source implementation for Firefox 10.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-13 10:56:27 PST
(In reply to Henri Sivonen (:hsivonen) from comment #5)
> So I'd like to ask the release drivers to either approve bug 535530, bug
> 699356, bug 699365, bug 700034, bug 700361, bug 703965, bug 704667 and bug
> 705473 plus bug 695640, which is a non-regression tweak, or to approve bug
> 710175 for reverting to the old View Source implementation for Firefox 10.

Given the number of patches, I think we should revert to the old version on Aurora (i.e. take this patch). We shouldn't generally be doing bug fixing on Aurora (we have in the past for a small number of minor fixes, but all of those bugs combined doesn't seem "small" to me).
Comment 7 christian 2011-12-13 14:37:03 PST
Comment on attachment 581228 [details] [diff] [review]
Go back to using the old View Source back end

[triage comment]
We decided to back out the new view source parser (which is this bug!) rather than take the fixups for Firefox 10.

Approved for Aurora, please land ASAP.
Comment 8 Henri Sivonen (:hsivonen) 2011-12-14 00:39:08 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbda7c5fe59c
Comment 9 j.j. 2011-12-14 19:35:23 PST
Documentation for bug 482921 probably needs to change.
Comment 10 Henri Sivonen (:hsivonen) 2011-12-14 22:44:18 PST
(In reply to j.j. from comment #9)
> Documentation for bug 482921 probably needs to change.

MDN edited accordingly.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:12:34 PST
Marking qa- as I don't think this is something QA needs to verify. Please mark qa+ if this is not the case.

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