Closed Bug 777926 Opened 7 years ago Closed 7 years ago

Default bookmark title for error pages is "Problem loading page"

Categories

(Firefox :: Bookmarks & History, defect, minor)

x86_64
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: pts+bmo, Unassigned)

References

Details

Attachments

(2 files, 4 obsolete files)

If you bookmark a page that's showing an error message (like for offline mode, server not found, etc.) the bookmark gets "Problem loading page" as its title by default.  I run into this problem when cleaning up my open tabs without a network connection.

It would be much more helpful to use the URL or (if available) a title from history.
Attached patch patch (obsolete) — Splinter Review
Taking a wild guess at who should review this.
Attachment #646336 - Flags: review?(mano)
FWIW, instead of calling _uri(webNav.document.documentURI), you should be able to just use webNav.document.documentURIObject.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch using documentURIObject instead (obsolete) — Splinter Review
I didn't know that existed, thanks.
Attachment #646336 - Attachment is obsolete: true
Attachment #646336 - Flags: review?(mano)
Attachment #646370 - Flags: review?(mano)
Comment on attachment 646370 [details] [diff] [review]
using documentURIObject instead

># HG changeset patch
># User Peter Simonyi <ptsimony@csclub.uwaterloo.ca>
># Date 1343341826 14400
># Node ID 7baa914421758a8fc2829a9043dee6e218c445e3
># Parent 98c2a42a3aefb716b570b5e6b746f09e442e3a87
>Bug 777926: use URL or title from history for bookmarking error pages
>
>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -264,17 +264,20 @@ var PlacesCommandHook = {
>       // no DOMWindow (?) but information about the loaded document
>       // may still be obtained from the webNavigation.
>       var webNav = aBrowser.webNavigation;
>       var url = webNav.currentURI;
>       var title;
>       var description;
>       var charset;
>       try {
>-        title = webNav.document.title || url.spec;
>+        let errorPage = url.equals(webNav.document.documentURIObject);

Please adopt the checks we use elsewhere (see BrowserOnClick in browser.js for example).

>+        title = !errorPage ? webNav.document.title
>+                           : PlacesUtils.history.getPageTitle(url));

This seems wrong. If the loaded page (which is not an error page) has an updated empty title, an "old" title shouldn't be used.
Attachment #646370 - Flags: review?(mano) → review-
(In reply to Mano from comment #4)
> Please adopt the checks we use elsewhere (see BrowserOnClick in browser.js
> for example).

So...
+        let errorPage = /^about:(neterror|certerror|blocked)/
+            .test(webNav.document.documentURI);
? The other way seemed more future-proof, but I guess you aren't adding error pages very fast. It was based on the comment for onLocationChange in nsIWebProgressListener.idl [1].
I'll update the patch when I get back.

> >+        title = !errorPage ? webNav.document.title
> >+                           : PlacesUtils.history.getPageTitle(url));
> 
> This seems wrong. If the loaded page (which is not an error page) has an
> updated empty title, an "old" title shouldn't be used.

This should be right. History is consulted only when it's an error page. If the loaded non-error page has an empty title, it will be grabbed from webNav.document.title and then (because blank is falsy) the URL will be used instead, just as without this patch. 

I put the typical case first, but perhaps it would be easier to read the other way around?
> >+        title = errorPage ? PlacesUtils.history.getPageTitle(url))
> >+                          : webNav.document.title;

(P.S. No rush; I'll be offline for the next week and a bit.)

[1] http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl#280
Attached patch patch v3 (obsolete) — Splinter Review
Addressed review comments above.
Attachment #646370 - Attachment is obsolete: true
Attachment #657866 - Flags: review?(mano)
Keywords: checkin-needed
Pushed to Try since I don't see any results posted here.
https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54
The new test is failing at least on OSX.

https://tbpl.mozilla.org/php/getParsedLog.php?id=16641642&tree=Try

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bookmark_titles.js | Offline mode successfully simulated network outage. - Got http://example, expected about:neterror
Stack trace:
    JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 471
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_bookmark_titles.js :: generatorTest :: line 51
    JS frame :: chrome://mochikit/content/browser-test.js :: test_nextStep :: line 518
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
Keywords: checkin-needed
For the record, it fails the same on all platforms.
Attached patch updated test (obsolete) — Splinter Review
So it turns out bug 87717 made localhost accessible in offline mode. (Thanks to mcsmurf on IRC for pointing me to the solution.) My test used offline mode to get an error for a page that is in history (simulating a network outage), so this version temporarily disables the mochitest proxy too.
Attachment #657866 - Attachment is obsolete: true
Attachment #679958 - Flags: review?(mano)
Attached patch patch v5Splinter Review
Apparently, someone registered neterror.com, so I switched to using unregistered-domain.example, which should never be registered. (Also updated patch context.)
Attachment #679958 - Attachment is obsolete: true
Attachment #679958 - Flags: review?(mano)
Attachment #694068 - Flags: review?(mano)
Comment on attachment 694068 [details] [diff] [review]
patch v5

Please test it on mozilla-try before laanding.
Attachment #694068 - Flags: review?(mano) → review+
https://hg.mozilla.org/mozilla-central/rev/59c4ddd0b7a4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Depends on: 825739
You need to log in before you can comment on or make changes to this bug.