Last Comment Bug 699356 - "Source of: [URL]" does not appear in the title bar with HTML5 parser-based View Source
: "Source of: [URL]" does not appear in the title bar with HTML5 parser-based V...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
: 704506 (view as bug list)
Depends on: 702448
Blocks: 482921
  Show dependency treegraph
 
Reported: 2011-11-03 03:29 PDT by Henri Sivonen (:hsivonen)
Modified: 2011-12-14 01:02 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected


Attachments
WIP (11.05 KB, patch)
2011-11-25 06:33 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Review
Add URL to the document title (10.94 KB, patch)
2011-11-30 05:05 PST, Henri Sivonen (:hsivonen)
bugs: review-
Details | Diff | Review
Add URL to the document title, v2 (11.20 KB, patch)
2011-11-30 08:50 PST, Henri Sivonen (:hsivonen)
bugs: review+
christian: approval‑mozilla‑aurora-
Details | Diff | Review

Description Henri Sivonen (:hsivonen) 2011-11-03 03:29:57 PDT
STR:
 1) View Source

Actual results:
The title bar doesn't say "Source of: [URL]"

Expected results:
Expected it to say "Source of: [URL]"
Comment 1 Geoff Lankow (:darktrojan) 2011-11-14 02:48:26 PST
In this code, mURL is never set.
http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5Highlighter.cpp#122
Comment 2 Alice0775 White 2011-11-22 10:01:16 PST
*** Bug 704506 has been marked as a duplicate of this bug. ***
Comment 3 Henri Sivonen (:hsivonen) 2011-11-25 06:33:13 PST
Created attachment 576918 [details] [diff] [review]
WIP

The mochitest fails. It sees the right title element but not the right window title. However, visual inspection shows that the window title looks right.
Comment 4 Henri Sivonen (:hsivonen) 2011-11-30 05:05:27 PST
Created attachment 577928 [details] [diff] [review]
Add URL to the document title

See bug 702448 comment 11 about the mochitest todo that this patch leaves in place.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-30 07:38:28 PST
Comment on attachment 577928 [details] [diff] [review]
Add URL to the document title

>+nsHtml5StreamParser::SetURL(nsIURI* aURL)
>+{
>+  if (aURL) {
>+    nsCOMPtr<nsIURI> temp;
>+    bool isViewSource;
>+    aURL->SchemeIs("view-source", &isViewSource);
>+    if (isViewSource) {
>+      nsCOMPtr<nsINestedURI> nested = do_QueryInterface(aURL);
>+      NS_ASSERTION(nested, "URI with scheme view-source didn't QI to nested!");
Useless assertion. There will be crash anyway pointing to the right place.


>+      nested->GetInnerURI(getter_AddRefs(temp));
>+    } else {
>+      temp = aURL;
>+    }
>+    bool isData;
>+    temp->SchemeIs("data", &isData);
>+    if (isData) {
>+      // Avoid showing potentially huge data: URLs
>+      mURL.AssignLiteral("data:\xE2\x80\xA6");
Please explain what \xE2\x80\xA6 means


>+    /**
>+     * Sets the URL for View Source title. If aURL is a view-source: URL,
>+     * takes the inner URL.
>+     */
>+    void SetURL(nsIURI* aURL);
...
>     /**
>+     * The URL of the document being parsed.
>+     */
>+    nsCString                     mURL;
>+

This is kind of strange. SetURL sets the URL for View Source, but
mURL is about document... some inconsistency here.
Also, since data: url handling is special, mURL is wrong.
Maybe mURLforViewSourceTitle; or something like that, and
rename also SetURL
Comment 6 Henri Sivonen (:hsivonen) 2011-11-30 08:50:01 PST
Created attachment 577981 [details] [diff] [review]
Add URL to the document title, v2

Is it OK to do the same s/aURL/aTitle/ substitution in the already-reviewed bug 703965, too?
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-30 09:09:47 PST
(In reply to Henri Sivonen (:hsivonen) from comment #6)
> Is it OK to do the same s/aURL/aTitle/ substitution in the already-reviewed
> bug 703965, too?

Should be ok.
Comment 8 Henri Sivonen (:hsivonen) 2011-11-30 09:46:27 PST
Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/38814e0bafb9
Comment 9 Matt Brubeck (:mbrubeck) 2011-11-30 11:47:44 PST
Backed out on inbound (along with two other bugs from the same push) because of test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20a24dd3f56b
Comment 10 Geoff Lankow (:darktrojan) 2011-11-30 16:15:07 PST
Re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b03e24707280
Comment 11 Marco Bonardo [::mak] 2011-12-01 04:44:54 PST
https://hg.mozilla.org/mozilla-central/rev/b03e24707280
Comment 12 Henri Sivonen (:hsivonen) 2011-12-13 05:04:30 PST
Comment on attachment 577981 [details] [diff] [review]
Add URL to the document title, v2

(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 13 christian 2011-12-13 14:39:28 PST
Comment on attachment 577981 [details] [diff] [review]
Add URL to the document title, v2

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

Denying for Aurora.
Comment 14 Henri Sivonen (:hsivonen) 2011-12-14 01:02:12 PST
Firefox 10 no longer affected due to bug 710175 landing.

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