482874 - Provide a friendlier/more useful alternative when the user encounters a 404 error page.

Status: NEW

(Core :: DOM: Navigation, enhancement)

Description

[Curtis Bartley [:cbartley]]

When the user encounters a 404 error page, we should provide them with help to resolve the situation.  This could be through an alternative error page similar to the ones provided by Google Chrome and Internet Explorer.  The replacement 404 page could, for example, suggest alternative URLs using the best matches from the Places DB or alternative URLs provided by the Google Link Doctor service, or both.  It might offer other tools that the user could try, such as a pre-populated search box, etc.

Some things to consider:
1. We could try to only present the replacement page when we thing a server default 404 page has been presented (these tend to esecially unhelpful).  There's no authoritative way to tell a server default 404 page, but Google Chrome and Internet Explorer seem to use size as a heuristic -- if the page is no longer than say 400 bytes, it's probably not too helpful.
2. Outright replacing the server-supplied 404 error page -- even in limited circumstances -- is not universally popular.  It might be a good idea to provide easy access to it from the replacement page.  Alternatively, we could show the server-supplied page in all cases, but provide a friendly adjunct UI in the surrounding chrome, e.g. a sidebar.
3. Note that the Google Link Doctor service in particular has potentially serious privacy issues.  One solution might be to require user intervention before we access the service.  Using the Places DB doesn't have this issue, but only offers utility if the user is trying to return to some place they've already been.
4. These approaches might be useful for other network error pages.  It would be nice to have a unified solution.  See bug #479922.

See also http://wiki.mozilla.org/Firefox3.1/Sprints/Network_Error_Pages.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Some quick notes:

1)  The IE heuristic really sucks.   I've known a number of people who basically
    padded out their 404s just to get them over the line.
2)  If we can actually recognize the server-default error pages for Apache and
    IIS, that would be pretty good, I think.  We don't need to handle all web
    servers to be useful.
3)  Fully agreed with everything in Curtis' item 2.
4)  Do we want to treat 404 responses to a url bar load differently from 404
    responses to a link click?  The former is much more likely to be pilot error
    (typos, stray punctuation at the end of a url when copying, etc) and the
    general tenor of the UI should be "if you made a mistake, here's how to fix
    it", whereas the latter is website author error, and we should make it
    clear that the user is not at fault, but they can try other things.  The
    set of desired fixups in those two scenarios might not be identical (and
    might even be disjoint).

ccing the folks mentioned in the wiki, too.

Comment 2

[Curtis Bartley [:cbartley]]

[Technical discussion copied from email.  "bz"=Boris Zbarsky, "cb"=Curtis Bartley]

bz >> Yeah.  In case it wasn't clear, I do think this is a good thing to do; I'm just not willing to take docshell changes this late in the 1.9.1 game, and I think we need to be careful about unilaterally stomping on custom server error pages.  You mention the latter issue explicitly in the bug, and as I said I fully agree with your take on it.

cb >> Trust me when I say I'm not eager to make docshell changes, anyway :-) .  

bz >> Oh, I do.

cb >> The extension [Firefox extension for 404 prototyping] just hooks http-on-examine-response and EndDocumentLoad, which is kind of crude, but does basically work.  Is it possible we could do something like that, without making deep docshell changes?  I suppose that has it's own issues, not to mention the whole L10N thing.

bz >> Right.  And if we want to preserve the security context of the page (without which I fully expect web sites to break), the above approach may not be enough.

cb >> It would open up some interesting possibilities if there was simply a way to tell in EndDocumentLoad what the response status was.

bz >> Are we talking about the "EndDocumentLoad" observer service notification?

bz >> If you can get from there to the document, we could probably toss something on nsIWindowUtils that lets you get that info.  The document certainly knows it.

bz >> That said, you could also observe progress listener notifications yourself and get the information that way, I think.

Comment 3

[Curtis Bartley [:cbartley]]

> bz >> Are we talking about the "EndDocumentLoad" observer service notification?
> 
> bz >> If you can get from there to the document, we could probably toss
> something on nsIWindowUtils that lets you get that info.  The document
> certainly knows it.
> 
> bz >> That said, you could also observe progress listener notifications
> yourself and get the information that way, I think.

I can get the document in EndDocumentLoad, but I couldn't figure out how to get the response status from the document.  The extension gets around this by maintaining a dictionary of 404'd URLs that's updated on http-on-examine-response and the EndDocumentLoad handler checks the document's URL against the dictionary.  Obviously it's better if the document (or window) carries the response status with it rather than figuring out some way to pass the status out-of-band.

Comment 4

[Boris Zbarsky [:bzbarsky]]

There's no scriptable API for this, but nsIDocument has a GetChannel() that can be used to get the channel it was loaded from.  Hence my comments about an nsIDOMWindowUtils API to get that info...

Comment 5

[Curtis Bartley [:cbartley]]

What's the difference between using the observer service/EndDocumentLoad and using a progress listener?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Progress listeners get channels handed to them.

Comment 7

[Curtis Bartley [:cbartley]]

This is my current thinking:

* do not redirect obvious custom server error pages
* when we do redirect, original error page is available at one click.
* use some heuristic beyond simple page size for identifying default 404 pages
    o compare to standard Apache and IIS 404 pages, possibly fuzzy (use edit distance?) 
* use EndDocumentLoad (or other event) + redirect
    o can't do deep docshell changes for 3.1
	+ should unify with existing error page architecture in v.next. 
    o similar to how the Friendly Error extension works now
    o implement listener in C++, should be able to access 404 status directly from document. 
* use 404 specific error page for 3.1 rather than trying to re-use the generic network error page.
    o more design flexibility
    o easier to get in before string freeze
    o unify with generic network error page in v.next 

Notes:

* I'm not sure if a progress listener or an observer service listener (for EndDocumentLoad, etc.) is the best choice.  If the listener is in C++, then -- if I understand correctly -- I can get to the HTTP response status either way.  There might be some other reason to prefer one solution or the other that I don't know yet.

* Driving 404 interception through a progress listener or EndDocumentLoad observer is kind of a hack, although it's a relatively benign one as hacks go.  We'd really want to clean this up in v.next.

* The impending string freeze is the tightest constraint.  I think we might need to land the 404 page independently before there's code to actually show it, and I don't know how people feel about that.

Comment 8

[Curtis Bartley [:cbartley]]

A note on security: The Firefox 404 error page proposed here should not be a chrome page, since that could expose us to a possible privilege escalation attach in the event of a new XSS attach.  I think we're OK as long as we serve the 404 page from an about: URL, similar to how about:neterror works now.

Comment 9

[Johnathan Nightingale [:johnath]]

yeah - the test is not that it is chrome or not (it will be accessible via a chrome url either way, of course) but that when we load it, we make sure it drops privilege.  e.g.

http://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/aboutCertError.js#55

Comment 10

[Curtis Bartley [:cbartley]]

(In reply to comment #9)
> yeah - the test is not that it is chrome or not (it will be accessible via a
> chrome url either way, of course) but that when we load it, we make sure it
> drops privilege.  e.g.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/aboutCertError.js#55

So if I add a new about: page, I need to explicitly drop privilege for it?

Comment 11

[Johnathan Nightingale [:johnath]]

(In reply to comment #10)
> (In reply to comment #9)
> > yeah - the test is not that it is chrome or not (it will be accessible via a
> > chrome url either way, of course) but that when we load it, we make sure it
> > drops privilege.  e.g.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/aboutCertError.js#55
> 
> So if I add a new about: page, I need to explicitly drop privilege for it?

Yeah, but if you're adding a new about page, you're implementing newChannel anyhow, so it's really just those couple extra lines setting the original URI and principal.

Comment 12

[Axel Hecht [:Pike]]

To copy my comment from bug 484313, I don't think we should take this for 1.9.1. 

String changes like this are prone to tweaks for some locales after landing, and this isn't a regression or anything as bad as to risk our string freeze.

Comment 13

[Curtis Bartley [:cbartley]]

Attachment: Entity definitions for new DNS Not Found and 404 Error pages Mk II

Comment 14

[Curtis Bartley [:cbartley]]

Attachment: Initial 404 response interceptor/redirector

This attachment is an initial implementation of a 404 response interceptor/redirector.  The patch is not complete since it doesn't include initialization code or the about:notfounderror changes.

The ns404Handler simple observes EndDocumentLoad events, and when they refer to 404 error responses, the handler simply redirects to about:notfounderror, sending some extra information along in the query string.  The current implementation lives in toolkit/components/places, but that was just an expedient place to put it for initial development.

Before I can proceed, I really need to answer some questions:

1. Is redirection from EndDocumentLoad really the right way to handle this?  Maybe we should look at adding support deeper in the infrastructure for 404 redirection (nsIHttpChannel?).  Now that we're no longer constrained by the 3.5 timeframe, something like this might make more sense.
2. Assuming this is a suitable approach, where does the ns404Handler class really belong?  I would guess docshell, but I'm not sure.
3. How should ns404Handler be initialized?  The current hacky implementation just hooks into nsNavHistory initialization which is wrong, wrong, wrong (but at least simple).  I think nsNavHistory gets initialized from JavaScript.  Do I need a full XPCOM class implementation so I can initialize from JS?  And if so, where in the JS should I initialize from?  Or is there some lower level C++ way to initialize it?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 1. Is redirection from EndDocumentLoad really the right way to handle this? 

I wouldn't think so.  Doing it that way means the 404 page from the server (if any) loads fully, and _then_ your code runs.  The problem is that you want to examine the data from the server-sent 404 page before deciding whether to do your own thing, right?

I doubt we need any HTTP channel changes here; this is all docshell-specific (not generic for all necko consumers), and can be handled in docshell.

I'm not sure about the code location either.  Is the goal for Gecko browsers to handle this uniformly?  If so, docshell is the right place.

Note that you'll need docshell support to do this "right" (e.g. without changing what the url bar shows while showing your error page) in any case.

Comment 16

[Curtis Bartley [:cbartley]]

(In reply to comment #15)
> > 1. Is redirection from EndDocumentLoad really the right way to handle this? 
> 
> I wouldn't think so.  Doing it that way means the 404 page from the server (if
> any) loads fully, and _then_ your code runs.  The problem is that you want to
> examine the data from the server-sent 404 page before deciding whether to do
> your own thing, right?
> 
> I doubt we need any HTTP channel changes here; this is all docshell-specific
> (not generic for all necko consumers), and can be handled in docshell.
> 
> I'm not sure about the code location either.  Is the goal for Gecko browsers to
> handle this uniformly?  If so, docshell is the right place.
> 
> Note that you'll need docshell support to do this "right" (e.g. without
> changing what the url bar shows while showing your error page) in any case.

Do you have any advice as to where in docshell I should be looking?  I don't want to go so far as to say I find that code intimidating -- well, actually, now that I think about it, I do find that code intimidating.  Do we have any docshell documentation around that I should be looking at?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Umm.. documentation is in hort supply.  The existing error page code is largely in nsWebShell.cpp, and in particular in nsWebShell::EndPageLoad.  That would have the same issue with showing the previous error page first, though it would let you avoid the issue with the url bar showing the wrong thing.

If we want to not show the server-provided error page at all, then the right place to hook in is an interesting one.  One option is to do something in nsDSURIContentListener to buffer up the input.  Another option is to employ and content sniffer that will change the type on the incoming data to some type we handle specially (e.g. via a stream converter that puts in the page we want).  The right avenue here will somewhat depend on whether we want the JS in the resulting error page (if any) to run with the principal of the site or with some other principal.  Do we know what we want there?

Comment 18

[Johnathan Nightingale [:johnath]]

(In reply to comment #17)

> The right avenue here will somewhat depend on whether we want the JS in the
> resulting error page (if any) to run with the principal of the site or with
> some other principal.  Do we know what we want there?

I think we have to assume it will run with the principal of the site, don't we? It would be wonderful to have it run with privilege, we could be more helpful that way, but that seems too dangerous: once again we'd be letting any cross-frame injection bug become a privilege escalation.  And I don't think we'd want it running with any other website's principal (e.g. the referring site) either, right?

Comment 19

[Curtis Bartley [:cbartley]]

(In reply to comment #18)
> (In reply to comment #17)
> 
> > The right avenue here will somewhat depend on whether we want the JS in the
> > resulting error page (if any) to run with the principal of the site or with
> > some other principal.  Do we know what we want there?
> 
> I think we have to assume it will run with the principal of the site, don't we?
> It would be wonderful to have it run with privilege, we could be more helpful
> that way, but that seems too dangerous: once again we'd be letting any
> cross-frame injection bug become a privilege escalation.  And I don't think
> we'd want it running with any other website's principal (e.g. the referring
> site) either, right?

I am clearly not diabolical enough to be thinking about this kind of stuff.  It just occurred to me that using the redirection approach like I'm using now can trivially expose user data.  The Places-based suggested links are passed in the query string to the error page, which I think will always be visible to JS in an owner document, regardless of principals.

I think that we want to preserve the original URL just from a usability standpoint (the original URL should show in the location bar, not the URL to our 404 page), which should make this particular problem go away.

This does make me wonder if we should treat 404s/net errors differently if they're getting loaded into an IFRAME.  In the 404 case, maybe it doesn't make sense to ever override the server's error page if we know it's going to be displayed in an IFRAME (assuming we can even tell, which I'm not sure about).

Anyway, we definitely don't want to allow JS in an owning document to be able to reach into one of our 404/net error pages if we're going to be putting user data in there.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I think we have to assume it will run with the principal of the site, don't we?

Why?  That's not what existing error pages do, right?

> And I don't think we'd want it running with any other website's principal

Sure, but we might want it running with a null principal, depending.

> The Places-based suggested links are passed in the
> query string to the error page, which I think will always be visible to JS in
> an owner document, regardless of principals.

location.href cannot be read cross-origin if that's what you're worried about.

> I think that we want to preserve the original URL just from a usability
> standpoint (the original URL should show in the location bar, not the URL to
> our 404 page)

Yes, agreed.

> (assuming we can even tell, which I'm not sure about)

Sure we can.  Certainly at the docshell level or anywhere higher.  We can't tell easily from inside necko, of course.

I have no opinion as to whether 404 in subframes should be handled differently from toplevel 404.  But what are the arguments for handling it differently?

> Anyway, we definitely don't want to allow JS in an owning document to be able
> to reach into one of our 404/net error pages

Agreed.

Comment 21

[Curtis Bartley [:cbartley]]

bz:

After our conversation last week and after reading a lot of code, I have a rough plan for actually writing some.  Here's the plan, such as it is.

I want to watch for HTTP responses which return a 404 status code.  For those responses, I want to install a "shim" stream listener object which will decide if the 404 response should be overridden or not.  If the shim decides to override, it will cancel the operation, and if it decides to continue, it will simply proxy StreamListener events to the original StreamListener, which will be (ultimately, at least) the parser.  I think this part is pretty much what you suggested last week.

I don't remember exactly where you suggested to hook in at, but after some investigation, nsDocumentOpenInfo::OnStartRequest (nsURILoader.cpp) seems like a plausible location.  Superficially, it seems like nsDocumentOpenInfo should be protocol-independent, but in fact it already checks to see if it's getting an HttpChannel, and if it is, it's checking for 204 and 205 status codes.  So adding a 404 test isn't going to make the coupling any worse.

I've still got some open questions:
* Is nsDocumentOpenInfo really the right place to hook in at?
* How do I cancel out of OnDataAvailable or OnStopRequest?  Just return NS_BINDING_ABORTED or some other error code?
* How do I then handle the redirect to about:neterror? (in nsDocShell somewhere?)

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'd hook into this in nsDSURIListener::DoContent instead.  nsDocumentOpenInfo may well not be pumping its data into a docshell and hence not actually showing the resulting page to the user (in which case you don't want to do the override thing, I would assume).

> * How do I cancel out of OnDataAvailable or OnStopRequest?  Just return
> NS_BINDING_ABORTED or some other error code?
> * How do I then handle the redirect to about:neterror? (in nsDocShell
> somewhere?)

These are actually the same question.  nsDocShell::EndPageLoad looks at the status of the channel, and if it's an error status it knows about it loads the appropriate error page.  So you want to add a new error code, return it from your OnDataAvailable or OnStopRequest to cancel the channel, then add code to EndPageLoad, similarly to the way other error codes are handled there.

Comment 23

[Curtis Bartley [:cbartley]]

Attachment: New, improved 404 interceptor/redirector (reference only)

This patch is functional but incomplete -- it uses the existing NS_ERROR_FILE_NOT_FOUND error code which I suspect is really intended for files not found on the local file system.  It just displays the default File-not-found error page right now.

My major questions at this point are:
1. Am I on the right track?
2. Should I introduce a new error code (e.g. NS_ERROR_HTTP_FILE_NOT_FOUND).
3. Should the shim refrain from passing OnStartRequest and OnStopRequest when it's going to redirect anyway? (It always passes them through now, only OnDataAvailable is supressed for redirections)
4. For redirects, does it matter whether we cancel the channel before or after sending OnStopRequest to the parser?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> 1. Am I on the right track?

Looks like it, yes.

> 2. Should I introduce a new error code (e.g. NS_ERROR_HTTP_FILE_NOT_FOUND).

Imo, yes.  If we can pass the http status to the error page, then the right error code might be something more generic than that and just indicate that we got an error http status...

> 3. Should the shim refrain from passing OnStartRequest and OnStopRequest 

That would be a violation of the contract for stream listeners...  You have to pass them through.

> does it matter whether we cancel the channel before or after sending
> OnStopRequest to the parser?

No, as long as the channel status and the status you pass to the parser OnStopRequest match.

The OnStopRequest behavior in the caller is correct: the IDL in nsIRequestObserver explicitly documents that error returns from OnStopRequest are ignored, no?

I have various code-level comments on this too, if desired.

Comment 25

[Curtis Bartley [:cbartley]]

(In reply to comment #24)

> I have various code-level comments on this too, if desired.

Absolutely.

Comment 26

[Boris Zbarsky [:bzbarsky]]

OK, non-exhaustive code comments:

* Why do MAX_REDIRECT_SIZE and BUFFER_SIZE need to be powers of 2?
* Why not just |mReceivedCount += count| in the ODA impl?
* In the next line, why not just check mReceivedCount?
* When exactly would mStorageOutputStream be null in the OnStartRequest, etc
  there?  Should those checks just be asserts?
* Please pass the listener to the constructor (and use an initializer to set
  mMyListener), instead of passing it to Init().  The constructor should assert
  that the listener is not null.
* CopyDataBetweenStreams is reinventing nsIOutputStream.writeFrom.
* I'd rather have an explicit mHaveEnoughData boolean instead of keeping track
  of mReceivedCount and relying on sourceOffset in ODA to decide whether to
  call SendOnDataAvailableForBuffer.
* You want an nsRefPtr<ns404HandlerStreamListener>, not an nsCOMPtr.
* You need to check for Init() failure.
* The lats three lines might be better as:

  NS_RELEASE(*aContentHandler);
  *aContentHandler = shim.forget().get();

Comment 27

[Curtis Bartley [:cbartley]]

* Why do MAX_REDIRECT_SIZE and BUFFER_SIZE need to be powers of 2?

Only BUFFER_SIZE needs to be a power of two.  The only constraint on 
MAX_REDIRECT_SIZE is that it must be no larger than BUFFER_SIZE.  I'll
update the comment to make this clearer.  BUFFER_SIZE has the power-of-2
limit because I'm using it as the "segment" size for nsStorageStream.
(Note that I could just use an arbitrary segment size and an unlimited 
number of segments for my storage stream, and then BUFFER_SIZE would no
longer be needed.)

* Why not just |mReceivedCount += count| in the ODA impl?

  "mReceivedCount = sourceOffset + count;"
and
  "mReceivedCount += count"
  
are equivalent in this case.  I could elaborate on why I chose the first 
rather than the second, but it's probably more useful just to say I'm
happy to use the second if you think that's easier to understand.

* In the next line, why not just check mReceivedCount?

Good catch.  I'll fix that.

* When exactly would mStorageOutputStream be null in the OnStartRequest, etc
  there?  Should those checks just be asserts?

Only if someone forgets to call Init(), so arguably, yes, those should be
assertions.  I've seen other Mozilla code that handled a similar situation
with run-time checks like this, but maybe that code wasn't the best model
to use.

* Please pass the listener to the constructor (and use an initializer to set
  mMyListener), instead of passing it to Init().  The constructor should assert
  that the listener is not null.

Sure.  Actually, how do you feel about

  ns404HandlerStreamListener(nsIStreamListener* listener, nsresult* error)

Returning an error code from a constructor as an out-param seems to be one
of those things that just is not done, but it doesn't seem any worse than
having a separate Init() method, and I'd really like to just get rid of that
Init() method altogether and do all of the initialization in the constructor.

* CopyDataBetweenStreams is reinventing nsIOutputStream.writeFrom.

Will do.  It didn't even occur to me to look for that guy, or I'd have used
it in the first place!

* I'd rather have an explicit mHaveEnoughData boolean instead of keeping track
  of mReceivedCount and relying on sourceOffset in ODA to decide whether to
  call SendOnDataAvailableForBuffer.
  
OK.
  
* You want an nsRefPtr<ns404HandlerStreamListener>, not an nsCOMPtr.

OK.  What's the difference, anyway?

* You need to check for Init() failure.

Oy! (Where are my exceptions?)

* The lats three lines might be better as:

  NS_RELEASE(*aContentHandler);
  *aContentHandler = shim.forget().get();
  
I tried it both ways.  I really think doing it this way

        (*aContentHandler)->Release();
        *aContentHandler = shim;
        (*aContentHandler)->AddRef();

is much less mysterious, even though (maybe because) it exposes the refcounting
mechanism.  YMMV, of course.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> I'm happy to use the second if you think that's easier to understand.

I think it is, yes.  Also, doesn't rely on the channel impl getting the offset right...

> BUFFER_SIZE has the power-of-2 limit because I'm using it as the "segment"
> size for nsStorageStream.

Ah, ok.  Just document that?

> I've seen other Mozilla code that handled a similar situation
> with run-time checks like this, but maybe that code wasn't the best model
> to use.

It really depends on how the class will be used.  Since we plan to have precisely one consumer, there's not much need to guard against misbehaving consumers.

>  ns404HandlerStreamListener(nsIStreamListener* listener, nsresult* error)

I'd be fine with that.

> OK.  What's the difference, anyway?

In brief, nsCOMPtr should only be used for interfaces.  Use nsRefPtr for concrete classes.  That won't stop compiling if you add another interface to the class at some point...

> (Where are my exceptions?)

Late-90s C++ compilers ate them?  ;)

> I really think doing it this way is much less mysterious

I'm fine without the forget().get() part, but do use the NS_RELEASE and NS_ADDREF macros instead of manual method calls.

Comment 29

[Curtis Bartley [:cbartley]]

Attachment: 404 interceptor/redirector Mk. III

* Rewrote the comments for MAX_REDIRECT_SIZE and BUFFER_SIZE to clarify their
relationship and constraints.

* Now just use mReceivedCount in comparison with MAX_REDIRECT_SIZE in 
OnDataAvailable.

* Now just use assertions when checking proper initialization in 
OnStartRequest(), OnDataAvailable(), and OnStopRequest().

* The constructor is now completely responsible for initialization -- the
public Init() method is now gone.  Note that the constructor does delegate
to a private InternalInit() method.  This is because the standard COM macros
like NS_ENSURE_SUCCESS, etc. expect to be used inside a function that returns
an nsresult.  The constructor, of course, has to use an outparam for its
nsresult.
 
* CopyDataBetweenStreams is still present.  It turns out that nsStorageStream
does not actually implement nsIOutputStream.WriteFrom().  I could just
implement WriteFrom() myself, but I haven't done that as yet.

* Now use the "mMayRedirect" boolean to communicate whether we should do a
redirection in OnStopRequest(), rather than testing mReceivedCount directly.

* Now use nsRefPtr for the shim declaration in 
nsDSURIContentListener::DoContent()

* Now check the status after constructing the shim.

* Now use NS_RELEASE and NS_ADDREFF for *aContentHandler in 
nsDSURIContentListener::DoContent().

Comment 30

[Boris Zbarsky [:bzbarsky]]

> * CopyDataBetweenStreams is still present.  It turns out that nsStorageStream
> does not actually implement nsIOutputStream.WriteFrom()

Gah.  Worth fixing that....

Comment 31

[Curtis Bartley [:cbartley]]

Attachment: 404 interceptor/redirector Mk. IV

Implemented nsStorageStream::WriteFrom and used that instead of ns404HandlerStreamListener::CopyDataBetweenStreams, which has been removed.  Note that nsStorageStream::WriteFrom uses the same naive implementation as ns404HandlerStreamListener::CopyDataBetweenStreams originally did where it simply allocates a temporary buffer.

Also note that nsFileOutputStream also does not implement WriteFrom.  I don't know that that matters, but I found it surprising.

Comment 32

[Curtis Bartley [:cbartley]]

I've got some more questions which arise from the extended version of this patch that I'm working on.

1. I want to integrate Places-based alternative URL suggestions.  The problem is that I'm pretty sure this violates some sort of dependency rule, since other apps may have docshell, but not places.  And speaking from a strictly functional standpoint, it was hard to get my prototype code even to link.  Do I need some sort of event-based solution, or a global service, some combination thereof, or something else entirely?

2. Right now the neterror code path is C++ until the about:neterror page itself loads, and the JavaScript there of course runs with minimal privileges.  However, it would be nice to write the new functionality I'm planning in JavaScript rather than C++, if at all possible.  (Frankly, it looks like a lot of the LoadErrorPage-and-friends codepath could be rewritten in JavaScript, but that's probably beyond the scope of the current feature)

3. I'd really like to provide the option of viewing the original 404 error page in cases where we override, but I'm not sure how this could be done cleanly.  My 404 test-bed extension uses a global dictionary of pages not to override, but that's really a crude hack which doesn't seem to be appropriate for a real feature.  (Note that jboriss hasn't signed off on this idea from the UI side anyway, but she does at least seems to be willing to contemplate it.)

Comment 33

[Boris Zbarsky [:bzbarsky]]

For item 1, your obvious options are the following:

1)  An interface and contract declared by docshell and then implemented by places.  Docshell would presumably get a service or instance of that contract, qi to that interface, then make the call to it.  Other embeddings could provide their own implementation of the contract + interface.  This is probably the better approach, depending on what you're doing.
2) Some code in the browser chrome that does something to the error page.

For item 2, approach #2 above seems like the right one....

For item 3, I don't have an obvious suggestion offhand.

Comment 34

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page Mk. V

This patch has grown quite a bit more complex.  There are three major components:

Conditional Interception of 404 webserver responses
---------------------------------------------------

* docshell/base/nsDSURIContentListener.cpp
* docshell/base/nsDocShell.cpp
* netwerk/base/public/nsNetError.h
* browser/locales/en-US/chrome/overrides/appstrings.properties
* dom/locales/en-US/chrome/appstrings.properties
* xpcom/io/nsStorageStream.cpp

These changes are largely the ones from earlier patches carried forward.  The major addition to nsDSURIContentListener.cpp is the ns404HandlerStreamListener, which monitors webserver responses that are 404s.  If the response is no larger than 512 bytes, ns404HandlerStreamListener overrides it by returning the new error code NS_ERROR_HTTP_FILE_NOT_FOUND (defined in nsNetError.h).  If the response is larger, it is displayed normally.

It was necessary to add "httpFileNotFound" entries to the appstrings.properties files since the normal code path looks them up.  The values are not actually used at the moment, however.

nsStorageStream.cpp didn't originally implement the writeFrom() method.  It does now.

Did-you-mean URLs drawn from the Places database
------------------------------------------------

* docshell/base/nsIUrlSuggester.idl
* docshell/base/Makefile.in
* toolkit/components/places/src/nsNavHistory.cpp
* toolkit/components/places/src/nsNavHistory.h
* docshell/base/nsDocShell.cpp

nsDocShell.cpp already has a handle to an nsNavHistory object, through its nsIGlobalHistory2 interface (mGlobalHistory).  When handling NS_ERROR_HTTP_FILE_NOT_FOUND errors, it QIs mGlobalHistory for the nsIUrlSuggester interface.  It then calls a method on this interface to get a list of suggested alternative URLs, returned as an nsIArray of nsIURL objects.

There's logic to just skip this step if nsIUrlSuggester isn't supported or just isn't currently available.

nsDocShell.cpp also contains logic to load the new 404-specific error page (about:neterrornew, see below).  The suggested URLs are passed to the page using the "a=..." query string parameter.

I attempted to define the nsIUrlSuggester interface so that it could be implemented in JavaScript in the future.  I don't know how successful I was, or if it's really all that important.

A new net-error page, currently used only for 404 errors
--------------------------------------------------------

* docshell/resources/content/netErrorNew.xhtml
* docshell/resources/content/background.png
* docshell/base/nsAboutRedirector.cpp
* docshell/build/nsDocShellModule.cpp
* docshell/resources/content/jar.mn

This page is accessed by about:neterrornew.  It takes arguments in the query string just like about:neterror.  It also adds support for a new query string parameter, "a=...", where the argument is a JSON encoded array of URLs (as strings).  This last argument supports "did-you-mean" URL suggestions.

This page now largely resembles jboriss's mocks on the https://wiki.mozilla.org/Firefox3.1/Sprints/Network_Error_Pages sprint page.  It's not complete, and it's not well tested as yet.  It's also full of literal strings, so it's not localization-friendly.

Comment 35

[Dietrich Ayala (:dietrich)]

Comment on attachment 393269 [details] [diff] [review]
404 override and new 404 error page Mk. V

>+  // Execute the statement and read off each result.  Create a URI object for
>+  // each result's URL, and accumlate the URIs in an array.
>+  nsCOMPtr<nsIMutableArray> altUrls = do_CreateInstance(NS_ARRAY_CONTRACTID);

check result here. iirc you can pass rv to do_CreateInstance.

i don't remember offhand how xpconnect exposes nsIArray return vals to script. please file a followup for ensuring this is easy to consume by script callers.

r=me otherwise, on the Places bits.

Comment 36

[Axel Hecht [:Pike]]

Jumping in somewhat in the middle here, do we have a defined feature set for this iteration by now? That should likely go through l10n review, in particular if it touches search. I'd like to invite the l10n community on general feedback on that in the newsgroup, too, to see if any of it is only working for English or latin script or just isn't what's happening in the local internets.

Separate from that I'd like to do a review on the actual patch at some point, too.

Comment 37

[Curtis Bartley [:cbartley]]

Some additional notes on attachment 393269 [details] [diff] [review], 404 override and new 404 error page Mk. V:

Known Deficiencies:
------------------

There are two major deficiencies in this patch that I know need to be addressed before I have something landable:

1. Search engine support should be parameterized and should use the user's preferred search engine, to the extent that it makes sense (e.g. Wikipedia search doesn't, but Yahoo search does.)

2. Strings need to be factored out into a DTD to support localization.  This part is a little bit problematical since we need to do parameterized strings and we can't use properties since the error page needs to run with minimal privileges.  There's also the question of whether we should embed HTML in the localized strings or not (IIRC this is done for some of the strings in the existing netError.xhtml page).  Some of these issues are discussed in more detail on bug 484313.  I don't think this part is rocket-science, but I do think it will take some exploratory programming on my part to find the best solution, plus feedback from the localization people.

Design Decisions:
----------------

Instead of extending the existing "netError.xhtml" page to support 404 errors, I've chosen to implement a new error page, "netErrorNew.xhtml".  The new 404 error page design and functionality is substantially different from the existing error pages.  In addition, even though the fundamental design of the older error pages is simpler in a lot of ways, "netError.xhtml" is nevertheless pretty complex because it needs to support all (or nearly all) of them in a single implementation.  Trying to plug the new (and quite different) 404 error page design into it would only complicate things further.

Additionally, if I were to modify the existing "netError.xhtml" I'd open up the possibility of breaking existing error pages, so they'd need to be tested.  Given the number of them (~20) that's quite a lot of extra testing exposure.  Note that this issue exists even if we only share JavaScript code between the pages.  (Note that CSS and ENTITY definitions are currently shared, although I have been careful not to change the parts of these files that will impact the existing error page).

I also think that the new error page design will undergo some more revision in the post 3.6 time frame before we're happy with it.  For example, see some of the suggestions on bug 479922.  Also there has been some fairly serious discussion of incorporating Google's LinkDoctor URL-correction service, which could have some fairly major UI implications as well.

Ultimately, I think we'd want to get back to a single common error page, or at the very least to a situation where we've aggressively factored out common behavior even if we do have multiple concrete error pages.  However,  I think doing much work along those lines in this patch would be premature.

Comment 38

[Boris Zbarsky [:bzbarsky]]

My one concern with that is that in the meantime (and for this project, that tends to mean "from now in perpetuity) we'll be carrying around duplicated code, and forget to fix bugs in one or the other copy....

Comment 39

[Curtis Bartley [:cbartley]]

Comment on attachment 393269 [details] [diff] [review]
404 override and new 404 error page Mk. V

bz: Can you take a look at the C++-implementation side of things?

johnath: I figure you're the best person to comment on netErrorNew.xhtml.  It's now quite different from netError.xhtml, so it should probably be subjected to some extra scrutiny.

Comment 40

[Boris Zbarsky [:bzbarsky]]

I'll look tomorrow morning.

Per my irc comments, the xhtml name should also probably be more useful than netErrorNew....

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The background image really, really wants to be an SVG, or at the very least a jpeg -- and I'm not just bikeshedding here.  797x580 isn't huge (~1.75MB of data), but the cost of scaling it up will be large, especially if the user resizes the window and the like.  Would much rather render SVG for this.

Comment 42

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #41)
> The background image really, really wants to be an SVG, or at the very least a
> jpeg -- and I'm not just bikeshedding here.  797x580 isn't huge (~1.75MB of
> data), but the cost of scaling it up will be large, especially if the user
> resizes the window and the like.  Would much rather render SVG for this.

Yeah, the image is definitely too large - I'm being told in IRC that it's 5% of Firefox's download size.  We could replace it with a solid color for this patch, but as Vlad said, an SVG could work.  Does anyone have a sense of how big an SVG file is too big?

Comment 43

[Boris Zbarsky [:bzbarsky]]

Other than the issues in comment 38 and comment 40:

Might be worth making BUFFER_SIZE and/or MAX_REDIRECT_SIZE pref-controlled.  In fact, BUFFER_SIZE could be computed from MAX_REDIRECT_SIZE, right?

There should be a way for Gecko consumers to disable this behavior; this could be done with a very large MAX_REDIRECT_SIZE if this were pref-controlled.  On the other hand, that would mean buffering up 404s entirely, so maybe the magic value should be 0, or just have a separate pref.

Was there a reason to manually do NS_IMPL_THREADSAFE_ADDREF/RELEASE and NS_INTERFACE_MAP_* instead of using NS_IMPL_THREADSAFE_ISUPPORTS2?

For that matter, was there a reason this stream listener needs to be threadsafe?

> +  NS_ASSERTION(mMyListener != nsnull, "No StreamListener!");

Better text might be "Must have stream listener" or some such.

> +  NS_ASSERTION(MAX_REDIRECT_SIZE <= BUFFER_SIZE, "Dang it, Bobby!");

And you could use better text here too.  If we stick to having these be constants, you could just make this a PR_STATIC_ASSERT, I think.  If we switch to a pref and computing BUFFER_SIZE, it'll have to stay an NS_ASSERTION.

> +  NS_ASSERTION((BUFFER_SIZE & (BUFFER_SIZE - 1)) == 0, "BUFFER_SIZE must be
> a power of 2");

Again, if BUFFER_SIZE stays constant make this a PR_STATIC_ASSERT.  And watch the 80th column?

> +  if (sourceOffset > 0 && sourceOffset <= MAX_REDIRECT_SIZE) {

You could condition that on the value mMayRedirect used to have at entry into this method.  That'll guarantee that this is the first OnDataAvailable you're passing through, without having to worry about the correctness of sourceOffset.  I thought I mentioned not wanting to rely on that earlier in this bug....

I guess you end up using sourceOffset in SendOnDataAvailableForBuffer too, right?  That seems ok to me; it's a lot less magic.

If SendOnDataAvailableForBuffer returns success, you need to check whether the callee canceled the request before making the other OnDataAvailable call.

>+    // Our caller (nsDocumentOpenInfo::OnStopRequest) always returns NS_OK 
>+    // despite what we return here.

Yes, that's what the API documentation for nsIRequestObserver says it should do.  Please take out this comment; if you want to make it clear why the method is returning NS_OK and canceling directly, reference the API documentation.

Note that the method _should_ in fact return NS_OK.  Better yet, it should cancel the channel as needed before calling mMyListener's OnStopRequest, and then just return whatever said OnStopRequest returns.  If it does this, it should make sure to pass the right status to mMyListener's OnStopRequest.

Presumably you shouldn't try redirecting if NS_FAILED(aStatus), right?  I think it doesn't matter much since the channel will ignore your Cancel() in that case, but...

>+    channel = do_QueryInterface(request);

You don't need this; Cancel is on nsIRequest.

>+static nsresult EncodeUriArrayAsJson(nsIArray *uriArray, nsCString &jsonText)

I'm not that happy with us rolling our own thing here.  Especially because it fails out on totally reasonable URIs (ones containing '\\' or '"').  Reusing something like nsJSON.h would make me happier...

> +    rv = uriArray->QueryElementAt(i, NS_GET_IID(nsIURI), getter_AddRefs(uri));

do_QueryElementAt, please.

> +static nsresult GetAlternativeUrlSuggestions(nsIGlobalHistory2

Why do we need to hardcode the fact that this suggestion service coincides with global history?  A separate contractid is the way to go here, imo; Places can register for both contracts.

If we want to force this to not work if this particular docshell is not hooked up to global history (do we?), then I'd prefer that were an explicit check.

>+    nsIArray SuggestAlternativeUrls(in ACString aUrl);

Is there a reason this doesn't take nsIURI?

And this should probably be nsIAlternateURISuggester, and the method should be named suggestAlternativeURIs.

Is returning a null array allowed if there are no suggestions?  I didn't review the huge background file, but I sort of wonder why we need something like that....  Why is that, exactly?  That file on its own bumps up our download size by something like 5% on Windows.  That doesn't seem desirable.  I talked to boriss, and she's going to try making an SVG version of this; we'll see what the file size looks like there.  That would probably play better with the scaling we do on the image too.

This also needs a better filename, especially if we're sticking it into global/content.  A toolkit peer needs to review that.

>+    <!-- If the location of the favicon is changed here, the FAVICON_ERRORPAGE_URL symbol in
>+         toolkit/components/places/src/nsFaviconService.h should be updated. -->

And this code needs to stay in sync with the other error page....  Need comments in both saying so.

I didn't really review the CSS; I assume that its all needed, etc.

>+      // Error url MUST be formatted like this:
>+      //   moz-neterror:page?e=error&u=url&d=desc

That comment is in fact false for this error page (need &a=etc)

>+      // or optionally, to specify an alternate CSS class to allow for
>+      // custom styling and favicon:
>+      //   moz-neterror:page?e=error&u=url&s=classname&d=desc

This is also false (getCSSClass is unused, right?)

And getDescription() is unused.

The templating stuff cries out to use an actual XUL template, if that's even possible here.  Would doing that make it possible to localize with a DTD, perhaps?  Or is the problem that we need to localize the search url?  I didn't review the template HTML and munging that carefully....

I didn't review the Places changes very carefully.

> nsStorageStream::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval)

This doesn't seem to be quite correct.  You should probably do something like:

  if (count > (mSegmentEnd - mWriteCursor) +
        (mSegmentedBuffer->GetMaxSize() - mSegmentedBuffer->GetSize())) {
    // Trying to write too much data
    return NS_ERROR_UNEXPECTED;
  }

or some such up front.  And maybe file a followup bug on implementing WriteSegments and implementing Write and WriteFrom in terms of it, so you don't have to do the extra buffer copy here?

>+  nsresult rv = inStr->Read(buffer, count, &readCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_TRUE(count == readCount, NS_ERROR_FAILURE);

In general, there's no guarantee that readCount == count there.  You need to deal with the situation when it's not (by doing the Read and Write calls in a loop, decrementing |count| as you go).

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 393269 [details] [diff] [review]
404 override and new 404 error page Mk. V

Oh, and the firefox-branded background probably shouldn't live in docshell/

As far as image size goes, 20-40KB is the ballpark that I was thinking of, if it's feasible.  If we can make it even smaller, great.

Comment 45

[Curtis Bartley [:cbartley]]

(In reply to comment #41)
> The background image really, really wants to be an SVG, or at the very least a
> jpeg -- and I'm not just bikeshedding here.  797x580 isn't huge (~1.75MB of
> data), but the cost of scaling it up will be large, especially if the user
> resizes the window and the like.  Would much rather render SVG for this.

Jboriss and I discussed using SVG from the very beginning.  The reason I didn't investigate that further is I expected we'd get pushback on the design before anybody ever paid any attention to the implementation.

We should be able to generate SVG for the current image relatively easily since I'm pretty sure we've got the original vector artwork for it.

I'm a little less sure how to integrate it into the page design, since bz is suggesting in another comment that it doesn't belong in docshell.  Can we specify an SVG document instead of PNG in CSS?

Comment 46

[Boris Zbarsky [:bzbarsky]]

I'm not sure what you're asking.  Right now you use <img src="something">.  You can just as easily use <object type="image/xml+svg" data="something">.

The docshell comment is just about where it is in the source tree...  Presumably any image in docshell/whatever should be a placeholder and the real image live elsewhere, right?

Comment 47

[Axel Hecht [:Pike]]

Does that background image work for RTL languages?

Comment 48

[Curtis Bartley [:cbartley]]

(In reply to comment #47)
> Does that background image work for RTL languages?

Since the image is essentially decorative, I think it should *work* for RTL languages, although there may be a legitimate question as to whether it's as aesthetically pleasing, since the image isn't symmetrical.

Here's a link to one of jboriss's mocks: https://wiki.mozilla.org/File:Twoplacesmatches.png (this is pretty close to the current implementation).

If we use SVG, it seems like we ought to be able to easily reverse the image for RTL languages if we cared to do so.

Comment 49

[Curtis Bartley [:cbartley]]

(In reply to comment #46)
> I'm not sure what you're asking.  Right now you use <img src="something">.  You
> can just as easily use <object type="image/xml+svg" data="something">.
> 
> The docshell comment is just about where it is in the source tree... 
> Presumably any image in docshell/whatever should be a placeholder and the real
> image live elsewhere, right?

For some reason, I was thinking we might have to embed the SVG source right in XHTML document so they'd by definition have to live in the same location in the source tree, since they'd be in the same file.

I don't know *why* I was thinking that...

Comment 50

[Joe Drew (not getting mail)]

(In reply to comment #48)
> If we use SVG, it seems like we ought to be able to easily reverse the image
> for RTL languages if we cared to do so.

You can do that for an arbitrary image too:

-moz-transform: matrix(-1, 0, 0, 1, 0, 0)

A reflection across the y axis.

Comment 51

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #49)
> For some reason, I was thinking we might have to embed the SVG source right in
> XHTML document so they'd by definition have to live in the same location in the
> source tree, since they'd be in the same file.

Yeah, the image is essentially a Photoshop illustration - it isn't vectorized now and won't easily just convert to them.  Like cbartley, I considered this a placeholder because I didn't expect this to move forward so fast.  Perhaps making a new image that is an SVG would be the best way to proceed.

Comment 52

[Curtis Bartley [:cbartley]]

(In reply to comment #51)

> Yeah, the image is essentially a Photoshop illustration - it isn't vectorized
> now and won't easily just convert to them.  Like cbartley, I considered this a
> placeholder because I didn't expect this to move forward so fast.  Perhaps
> making a new image that is an SVG would be the best way to proceed.

We should talk to the original artist.  Just by the look of it, I find it hard to believe that the original work was anything other than vector-based.

Comment 53

[(no longer active)]

(In reply to comment #47)
> Does that background image work for RTL languages?

Because currently we don't use a mirrored logo for RTL languages, the current design would fit RTL locales just fine.

Comment 54

[Johnathan Nightingale [:johnath]]

Comment on attachment 393269 [details] [diff] [review]
404 override and new 404 error page Mk. V

I might be duplicating some of bz's comments below. The r- is principally because of the localization issues - if we're going to template like this, we should get Axel to comment on it, but I hope we can find a cleaner way, here - this is hard code to read/maintain.

>diff --git a/browser/locales/en-US/chrome/overrides/appstrings.properties b/browser/locales/en-US/chrome/overrides/appstrings.properties
>--- a/browser/locales/en-US/chrome/overrides/appstrings.properties
>+++ b/browser/locales/en-US/chrome/overrides/appstrings.properties
>@@ -31,16 +31,17 @@
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> malformedURI=The URL is not valid and cannot be loaded.
> fileNotFound=Firefox can't find the file at %S.
>+httpFileNotFound=The file %S cannot be found. Please check the location and try again.
> dnsNotFound=Firefox can't find the server at %S.

Since this is the Firefox-specific override, it should follow the style of the other strings in the file, i.e. use the product name, and don't provide supplemental instructions, since that's what the rest of the page is for. e.g. "Firefox can't find the file at %S." 

>+static nsresult EncodeUriArrayAsJson(nsIArray *uriArray, nsCString &jsonText)

This scares me for the reasons it scares Boris.

>+++ b/docshell/resources/content/netErrorNew.xhtml
>@@ -0,0 +1,399 @@
>+<?xml version="1.0" encoding="UTF-8"?>
>+
>+<!DOCTYPE html [
>+  <!ENTITY % htmlDTD
>+    PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
>+    "DTD/xhtml1-strict.dtd">
>+  %htmlDTD;
>+  <!ENTITY % netErrorDTD
>+    SYSTEM "chrome://global/locale/netError.dtd">

Will we be using netError.dtd, once we get the strings here localized?

>+   - Contributor(s):
>+   -   Adam Lock <[e-mail]>
>+   -   William R. Price <[e-mail]>
>+   -   Henrik Skupin <[e-mail]>
>+   -   Jeff Walden <[e-mail]>
>+   -   Johnathan Nightingale <[e-mail]>
>+   -   Ehsan Akhgari <[e-mail]>

 - Curtis Bartley?

>+<html xmlns="http://www.w3.org/1999/xhtml">
>+  <head>
>+    <title>&loadError.label;</title>
>+    <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all" />
>+    <!-- If the location of the favicon is changed here, the FAVICON_ERRORPAGE_URL symbol in
>+         toolkit/components/places/src/nsFaviconService.h should be updated. -->
>+    <!--link rel="icon" type="image/png" id="favicon" href="chrome://global/skin/icons/warning-16.png"/-->

What Boris said, and also, I don't think we want the warning favicon here, do we? Boriss will have ideas, I'm sure.

>+    <style type="text/css"><![CDATA[

Haven't reviewed the CSS to ensure it's all necessary, but:

>+      html {
>+        height: 100%; 
>+      }
>+      body {
>+        height:100%;
>+        margin:0px;
>+        padding:0px;
>+      }

Are these necessary? Does the background clip or something, otherwise?

>+      img.background {

probably .background is fine here? We're not using the class elsewhere, right? Might be a dumb question, but why didn't we do this whole thing as a background image on one of the divs?

>+      #errorLongDesc {
>+      }

?

>+      .suggestions {
>+        padding-left: 1em; 
>+      }

padding-left and padding-right aren't RTL-friendly. -moz-padding-start and -moz-padding-end

>+    <script type="application/x-javascript"><![CDATA[
>+      // Error url MUST be formatted like this:
>+      //   moz-neterror:page?e=error&u=url&d=desc
>+      //
>+      // or optionally, to specify an alternate CSS class to allow for
>+      // custom styling and favicon:
>+      //
>+      //   moz-neterror:page?e=error&u=url&s=classname&d=desc

What boris said - make sure these comments are accurate.

>+
>+      // Note that this file uses document.documentURI to get
>+      // the URL (with the format from above). This is because
>+      // document.location.href gets the current URI off the docshell,
>+      // which is the URL displayed in the location bar, i.e.
>+      // the URI that the user attempted to load.
>+      
>+      function getAlternates()
>+      {
>+        var url = document.documentURI;
>+        var matches = /[?&]a=([^&]*)/.exec(url);
>+        if (!matches || matches.length < 2) return "";
>+        return JSON.parse(decodeURIComponent(matches[1]));
>+      }
>+
>+      function getErrorCode()
>+      {
>+        var url = document.documentURI;
>+        var matches = /[?&]e=([^&]*)/.exec(url);
>+        if (!matches || matches.length < 2) return "";
>+        return decodeURIComponent(matches[1]);
>+      }
>+
>+      function getUrl()
>+      {
>+        var url = document.documentURI;
>+        var matches = /[?&]u=([^&]*)/.exec(url);
>+        if (!matches || matches.length < 2) return "";
>+        return decodeURIComponent(matches[1]);
>+      }
>+
>+      function getCSSClass()
>+      {
>+        var url = document.documentURI;
>+        var matches = url.match(/s\=([^&]+)\&/);
>+        // s is optional, if no match just return nothing
>+        if (!matches || matches.length < 2)
>+          return "";
>+
>+        // parenthetical match is the second entry
>+        return decodeURIComponent(matches[1]);
>+      }
>+
>+      function getDescription()
>+      {
>+        var url = document.documentURI;
>+        var desc = url.search(/d\=/);
>+
>+        // desc == -1 if not found; if so, return an empty string
>+        // instead of what would turn out to be portions of the URI
>+        if (desc == -1)
>+          return "";
>+
>+        return decodeURIComponent(url.slice(desc + 2));
>+      }
>+
>+      function retryThis(buttonEl)
>+      {
>+        // Session history has the URL of the page that failed
>+        // to load, not the one of the error page. So, just call
>+        // reload(), which will also repost POST data correctly.
>+        try {
>+          location.reload();
>+        } catch (e) {
>+          // We probably tried to reload a URI that caused an exception to
>+          // occur;  e.g. a non-existent file.
>+        }
>+
>+        buttonEl.disabled = true;
>+      }
>+
>+      function initPage()
>+      {
>+        var templateElements = loadTemplates("templates");
>+
>+        // Create the suggestions template.
>+        var errorCode = getErrorCode();
>+        var errorElem = createElementFromTemplate(templateElements[errorCode]);
>+        
>+        var errorPageContainer = document.getElementById("errorPageContainer");
>+        errorPageContainer.appendChild(errorElem);
>+
>+        var alternates = getAlternates();
>+
>+        errorElem.elements.firstSuggestion.href = alternates[0];
>+        errorElem.elements.firstSuggestion.textContent = alternates[0];
>+
>+        var otherSuggestions = errorElem.elements.otherSuggestions;
>+        for (var i = 1; i < alternates.length; i++) {
>+          var anotherSuggestion =
>+              createElementFromTemplate(templateElements.anotherSuggestion);
>+ 
>+          otherSuggestions.insertBefore(anotherSuggestion, otherSuggestions.firstChild);
>+          anotherSuggestion.elements.anchor.href = alternates[i];
>+          anotherSuggestion.elements.anchor.textContent = alternates[i];
>+        }
>+
>+        var badUrl = getUrl();
>+        var siteMatch = /[/][/]([^/]*)[/]?(.*)/.exec(badUrl);
>+        if (siteMatch && siteMatch.length > 2) {
>+          var site = siteMatch[1];
>+          var subject = siteMatch[2];
>+          var searchString = "site:" + site + " " + subject;
>+          var searchUrlPrefix = "http://www.google.com/search?q=";
>+          var searchUrl = searchUrlPrefix + encodeURIComponent(searchString);
>+          var siteSearch = createElementFromTemplate(templateElements.siteSearch);
>+          otherSuggestions.appendChild(siteSearch);
>+          siteSearch.elements.site.textContent = site;
>+          siteSearch.elements.anchor.href = searchUrl;
>+          siteSearch.elements.anchor.textContent = subject;
>+        }
>+
>+        var searchEngine = "Google";
>+        var webSearch = createElementFromTemplate(templateElements.webSearch);
>+        webSearch.elements.searchEngine.textContent = searchEngine;
>+        webSearch.elements.input.value = badUrl;
>+        webSearch.elements.input.onkeypress = search;
>+        otherSuggestions.appendChild(webSearch);
>+
>+        // ...................................
>+
>+      }
>+      
>+      function search(keyPressEvent)
>+      {
>+        if (keyPressEvent.keyCode == 13) {
>+          var searchUrlPrefix = "http://www.google.com/search?q=";
>+          var searchUrl = searchUrlPrefix + encodeURIComponent(this.value);
>+          document.location = searchUrl;
>+        }
>+      }
>+ 
>+      function loadTemplates(templatesId)
>+      {
>+        var templates = document.getElementById("templates");
>+        var templateElements = createElementsDictionary(templates, "template-id");
>+        templates.parentNode.removeChild(templates);
>+        return templateElements;
>+      }
>+
>+      function createElementFromTemplate(templateElem)
>+      {
>+        var elem = templateElem.cloneNode(true);
>+        elem.elements = createElementsDictionary(elem, "local-id");
>+        return elem;
>+      }
>+
>+      function createElementsDictionary(rootElem, propName)
>+      {
>+        var dictionary = {};
>+        var elemList = rootElem.getElementsByTagName("*");
>+        for (var i = 0; i < elemList.length; i++) {
>+          var templateId = elemList[i].getAttribute(propName);
>+          if (templateId != null) {
>+            dictionary[templateId] = elemList[i]; 
>+          } 
>+        }
>+        return dictionary;
>+      }
>+
>+    ]]></script>
>+  </head>
>+
>+  <body dir="&locale.dir;">
>+
>+    <img class="background" src="chrome://global/content/background.png"/>

See (possibly dumb!) question above about backgrounds-as-element vs. background-in-css

>+    <div class="top-spacer">

Why a spacer instead of just positioning the div we care about directly?


>+    <!-- ERROR ITEM CONTAINER (removed during loading to avoid bug 39098) -->
>+    <div id="errorContainer">
>+    
>+      <div id="templates" style="display:none">

This template system feels overwrought - we create a bunch of tags we end up removing, mostly just so that we can insert an arbitrary number of elements. I think I'd rather see this done in JS as a series of:

document.createElement("li");
/* set up this <li>, give it the appropriate entity as content, with the appropriate substitutions */
/* append it to the <ul> */

That way the structure of the HTML can directly mimic the resulting structure of the page. 

>+            Go to <a local-id="anchor" href="about:blank"></a>

Obviously all the text here needs to be localized into a DTD.

>+            Search <b local-id="site">www.example.com</b> for 
>+            "<a local-id="anchor" href="about:blank">example</a>"

Regardless of where we end up with the templating, the placeholder <a> tags don't need hrefs, we can just set them in JS as we spin them up.

>+          <li template-id="webSearch" style="white-space:nowrap;position:relative">

Why inline style here?

>+            <li xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+              <xul:hbox style="width:100%;-moz-box-align:center;-moz-user-select:text">
>+                <div flex="0">
>+                  Search <span local-id="searchEngine">Search Engine</span> for&nbsp;
>+                </div>
>+                <div flex="1">
>+                  <input local-id="input" type="text" style="width:90%"></input>
>+                </div>
>+              </xul:hbox>
>+            </li>

Do we need XUL here?

>+      <div id="errorTitlesContainer">
>+        
>+        <h1 id="et_generic">&generic.title;</h1>
>+        <h1 id="et_httpFileNotFound">This page can't be found.</h1>
>+        
>+      </div>
>+      <div id="errorDescriptionsContainer">
>+      </div>

These are vestiges of the netError code that we don't need now - we can just set the title directly, right?

Comment 55

[Curtis Bartley [:cbartley]]

> >+static nsresult EncodeUriArrayAsJson(nsIArray *uriArray, nsCString &jsonText)
> 
> This scares me for the reasons it scares Boris.

I've implemented full JSON encoding of ASCII strings in my local copy.  That should address the most severe complaint about this approach.  That leaves two issues: Why I chose to use JSON encoding, and why I chose to roll my own.

Why did I choose to use JSON encoding?

Basically I need a way to encode a list of URLs so that then I can embed them in the query string for the error page.  This boils down to a choice between an ad hoc encoding scheme or using some existing standard such as JSON.

JSON has the obvious advantage of being really easy to decode on the JS side, and it's also scalable in the sense that if we decide to pass more complicated kinds of data to the error page, then there will be a straighforward way to encode it.  Notably, if and when we support Google LinkDoctor in the future, we will likely be getting JSON from the LinkDoctor service and just passing it on unchanged (with the exception of URI-encoding it, of course).

Why did I choose to roll my own JSON encoding?

Our internal support for JSON seems to be very JS-engine specific.  If there's a function in there that you can call to generate the JSON for C++ string, I can't find it.  (I will talk to the JS guys to make sure that I didn't miss an API, though.)

Since the JSON string format is so simple, I decided it was reasonable to write a simple encoder myself.  I'm not sure that an ad hoc format would really be any easier to encode.

How simple is it?  Well if RFC  


The downside of using JSON is that we don't seem to have a standard way of encoding JSON in C++.  Certainly there's support inside the JS engine for it, but it looks like that support's intended for encoding JavaScript objects, not C++ strings.  (It's possible that I've missed an API function, so I need to follow up with the JS team, and I'll do that shortly.)

If we assume for now that I have to generate the JSON myself, then I have a choice between generating JSON or generating some ad hoc encoding scheme, which seems like a toss up.

Comment 56

[Curtis Bartley [:cbartley]]

(In reply to comment #55)

Just ignore the last three paragraphs of the previous comment.  I accidentally posted it before I was done with final editing.  Sorry for any confusion.

Comment 57

[Curtis Bartley [:cbartley]]

(In reply to comment #54)
> (From update of attachment 393269 [details] [diff] [review])


I'm already working on the next version of the patch which I hope to have up in a couple of days.  In the meantime, I've responded to some of the comments.


> I might be duplicating some of bz's comments below. The r- is principally
> because of the localization issues - if we're going to template like this, we
> should get Axel to comment on it, but I hope we can find a cleaner way, here -
> this is hard code to read/maintain.


I've done a lot more work on the localization issues since this patch was originally uploaded.  I agree about flying it by Axel -- in fact I was already planning to do that.

There is probably a legitimate question of whether I've prematurely generalized this code.  I have found that introducing some sort of templating mechanism for HTML is a net win sooner than one might think, but others may differ on this.


> >diff --git a/browser/locales/en-US/chrome/overrides/appstrings.properti..,.
> >--- a/browser/locales/en-US/chrome/overrides/appstrings.properties
> >+++ b/browser/locales/en-US/chrome/overrides/appstrings.properties
> >@@ -31,16 +31,17 @@
> > # and other provisions required by the GPL or the LGPL. If you do not delete
> > # the provisions above, a recipient may use your version of this file under
> > # the terms of any one of the MPL, the GPL or the LGPL.
> > #
> > # ***** END LICENSE BLOCK *****
> > 
> > malformedURI=The URL is not valid and cannot be loaded.
> > fileNotFound=Firefox can't find the file at %S.
> >+httpFileNotFound=The file %S cannot be found. Please check the location ...
> > dnsNotFound=Firefox can't find the server at %S.
> 
> Since this is the Firefox-specific override, it should follow the style of the
> other strings in the file, i.e. use the product name, and don't provide
> supplemental instructions, since that's what the rest of the page is for. e.g.
> "Firefox can't find the file at %S." 


It might be possible to factor the "Please check the location and try again." part out so it's not defined in this file.  The phrasing, however, is straight from jboriss.  Rephrasing it to match the other strings in this file will kind of defeat the purpose...


> >+static nsresult EncodeUriArrayAsJson(nsIArray *uriArray, nsCString &json...
> 
> This scares me for the reasons it scares Boris.


This seems important enough that I've addressed it in a separate comment.  See comment #55.


> >+++ b/docshell/resources/content/netErrorNew.xhtml
> >@@ -0,0 +1,399 @@
> >+<?xml version="1.0" encoding="UTF-8"?>
> >+
> >+<!DOCTYPE html [
> >+  <!ENTITY % htmlDTD
> >+    PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
> >+    "DTD/xhtml1-strict.dtd">
> >+  %htmlDTD;
> >+  <!ENTITY % netErrorDTD
> >+    SYSTEM "chrome://global/locale/netError.dtd">
> 
> Will we be using netError.dtd, once we get the strings here localized?


Yes.


> >+   - Contributor(s):
> >+   -   Adam Lock <[e-mail]>
> >+   -   William R. Price <[e-mail]>
> >+   -   Henrik Skupin <[e-mail]>
> >+   -   Jeff Walden <[e-mail]>
> >+   -   Johnathan Nightingale <[e-mail]>
> >+   -   Ehsan Akhgari <[e-mail]>
> 
>  - Curtis Bartley?


Yeah, just haven't updated the comment yet.


> >+<html xmlns="http://www.w3.org/1999/xhtml">
> >+  <head>
> >+    <title>&loadError.label;</title>
> >+    <link rel="stylesheet" href="chrome://global/skin/netError.css" ...
> >+    <!-- If the location of the favicon is changed here, the FAVICON_...
> >+         toolkit/components/places/src/nsFaviconService.h should be upd...
> >+    <!--link rel="icon" type="image/png" id="favicon" href="chrome://...
> 
> What Boris said, and also, I don't think we want the warning favicon here, do
> we? Boriss will have ideas, I'm sure.


I haven't actually thought about this.  I think I commented it out just because it was giving me a security error.  I'll consult with jboriss about what we want to do here.


> >+    <style type="text/css"><![CDATA[
> 
> Haven't reviewed the CSS to ensure it's all necessary, but:
> 
> >+      html {
> >+        height: 100%; 
> >+      }
> >+      body {
> >+        height:100%;
> >+        margin:0px;
> >+        padding:0px;
> >+      }
> 
> Are these necessary? Does the background clip or something, otherwise?


This is necessary to get the body to expand to the full size of the window -- otherwise the background image doesn't fill the window.  The height on the "html" element is weird, but necessary.  I believe that's an XHTML vs HTML difference.


> >+      img.background {
> 
> probably .background is fine here? We're not using the class elsewhere, right?


Yeah, the "img" is not really necessary.  I'll probably change this to something like " .document-background" {" or something like that.


> Might be a dumb question, but why didn't we do this whole thing as a backgr...
> image on one of the divs?


-moz-background-size (necessary to fit the background to the size of the window) didn't work when I originally tried this a few weeks ago.  I think it landed not long ago, but I haven't had a chance to go back and try it out.  Anyway, it turns out that this might be moot, since we'll probably be using an SVG background.


> >+      #errorLongDesc {
> >+      }
> 
> ?


There's a lot of snarge left around since the page design is still in flux and I haven't made a final cleanup pass.  A lot of this stuff has already been cleaned up in my local copy.


> >+      .suggestions {
> >+        padding-left: 1em; 
> >+      }
> 
> padding-left and padding-right aren't RTL-friendly. -moz-padding-start and
> -moz-padding-end


Noted.


> >+    <script type="application/x-javascript"><![CDATA[
> >+      // Error url MUST be formatted like this:
> >+      //   moz-neterror:page?e=error&u=url&d=desc
> >+      //
> >+      // or optionally, to specify an alternate CSS class to allow for
> >+      // custom styling and favicon:
> >+      //
> >+      //   moz-neterror:page?e=error&u=url&s=classname&d=desc
> 
> What boris said - make sure these comments are accurate.


Noted.


> >+    <img class="background" src="chrome://global/content/background.png"/>
> 
> See (possibly dumb!) question above about backgrounds-as-element vs.
> background-in-css


As mentioned above, a background image couldn't be used because "-moz-background-size" wasn't yet working when I original wrote this.  We're probably going to switch to an SVG backround anyway.


> >+    <div class="top-spacer">
> 
> Why a spacer instead of just positioning the div we care about directly?


I couldn't get the layout to work right using either padding or margin.  In fact the behavior was so weird that I think I might have been hitting a layout bug, although I didn't investigate that any further.  I just wanted to get something simple that would work.


> 
> >+    <!-- ERROR ITEM CONTAINER (removed during loading to avoid bug 39098)...
> >+    <div id="errorContainer">
> >+    
> >+      <div id="templates" style="display:none">
> 
> This template system feels overwrought - we create a bunch of tags we end up
> removing, mostly just so that we can insert an arbitrary number of elements. I
> think I'd rather see this done in JS as a series of:
> 
> document.createElement("li");
> /* set up this <li>, give it the appropriate entity as content, with the
> appropriate substitutions */
> /* append it to the <ul> */
> 
> That way the structure of the HTML can directly mimic the resulting structure
> of the page. 


The problem with building elements programmatically is that it gets unwieldy very fast if you start doing anything remotely complicated like using nested elements for more formatting control, or heaven forbid, start messing with tables for layout.  It's also harder to understand how CSS applies if you have to mentally execute JavaScript to understand what the DOM actually looks like.

Now that said, I think right now this code is right at the cusp where it's about a toss up which style I use.  But if the HTML gets any more complicated, I think the templating mechanism will be a clear win, and in the meantime, I don't think it really hurts.

(I have code coming to support localization that may unquestionably be overblown, since I think it might be overkill myself. But let's cross that bridge when we come to it.)


> >+            Go to <a local-id="anchor" href="about:blank"></a>
> 
> Obviously all the text here needs to be localized into a DTD.


Check.  I'm already working on that.


> >+            Search <b local-id="site">www.example.com</b> for 
> >+            "<a local-id="anchor" href="about:blank">example</a>"
> 
> Regardless of where we end up with the templating, the placeholder <a> tags
> don't need hrefs, we can just set them in JS as we spin them up.


Noted.


> >+          <li template-id="webSearch" style="white-space:nowrap;po...
> 
> Why inline style here?


It just hasn't been factored out yet.


> >+            <li xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/t...
> >+              <xul:hbox style="width:100%;-moz-box-align:center;-moz-user...
> >+                <div flex="0">
> >+                  Search <span local-id="searchEngine">Search Engine</...
> >+                </div>
> >+                <div flex="1">
> >+                  <input local-id="input" type="text" style="width:90...
> >+                </div>
> >+              </xul:hbox>
> >+            </li>
> 
> Do we need XUL here?


It's not XUL we need, but rather "flex".  Turns out you can do flex just through CSS, so no, we don't need XUL after all.


> >+      <div id="errorTitlesContainer">
> >+        
> >+        <h1 id="et_generic">&generic.title;</h1>
> >+        <h1 id="et_httpFileNotFound">This page can't be found.</h1>
> >+        
> >+      </div>
> >+      <div id="errorDescriptionsContainer">
> >+      </div>
> 
> These are vestiges of the netError code that we don't need now - we can just
> set the title directly, right?

Correct.  I think I've already cleaned this up on my local copy.

Comment 58

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page Mk. VI

(In reply to comment #43)

> Might be worth making BUFFER_SIZE and/or MAX_REDIRECT_SIZE pref-controlled...
> fact, BUFFER_SIZE could be computed from MAX_REDIRECT_SIZE, right?
> 
> There should be a way for Gecko consumers to disable this behavior; this could
> be done with a very large MAX_REDIRECT_SIZE if this were pref-controlled.  On
> the other hand, that would mean buffering up 404s entirely, so maybe the magic
> value should be 0, or just have a separate pref.


Done:

Introduced two new prefs.  These prefs are assigned default values in "all.js".  I'm making no claims about the appropriateness of these names, or even whether they belong under "network".  I'm not sure what ordering is used in "all.js".  It looks like it may simply be chronological, so I just put the assignments at the end.  The prefs and their default values are:

  network.error-pages.http404.override           true
  network.error-pages.http404.max-override-size  512

I introduced a new private static method to nsDSURIContentListener for checking the prefs:

  static PRBool Is404OverrideEnabled(PRInt32 *pMaxOverrideSize);

I also introduced a new private static function to ns404HandlerStreamListener:

  static PRUint32 ns404HandlerStreamListener::NextPowerOfTwo(PRUint32 n);

I replaced the constants

  static const PRUint32 BUFFER_SIZE = 512;
  static const PRUint32 MAX_REDIRECT_SIZE = 512;

with the member variables

  const PRUint32 mMaxRedirectSize;
  const PRUint32 mBufferSize;

I added the "PRUint32 maxRedirectSize" parameter to the ns404HandlerStreamListener constructor, so the "maximum redirect size" is now completely parameterized.


> Was there a reason to manually do NS_IMPL_THREADSAFE_ADDREF/RELEASE and
> NS_INTERFACE_MAP_* instead of using NS_IMPL_THREADSAFE_ISUPPORTS2?
> 
> For that matter, was there a reason this stream listener needs to be
> threadsafe?


Er, no.  It was just whatever class I was using as a model here did it that way, and it didn't occur to me that I had a choice.  Anyway, ns404HandlerStreamListener now uses NS_IMPL_ISUPPORTS2 macro rather than NS_IMPL_THREADSAFE_ADDREF, NS_IMPL_THREADSAFE_RELEASE, and NS_INTERFACE_MAP_XXX macros.


> > +  NS_ASSERTION(mMyListener != nsnull, "No StreamListener!");
> 
> Better text might be "Must have stream listener" or some such.
> 
> > +  NS_ASSERTION(MAX_REDIRECT_SIZE <= BUFFER_SIZE, "Dang it, Bobby!");
> 
> And you could use better text here too.  If we stick to having these be
> constants, you could just make this a PR_STATIC_ASSERT, I think.  If we switch
> to a pref and computing BUFFER_SIZE, it'll have to stay an NS_ASSERTION.
> 
> > +  NS_ASSERTION((BUFFER_SIZE & (BUFFER_SIZE - 1)) == 0, "BUFFER_SIZE must be
> > a power of 2");
> 
> Again, if BUFFER_SIZE stays constant make this a PR_STATIC_ASSERT.  And watch
> the 80th column?


I've improved the error messages.  Still using run-time assertions since I've replaced the constants with regular member variables.


> > +  if (sourceOffset > 0 && sourceOffset <= MAX_REDIRECT_SIZE) {
> 
> You could condition that on the value mMayRedirect used to have at entry into
> this method.  That'll guarantee that this is the first OnDataAvailable you're
> passing through, without having to worry about the correctness of sourceOffset.
>  I thought I mentioned not wanting to rely on that earlier in this bug....
> 
> I guess you end up using sourceOffset in SendOnDataAvailableForBuffer too,
> right?  That seems ok to me; it's a lot less magic.


I now have:

  if (mBufferedCount > 0 && mBufferedCount <= mMaxRedirectSize) {
  
where mBufferedCount is whatever mReceivedCount was at the beginning of this call.


> If SendOnDataAvailableForBuffer returns success, you need to check whether the
> callee canceled the request before making the other OnDataAvailable call.


Done.  But you should double check this code.  In particular, I'm not sure what I should be returning if the request got canceled.  Right now I'm returning NS_OK.


> >+    // Our caller (nsDocumentOpenInfo::OnStopRequest) always returns NS_OK 
> >+    // despite what we return here.
> 
> Yes, that's what the API documentation for nsIRequestObserver says it should
> do.  Please take out this comment; if you want to make it clear why the method
> is returning NS_OK and canceling directly, reference the API documentation.


Comment is gone.


> Note that the method _should_ in fact return NS_OK.  Better yet, it should
> cancel the channel as needed before calling mMyListener's OnStopRequest, and
> then just return whatever said OnStopRequest returns.  If it does this, it
> should make sure to pass the right status to mMyListener's OnStopRequest.


I now have:

  // If it's OK to redirect then do so by canceling the channel.
  if (mMayRedirect) {

    // Note that the real stream listener (presumably the parser) has seen no
    // data yet, and now it's not going to.
    request->Cancel(NS_ERROR_HTTP_FILE_NOT_FOUND);

    // We need to pass this error code to mMyListener->OnStopRequest as well.
    aStatus = NS_ERROR_HTTP_FILE_NOT_FOUND;
  }

  // Pass the OnStopRequest to the real listener.
  return mMyListener->OnStopRequest(request, aCtxt, aStatus);


> Presumably you shouldn't try redirecting if NS_FAILED(aStatus), right?  I ...
> it doesn't matter much since the channel will ignore your Cancel() in that
> case, but...
> 
> >+    channel = do_QueryInterface(request);
> 
> You don't need this; Cancel is on nsIRequest.


Superfluous code is now gone.


> >+static nsresult EncodeUriArrayAsJson(nsIArray *uriArray, nsCString &j...
> 
> I'm not that happy with us rolling our own thing here.  Especially because it
> fails out on totally reasonable URIs (ones containing '\\' or '"').  Reusing
> something like nsJSON.h would make me happier...


I discussed this with Rob Sayre on IRC.  He says there's not really an easy alternative here unless I want to pull apart json.cpp.  I'm pretty sure  I don't want to go that far.

In the meantime, I've implemented full JSON support for all 8-bit characters.  It turns out that you only have to escape characters 0-31, backslash, and double-quote, which is easy enough.  Also see comment #55 where I discuss this issue in more detail.


> > +    rv = uriArray->QueryElementAt(i, NS_GET_IID(nsIURI), getter_AddRefs...
> 
> do_QueryElementAt, please.


do_QueryElementAt requires an nsICollection, but alas, nsArray doesn't implement nsICollection.


> > +static nsresult GetAlternativeUrlSuggestions(nsIGlobalHistory2
> 
> Why do we need to hardcode the fact that this suggestion service coincide...
> global history?  A separate contractid is the way to go here, imo; Places can
> register for both contracts.


Done.


> If we want to force this to not work if this particular docshell is not hooked
> up to global history (do we?), then I'd prefer that were an explicit check.
> 
> >+    nsIArray SuggestAlternativeUrls(in ACString aUrl);
> 
> Is there a reason this doesn't take nsIURI?


Well, there is.  Originally it returned the results in a string as well.  I just failed to maintain orthogonality when I modified it to return the array.  Oh, wait, was that rhetorical question? :-).

It's fixed now.


> And this should probably be nsIAlternateURISuggester, and the method should be
> named suggestAlternativeURIs.


Done.


> Is returning a null array allowed if there are no suggestions?  I didn't r...


The array may be empty, but it will never be null.  I don't think there's a meaningful distinction we can make between null and not-null but empty, anyway.

On the other hand, if there is no URL suggestion service, then we end up without an array at all.  I was returning the empty string for the JSON in that case, but that was definitely wrong.  I'm returning "[]" now.


> the huge background file, but I sort of wonder why we need something like
> that....  Why is that, exactly?  That file on its own bumps up our download
> size by something like 5% on Windows.  That doesn't seem desirable.  I talked
> to boriss, and she's going to try making an SVG version of this; we'll see ...
> the file size looks like there.  That would probably play better with the
> scaling we do on the image too.
> 
> This also needs a better filename, especially if we're sticking it into
> global/content.  A toolkit peer needs to review that.


Boriss is telling me to punt on the background image for now, so I've removed it.  We can certainly come up with something smaller in the future.


> >+    <!-- If the location of the favicon is changed here, the FAVICON_ERR...
> >+         toolkit/components/places/src/nsFaviconService.h should be upd...
> 
> And this code needs to stay in sync with the other error page....  Need
> comments in both saying so.


TO DO!!!!!!!!!!!!!!!!!!!!!


> I didn't really review the CSS; I assume that its all needed, etc.


I've removed a lot and rewritten most of the rest for this latest patch, anyway.


> >+      // Error url MUST be formatted like this:
> >+      //   moz-neterror:page?e=error&u=url&d=desc
> 
> That comment is in fact false for this error page (need &a=etc)


Fixed.


> >+      // or optionally, to specify an alternate CSS class to allow for
> >+      // custom styling and favicon:
> >+      //   moz-neterror:page?e=error&u=url&s=classname&d=desc
> 
> This is also false (getCSSClass is unused, right?)


Fixed.


> And getDescription() is unused.


Gone.


> The templating stuff cries out to use an actual XUL template, if that's even
> possible here.  Would doing that make it possible to localize with a DTD,
> perhaps?  Or is the problem that we need to localize the search url?  I didn't
> review the template HTML and munging that carefully....


Is it possible to use XUL templates for anything other than databinding?  Regardless, I think it will be hard to find an alternative that's simpler than what I have here.  Then there's the question as to whether I need a template system at all.  I'll break this topic out into a separate comment...


> I didn't review the Places changes very carefully.


Dietrich did an informal review and thought it was OK.  It probably deserves another pass since I don't think he scrutinized it line-by-line.


> > nsStorageStream::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint3...
> 
> This doesn't seem to be quite correct.  You should probably do something like:
> 
>   if (count > (mSegmentEnd - mWriteCursor) +
>         (mSegmentedBuffer->GetMaxSize() - mSegmentedBuffer->GetSize())) {
>     // Trying to write too much data
>     return NS_ERROR_UNEXPECTED;
>   }


Done.


> or some such up front.  And maybe file a followup bug on implementing
> WriteSegments and implementing Write and WriteFrom in terms of it, so you ...
> have to do the extra buffer copy here?


Done.  See bug 512016, "nsStorageStream: implement WriteSegments and re-implement Write and WriteFrom using WriteSegments"


> >+  nsresult rv = inStr->Read(buffer, count, &readCount);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  NS_ENSURE_TRUE(count == readCount, NS_ERROR_FAILURE);
> 
> In general, there's no guarantee that readCount == count there.  You need to
> deal with the situation when it's not (by doing the Read and Write calls in a
> loop, decrementing |count| as you go).


Done.

Comment 59

[Curtis Bartley [:cbartley]]

(In reply to comment #58)

> > >+    <!-- If the location of the favicon is changed here, the FAVICON_ERR...
> > >+         toolkit/components/places/src/nsFaviconService.h should be upd...
> > 
> > And this code needs to stay in sync with the other error page....  Need
> > comments in both saying so.
> 
> 
> TO DO!!!!!!!!!!!!!!!!!!!!!

That "TO DO" was supposed to be a reminder to address this comment before I uploaded the patch.  Now I guess it's a TODO for the next patch.  But since I'm on the topic, I have an interesting problem: notFoundError.xhtml can't access the favicon -- it gets a security error.  netError.xhtml can access the favicon file just fine, and it's not clear to me what the difference is.

Plus I still need to consult with jboriss about whether we even want to show the same favicon as newError.xhtml.

And as a general note, there's very little that's left in notFoundError.xhtml that is shared with netError.xhtml.

Comment 61

[Boris Zbarsky [:bzbarsky]]

I'll look at the patch in the next few days, but the JSON format is NOT that simple, as evidenced by the various buggy encoders/decoders I've seen (including the one in the patch I reviewed)...

Comment 62

[Curtis Bartley [:cbartley]]

(In reply to comment #61)
> I'll look at the patch in the next few days, but the JSON format is NOT that
> simple, as evidenced by the various buggy encoders/decoders I've seen
> (including the one in the patch I reviewed)...

Just to be clear, I'm not advocating generation of arbitrary JSON, just specifically an array of strings.  What I meant to say earlier is that the JSON *string* format seems to be pretty simple, at least given my current requirements.

From http://www.ietf.org/rfc/rfc4627.txt:

2.5.  Strings

   The representation of strings is similar to conventions used in the C
   family of programming languages.  A string begins and ends with
   quotation marks.  All Unicode characters may be placed within the
   quotation marks except for the characters that must be escaped:
   quotation mark, reverse solidus, and the control characters (U+0000
   through U+001F).

I agree it would be better if we had common code to generate JSON.  Unfortunately, the existing code in nsJSON.cpp and json.cpp is clearly not intended to be general.  For example nearly every single function in json.cpp takes a JSContext pointer.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Curtis, do you think you can put up an interdiff against the previous patch (except for the image bit)?

Comment 64

[Curtis Bartley [:cbartley]]

Attachment: interdiff: Mk V to Mk VI

Comment 65

[Curtis Bartley [:cbartley]]

Attachment: view original URL (prototype)

We've discussed presenting the user with the ability to see the original server-supplied 404 error page.  This patch is a prototype (or maybe "proof of concept") for how such a feature might be implemented.  The code is pretty ugly, but does more or less work.

The patch extends the current (Mk VI) patch with a way to specify a URL that will 404 and will not be overridden by the Firefox 404 page.  To turn a normal URL into a "non-overridable" URL, the patch simply adds 65536 to the port number.  E.g.

    http://www.almostinfinite.com/foobar
==> http://www.almostinfinite.com:65616/foobar

This is obviously not going to fly for a production implementation.  I'm thinking a new scheme type similar to "view-source:" or "wyciwyg:" is a better approach.  So you might have something like:

    view-original:http://www.almostinfinite.com/foobar

This is probably complicated enough that if we decide to do it, we should probably spin it out into a new bug.

Comment 66

[Johnathan Nightingale [:johnath]]

(In reply to comment #65)
> Created an attachment (id=396791) [details]
> view original URL (prototype)
> 
> We've discussed presenting the user with the ability to see the original
> server-supplied 404 error page.  This patch is a prototype (or maybe "proof of
> concept") for how such a feature might be implemented.  The code is pretty
> ugly, but does more or less work....
> This is probably complicated enough that if we decide to do it, we should
> probably spin it out into a new bug.

I think this is a really important piece of the 404 experience we want to offer, but I agree that it's probably best dealt with as a separate bug, marked as dependent on this one.

Comment 67

[Curtis Bartley [:cbartley]]

Because of the way the error page (notFoundError.xhtml) works, it's necessary to use entities for localization.  There's no general support for parameterizing the text in these entities, so it was necessary for me to roll my own.

I need to explain how this works in the latest patch and see what everybody thinks about the approach.

I'm using the <em> element to represent parameters inside an entity definition.  I chose <em> because it's short and rarely used, and if you really wanted an <em> you can always use <i> instead.  And in the worst case you can always fall back on a <span> with inline style.  You can think of the <em> element as the "Expand Macro" element in its repurposed formulation.

As an example, in "netError.dtd", I have this entity definition:

  <!ENTITY httpFileNotFound.searchSiteFor "Search <em>site</em> for 
  <em>searchUrl</em>">

This corresponding use of this entity is in "notFoundError.xhtml":

  <!-- e.g. "Search <em>site</em> for <em>searchUrl</em>" -->
  <!-- Note that "<em>site</em>" and "<em>searchUrl</em>" -->
  <!-- are parameters and should not be localized. -->
  
  &httpFileNotFound.searchSiteFor;

Doing the parameterization with an element really simplifies the JavaScript on the implementation side.  On the other hand, there's not much difference between 

  <em>searchUrl</em>
  
and

  <em id="searchUrl"/>

The latter is uglier (IMHO) but probably much less likely to get accidentally localized.

The discussion on the now-abandoned Bug 484313 covers some of reasons why parameterization is desirable.

Comment 68

[Axel Hecht [:Pike]]

Not sure, is &locale.dir; still used?

I'd prefer to do this review with some screenshots of the scenarios, not sure how many. It's hard to tell ad-hoc how things will actually display.

I expect that you'll get an r- on the current notFound page, all inline blocks with markups need leading and trailing l10n content, even if it's empty in English. We'll need to do an in-depth review on whether all languages can live with hard-coded whitespace. It might be OK in this case, but we need to check with east asia.

For the templating, I'd prefer <em class="site"/>, and there should be a localization note for each to make sure that doesn't get translated.

The concept of "httpFileNotFound" confuses me, I didn't really understand the reason why we have it, and why it's talking about files, from glancing over this bug.

Comment 69

[Gervase Markham [:gerv]]

Rather than something as complex as an alternative port number or scheme, couldn't we show the original in an iframe above or below the "helpful" version?

Gerv

Comment 70

[Curtis Bartley [:cbartley]]

(In reply to comment #69)
> Rather than something as complex as an alternative port number or scheme,
> couldn't we show the original in an iframe above or below the "helpful"
> version?
> 
> Gerv

Because the 404 override is implemented at such a low level, there's no simple way to turn it off selectively.  We need to figure that part out regardless of what the ultimate view-original-error-page UI looks like.

In regards to the UI for this feature, I'm not opposed to an iframe-type approach, but I think jboriss may differ.  We should probably hold off on any related UI discussion until we can spin out a new bug for this sub-feature, however.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 396043 [details] [diff] [review]
 404 override and new 404 error page Mk. VI

>+++ b/docshell/base/nsDSURIContentListener.cpp	Fri Aug 21 22:27:17 2009 -0700
>+  PRUint32 mBufferedCount = mReceivedCount;

Should probably call the local |bufferedCount| instead.

>+    // We need to pass this error code to mMyListener->OnStopRequest as well.
>+    aStatus = NS_ERROR_HTTP_FILE_NOT_FOUND;

This isn't quite right; aStatus should be the request's status, and if the request already had an error status at entry into this method it'll ignore your Cancel() call, as mentioned above.  You need to either not do your canceling if the aStatus is already failure (probably easiest) or reget the request status into aStatus after canceling.

>+++ b/docshell/base/nsDocShell.cpp	Fri Aug 21 22:27:17 2009 -0700
>+static void EncodeStringAsJson(nsACString &s, nsACString &jsonText)
>+  jsonText.SetLength(0);

.Truncate()

|s| should be const here, right?

> static nsresult GetAlternativeUrlSuggestions(nsIGlobalHistory2 *globalHistory, 
You don't need the |globalHistory| argument anymore, right?  Or do you really want to not ask this other service if the docshell has no globalHistory?

>+static nsresult EncodeUriArrayAsJson(nsIArray *uriArray, nsCString &jsonText)
>+    rv = uriArray->QueryElementAt(i, NS_GET_IID(nsIURI), getter_AddRefs(uri));

do_QueryElementAt, seriously.  Include nsArrayUtils.h to get the nsIArray version.

>+++ b/docshell/base/nsIAlternateURISuggester.idl	Fri Aug 21 22:27:17 2009 -0700
>+    nsIArray suggestAlternativeURIs(in nsIURI aURI);

Still need to document whether this is allowed to return null if the list of suggestions is empty.  I really don't care which way it is all that much, but consumers need to know whether they can assume the return value is non-null on NS_OK.

With the above issues fixed, r=bzbarsky on the docshell stuff and the places component registration; I'm really not qualified to review the details of the places database-querying; please get someone familiar with that code to look at it?

Comment 72

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 396043 [details] [diff] [review]
 404 override and new 404 error page Mk. VI

Hey, i have a couple drive by comments/ideas, not actually a review

Why not making this async, with a callback instead of returning an array of urls, you could just register a callback with the method, and each time results are available you get notified and add them. Actually that could be over-engineered, since you limit to 3 results, but just wanted to point out an opportunity.

>+NS_IMETHODIMP nsNavHistory::SuggestAlternativeURIs(nsIURI* aURI, nsIArray **_retval NS_OUTPARAM)
>+{
>+  mozIStorageConnection* dbConn = GetStorageConnection();

just use mDBConn?

>+  NS_ENSURE_TRUE(dbConn, NS_ERROR_FAILURE);
>+  
>+  nsCAutoString spec;
>+  aURI->GetSpec(spec);
>+
>+  // Build the query.  We have to parameterize the LIKE pattern to avoid an
>+  // overzealous warning about escaping patterns for LIKE.
>+  const nsACString &param0 = spec;                         // parameter for "?1"

why reassigning spec to a new ACString? i'd just pass it to the binding method below, it's not hard to read the query and go down to realize what you're binding.

>+  const nsACString &param1 = NS_LITERAL_CSTRING("%404%");  // parameter for "?2"

Personally i'd prefer these vars to have meaningful names, maybe use NS_NAMES_LITERAL_CSTRING? buti'd just pass an anonymous LITERAL_CSTRING at the binding method below

>+  nsCString query = NS_LITERAL_CSTRING(
>+      "SELECT url, levenshteinDistance(url, ?1) ld, title FROM moz_places"
>+      " WHERE url <> ?1 AND NOT title LIKE ?2 ORDER BY ld LIMIT 3"

you are querying just the disk table, i suppose most of the times this will work as expected, but in some case you could miss some result that has just been added to history and is still in moz_places_temp
Why do you select title you don't use it later, should not be needed, the condition will just work.
actually i can't recall if you can also directly make ORDER BY levenshteinDistance(url, ?1), worth a try, the only thing you need to SELECT ideally is url.

Have you tried this on a big database (something like 100 000 places)? i'm a bit worried for a couple things:
- you order on a custom function, that won't use an index
- the like is going to be slow

i'm not sure those are fixable though... making the thing async could help. Please just take some measurement, or eventually i can do that

>+  );
>+
>+  // Create the SQL statement.
>+  nsCOMPtr<mozIStorageStatement> statement;
>+  nsresult rv = dbConn->CreateStatement(query, getter_AddRefs(statement));

just put the query here, try to preserve Places code style avoiding the above nsCString above

>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Bind the parameters.
>+  statement->BindUTF8StringParameter(0, param0);
>+  statement->BindUTF8StringParameter(1, param1);

check rv?

Comment 73

[(no longer active)]

(In reply to comment #68)
> Not sure, is &locale.dir; still used?

Yes, :moz-locale-dir is only used for XUL.  Neil, please correct me if I'm wrong.

Comment 74

[Neil Deakin]

(In reply to comment #73)

> Yes, :moz-locale-dir is only used for XUL.  Neil, please correct me if I'm
> wrong.

It is currently only useful in XUL, but it should probably be adapted to work
in any kind of document.

Comment 75

[Curtis Bartley [:cbartley]]

Attachment: Annotated screenshot showing how entities are used

The attached screenshot is annotated to show which text fragments correspond to which entity definitions.  It also shows which parts belong to parameters inside the entity definitions.

The relevant entities definitions from netError.dtd:

<!ENTITY httpFileNotFound.title "This page can't be found.">

<!-- LOCALIZATION NOTE (httpFileNotFound.didYouMeanLink) - Do not
translate the
span tag.  It is a parameter which will be expanded on display. -->
<!ENTITY httpFileNotFound.didYouMeanLink "Did you mean: <span
macro='link'/>">

<!ENTITY httpFileNotFound.suggestions "Suggestions:">
<!ENTITY httpFileNotFound.otherSuggestions "Other suggestions:">

<!-- LOCALIZATION NOTE (httpFileNotFound.goToLink) - Do not translate the
span tag.  It is a parameter which will be expanded on display. -->
<!ENTITY httpFileNotFound.goToLink "Go to <span macro='link'/>">

<!-- LOCALIZATION NOTE (httpFileNotFound.searchSiteFor) - Do not translate
the span tags.  They are parameters which will be expanded on display. -->
<!ENTITY httpFileNotFound.searchSiteFor
     "Search <span macro='site'/> for <span macro='searchUrl'/>">

<!-- LOCALIZATION NOTE (httpFileNotFound.searchTheWebBefore) - Do not
translate
the span tag.  It is a parameter which will be expanded on display. -->
<!ENTITY httpFileNotFound.searchTheWebBefore "Search <span
macro='searchEngine'/>:">

<!ENTITY httpFileNotFound.searchTheWebAfter "">

Comment 76

[Curtis Bartley [:cbartley]]

Attachment: Secondary annotation screenshot showing the "no alternative URLs" case

When no alternative URLs are provided, we use the subtitle "Suggestions" rather than "Other suggestions".

Comment 77

[Curtis Bartley [:cbartley]]

(In reply to comment #68)
> Not sure, is &locale.dir; still used?
> 
> I'd prefer to do this review with some screenshots of the scenarios, not sure
> how many. It's hard to tell ad-hoc how things will actually display.

Axel:

Can you take a look at the screenshots in attachment 397764 [details] and attachment 397766 [details] and the associated comment #75?  Let me know if this is the kind of information you guys need to do a proper localization review.

Comment 78

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page Mk. VII

bz      docshell
sdwilsh places
johnath xhtml/css
axel    l10n

breakdown by file (some files are mentioned twice)
==================================================

docshell/bz
-----------
docshell/base/nsIAlternateURISuggester.idl
docshell/base/Makefile.in
docshell/base/nsAboutRedirector.cpp
docshell/base/nsDSURIContentListener.cpp
docshell/base/nsDSURIContentListener.h
docshell/base/nsDocShell.cpp
docshell/build/nsDocShellCID.h
docshell/build/nsDocShellModule.cpp
docshell/resources/content/jar.mn
modules/libpref/src/init/all.js
netwerk/base/public/nsNetError.h
xpcom/io/nsStorageStream.cpp

places/sdwilsh
--------------
docshell/base/nsIAlternateURISuggester.idl
toolkit/components/places/src/nsNavHistory.cpp
toolkit/components/places/src/nsNavHistory.h
toolkit/components/places/src/nsPlacesModule.cpp

xhtml+css/johnath
-----------------
docshell/resources/content/notFoundError.xhtml
browser/base/content/browser.js
browser/locales/en-US/chrome/overrides/netError.dtd
browser/locales/en-US/chrome/overrides/appstrings.properties
dom/locales/en-US/chrome/appstrings.properties

l10n/axel
---------
browser/locales/en-US/chrome/overrides/netError.dtd
browser/locales/en-US/chrome/overrides/appstrings.properties
dom/locales/en-US/chrome/appstrings.properties

Response to bz's previous review comments
=========================================

(In reply to comment #71)
> (From update of attachment 396043 [details] [diff] [review])
> >+++ b/docshell/base/nsDSURIContentListener.cpp	Fri Aug 21 22:27:17 2009 -0700
> >+  PRUint32 mBufferedCount = mReceivedCount;
> 
> Should probably call the local |bufferedCount| instead.


Fixed.


> >+    // We need to pass this error code to mMyListener->OnStopRequest as well.
> >+    aStatus = NS_ERROR_HTTP_FILE_NOT_FOUND;
> 
> This isn't quite right; aStatus should be the request's status, and if the
> request already had an error status at entry into this method it'll ignore your
> Cancel() call, as mentioned above.  You need to either not do your canceling if
> the aStatus is already failure (probably easiest) or reget the request status
> into aStatus after canceling.

OK, I now have:

  // If it's OK to redirect then do so by canceling the channel, unless we're
  // already in an error state.
  if (mMayRedirect && NS_SUCCEEDED(aStatus)) {

    // Note that the real stream listener (presumably the parser) has seen no
    // data yet, and now it's not going to.
    request->Cancel(NS_ERROR_HTTP_FILE_NOT_FOUND);

    // aStatus should reflect the new state of the request when we forward the
    // OnStopRequest call below.
    aStatus = NS_ERROR_HTTP_FILE_NOT_FOUND;
  }


> >+++ b/docshell/base/nsDocShell.cpp	Fri Aug 21 22:27:17 2009 -0700
> >+static void EncodeStringAsJson(nsACString &s, nsACString &jsonText)
> >+  jsonText.SetLength(0);
> 
> .Truncate()


Fixed.


> |s| should be const here, right?


Fixed.


> > static nsresult GetAlternativeUrlSuggestions(nsIGlobalHistory2 *globalHistory, 
> You don't need the |globalHistory| argument anymore, right?  Or do you really
> want to not ask this other service if the docshell has no globalHistory?


Fixed.


> >+static nsresult EncodeUriArrayAsJson(nsIArray *uriArray, nsCString &jsonText)
> >+    rv = uriArray->QueryElementAt(i, NS_GET_IID(nsIURI), getter_AddRefs(uri));
> 
> do_QueryElementAt, seriously.  Include nsArrayUtils.h to get the nsIArray
> version.


Fixed.


> >+++ b/docshell/base/nsIAlternateURISuggester.idl	Fri Aug 21 22:27:17 2009 -0700
> >+    nsIArray suggestAlternativeURIs(in nsIURI aURI);
> 
> Still need to document whether this is allowed to return null if the list of
> suggestions is empty.  I really don't care which way it is all that much, but
> consumers need to know whether they can assume the return value is non-null on
> NS_OK.


Fixed.


> With the above issues fixed, r=bzbarsky on the docshell stuff and the places
> component registration; I'm really not qualified to review the details of the
> places database-querying; please get someone familiar with that code to look at
> it?

Comment 79

[Curtis Bartley [:cbartley]]

Attachment: interdiff: Mk VI to Mk VII (missing 2 lines in all.js)

Comment 80

[Axel Hecht [:Pike]]

Comment on attachment 398517 [details] [diff] [review]
 404 override and new 404 error page Mk. VII

r- with the following:

Nits on the localization files:

I'd rather use 'class' than 'macro' as attribute. Just personal taste on standards and stuff.

More severly, the localization notes need to describe what you're actually doing with the string. How a localizer words the text largely depends on what data you insert.

The real r- goes on the searches, though.

You hardcoded google for both search web and search site. Search web should use the default search engine, and search site probably whatever comes out of bug 482229.

I need to admit that I don't have ideas on how to hook this up. Nor what the expected integration of the site search and non-Firefox apps is.

Comment 81

[Shawn Wilsher :sdwilsh]

Comment on attachment 398517 [details] [diff] [review]
 404 override and new 404 error page Mk. VII

For more detailed comments, please see http://reviews.visophyte.org/r/398517/

on file: toolkit/components/places/src/nsNavHistory.h line 125
>                    , public nsIAlternateURISuggester

I'd much rather see a new object created, in its own file in the
mozilla::places namespace instead of tacking on yet another interface onto
nsNavHistory.cpp.  You could even implement the interface in JavaScript which
we'd actually rather you do.  See nsPlacesAutoComplete.js for an example of a
standalone component in JS that you could model your work after.  This not
being in JS is largely why this is getting r- (the status will be re-evaluated
if there are some architecture concerns which make JS a bad choice here).

If there are technical reasons as to why you shouldn't, then you can accesses
the database in native code like so:
mozIStorageConnection *conn =
nsNavHistory::GetSingleton()->GetStorageConnection();
You should probably check that GetSingleton is non-null too.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5294
>   const PRInt32 maxUrlLength = 80;
>   const PRInt32 maxEditDistance = 10;

We lean towards #define constants in C++, and const in JS.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5297
>   // Get the URI spec.  If it's too long, just return NULL and NS_OK.

It's not clear why we are doing this.  A comment explaining that would be
nice.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5299
>   aURI->GetSpec(spec);

(void) result if you are not checking it please


on file: toolkit/components/places/src/nsNavHistory.cpp line 5308
>   nsCString query = NS_LITERAL_CSTRING(
>       "SELECT url FROM moz_places "
>       " WHERE ABS(LENGTH(url) - LENGTH(?1)) <= ?3 "
>       " AND levenshteinDistance(url || '', ?1 || '') <= ?3 "
>       " AND url <> ?1 "
>       " AND NOT title LIKE ?2 "
>       " ORDER BY levenshteinDistance(url, ?1) LIMIT 3 "
>   );

If you are going to keep using native code, please put the NS_LITERAL_CSTRING
inline with the CreateStatement call like the rest of this file please.


on file: toolkit/components/places/src/nsNavHistory.cpp line 5323
>   rv = statement->BindUTF8StringParameter(0, spec);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = statement->BindUTF8StringParameter(1, NS_LITERAL_CSTRING("%404%"));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = statement->BindInt32Parameter(2, maxEditDistance);
>   NS_ENSURE_SUCCESS(rv, rv);

Moving this to JS means you can bind to named parameters which makes this
easier to read.

Comment 82

[Shawn Wilsher :sdwilsh]

Forgot to ask about tests for the places interface you are adding.

Comment 83

[Dietrich Ayala (:dietrich)]

Comment on attachment 398517 [details] [diff] [review]
 404 override and new 404 error page Mk. VII

per irc, since the additional method on nsNavHistory no longer has dependencies on private code there, please break it out into a separate JS component. sdwilsh can assist you in doing this.

Comment 84

[Curtis Bartley [:cbartley]]

sdwilsh: Can you point me to a good example of a Places service written in JS?

Comment 85

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 398517 [details] [diff] [review]
 404 override and new 404 error page Mk. VII

r=me on the docshell bits

Comment 86

[Shawn Wilsher :sdwilsh]

(In reply to comment #84)
> sdwilsh: Can you point me to a good example of a Places service written in JS?
nsPlacesAutoComplete.js

Comment 87

[Johnathan Nightingale [:johnath]]

Comment on attachment 398517 [details] [diff] [review]
 404 override and new 404 error page Mk. VII

Clearing my review request here. AIUI, this patch will be reworked before I need to take another pass at it.

Comment 88

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v8

bz: The changes to nsDocShell.cpp are pretty mundane, but they need a sanity check at least, especially considering the long delay from v7 to v8.  You may just want to look at the interdiff, which I'll attach shortly.

---

Pike:

(In reply to comment #80)
> (From update of attachment 398517 [details] [diff] [review])
> r- with the following:
> 
> Nits on the localization files:
> 
> I'd rather use 'class' than 'macro' as attribute. Just personal taste on
> standards and stuff.


I need to be able to tell the difference between a macro and an ordinary CSS class.


> More severly, the localization notes need to describe what you're actually
> doing with the string. How a localizer words the text largely depends on what
> data you insert.


I've added examples in the localization notes which should make things clearer.


> The real r- goes on the searches, though.
> 
> You hardcoded google for both search web and search site. Search web should use
> the default search engine, and search site probably whatever comes out of bug
> 482229.
> 
> I need to admit that I don't have ideas on how to hook this up. Nor what the
> expected integration of the site search and non-Firefox apps is.

My current solution probably won't please anyone.  The search controls really only make sense for conventional search engines, which is to say those that attempt to index the entire web.  So Google and Yahoo searches makes sense, but Wikipedia, for example, does not.

This code now uses the user's current search engine if it makes sense.  Otherwise, the search controls are just not shown at all.

OK, so how do we know if a search engine makes sense?  The rule I'm using is that if the search engine supports "site:xxxx" syntax, it's probably a conventional search-the-whole-web sort of engine.  Therefore we can use it for both search controls.

So how do we know if an engine supports site search?  Er, umm, yeah.  Right now I've just got a hard-coded list that boils down to Google and Yahoo.

Like I said, it's probably not going to please anybody.

In an ideal world, each search engine would know whether it supported site search, and if it did it would also know how to build a ready-to-go site search URL.  This ought to be a really easy change to make, but I have this nagging feeling that it's not easy at all, and at the very least should be tackled in a separate bug.

---

sdwilsh:

(In reply to comment #81)
> (From update of attachment 398517 [details] [diff] [review])
> For more detailed comments, please see http://reviews.visophyte.org/r/398517/
> 
> on file: toolkit/components/places/src/nsNavHistory.h line 125
> >                    , public nsIAlternateURISuggester
> 
> I'd much rather see a new object created, in its own file in the
> mozilla::places namespace instead of tacking on yet another interface onto
> nsNavHistory.cpp.  You could even implement the interface in JavaScript which
> we'd actually rather you do.  See nsPlacesAutoComplete.js for an example of a
> standalone component in JS that you could model your work after.  This not
> being in JS is largely why this is getting r- (the status will be re-evaluated
> if there are some architecture concerns which make JS a bad choice here).


Done.  See nsAlternateUrlSuggester.js.


> If there are technical reasons as to why you shouldn't, then you can accesses
> the database in native code like so:
> mozIStorageConnection *conn =
> nsNavHistory::GetSingleton()->GetStorageConnection();
> You should probably check that GetSingleton is non-null too.
> 
> 
> on file: toolkit/components/places/src/nsNavHistory.cpp line 5294
> >   const PRInt32 maxUrlLength = 80;
> >   const PRInt32 maxEditDistance = 10;
> 
> We lean towards #define constants in C++, and const in JS.


It's in JS now.


> on file: toolkit/components/places/src/nsNavHistory.cpp line 5297
> >   // Get the URI spec.  If it's too long, just return NULL and NS_OK.
> 
> It's not clear why we are doing this.  A comment explaining that would be
> nice.


Done.


> on file: toolkit/components/places/src/nsNavHistory.cpp line 5299
> >   aURI->GetSpec(spec);
> 
> (void) result if you are not checking it please


In JS now.


> on file: toolkit/components/places/src/nsNavHistory.cpp line 5308
> >   nsCString query = NS_LITERAL_CSTRING(
> >       "SELECT url FROM moz_places "
> >       " WHERE ABS(LENGTH(url) - LENGTH(?1)) <= ?3 "
> >       " AND levenshteinDistance(url || '', ?1 || '') <= ?3 "
> >       " AND url <> ?1 "
> >       " AND NOT title LIKE ?2 "
> >       " ORDER BY levenshteinDistance(url, ?1) LIMIT 3 "
> >   );
> 
> If you are going to keep using native code, please put the NS_LITERAL_CSTRING
> inline with the CreateStatement call like the rest of this file please.


In JS now.


> on file: toolkit/components/places/src/nsNavHistory.cpp line 5323
> >   rv = statement->BindUTF8StringParameter(0, spec);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   rv = statement->BindUTF8StringParameter(1, NS_LITERAL_CSTRING("%404%"));
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   rv = statement->BindInt32Parameter(2, maxEditDistance);
> >   NS_ENSURE_SUCCESS(rv, rv);
> 
> Moving this to JS means you can bind to named parameters which makes this
> easier to read.


Done.

Comment 89

[Curtis Bartley [:cbartley]]

Attachment: interdiff: v7 to v8

Comment 90

[Staś Małolepszy :stas]

(In reply to comment #88)

> My current solution probably won't please anyone.  The search controls really
> only make sense for conventional search engines, which is to say those that
> attempt to index the entire web.  So Google and Yahoo searches makes sense, but
> Wikipedia, for example, does not.

Would it make sense to use user's keyword.URL pref?

If I'm using a default en-US profile, my keyword.URL is set to google. If I'm using a default Russian profile, my keyword.URL is set to yandex.ru. So it gives us a good default global search engine.

If I changed my default search engine, for example to Wikipedia, my keyword.URL is still the same, pointing to google/yandex.

And whatever my current default search engine is, what keyword.URL gives me is an experience I'm familiar with. Every time I type something in the location bar (which is not a URL), I get search results from the search  specified in this pref. So it would quite consistent to have the 404 page would suggest searching with this engine, too.

Comment 91

[Curtis Bartley [:cbartley]]

(In reply to comment #90)

> Would it make sense to use user's keyword.URL pref?
> 
> If I'm using a default en-US profile, my keyword.URL is set to google. If I'm
> using a default Russian profile, my keyword.URL is set to yandex.ru. So it
> gives us a good default global search engine.
> 
> If I changed my default search engine, for example to Wikipedia, my keyword.URL
> is still the same, pointing to google/yandex.
> 
> And whatever my current default search engine is, what keyword.URL gives me is
> an experience I'm familiar with. Every time I type something in the location
> bar (which is not a URL), I get search results from the search  specified in
> this pref. So it would quite consistent to have the 404 page would suggest
> searching with this engine, too.

I don't think it makes sense to always use keyword.URL.  However, it does seem like a good fallback if the user has chosen a default search engine that we can't reasonably use (e.g. Wikipedia).

In fact, we could probably take it further than that.  If the user's currently chosen search engine matches the keyword.URL (whatever it is), then we assume it's also usable for global web search.  This would be better than just relying on a minimal whitelist.

One problem is there's still no way to tell if site search is supported, and what syntax it uses if it does.  Interestingly, yandex.ru seems to support the same "site:" syntax as Google, Yahoo, and Bing.

I think the ultimate solution here is still extending search engine objects to know a little more about themselves.  However, putting some reliance on keyword.URL might make for a better stop-gap solution.

Comment 92

[Axel Hecht [:Pike]]

The site-search stuff should be in sync with bug 482229.

Not sure what the   does in &httpFileNotFound.searchTheWebBefore; 

Why are the dom strings more webby with
httpFileNotFound=The file %S cannot be found. Please check the location and try again.
than the browser overload ones?

Can the span for the web search service be in searchTheWebAfter? I suspect it could, would make for a good comment. Also, it'd be helpful to get a localization note on how the two play together.

Comment 93

[Curtis Bartley [:cbartley]]

(In reply to comment #92)
> The site-search stuff should be in sync with bug 482229.


Ideally, sure.  It's not clear to me how to do that until 482229 lands. 


> Not sure what the   does in &httpFileNotFound.searchTheWebBefore; 


That's just for extra spacing.  I can handle that in CSS instead.

> Why are the dom strings more webby with
> httpFileNotFound=The file %S cannot be found. Please check the location and try
> again.
> than the browser overload ones?


I'm not sure I understand the question.  What do you mean by "webby", "dom strings", and "browser overload ones"?

Anyway, the "httpFileNotFound" is defined in a property file and the %S is expanded by privileged code (C++, in fact).  The other strings are DTDs which are never touched by privileged code, just by the JavaScript in notFoundError.xhtml, which must be unprivileged.  But I'm not sure if this answers your question.

Note that I can de-parameterize the DTD entities, and just use before/after entity pairs.  This is how searchTheWebBefore and searchTheWebAfter work now (for reasons I'll explain below).


> Can the span for the web search service be in searchTheWebAfter? I suspect it
> could, would make for a good comment. Also, it'd be helpful to get a
> localization note on how the two play together.

The searchTheWeb UI is more complex than the other parts.  In particular I'm using XUL style "flex" which dictates how the HTML must be structured.

The whole thing expands to something like:

  Search Google: ###################

where the "##############" is an INPUT element.

The complication comes from the fact that we really want the INPUT element to use as much horizontal space as is available, dependent on the page width.  The way this is done in the HTML is with some DIV wrapping elements and -moz-flex attributes.  The other alternative would be to use an HTML table, which would be even more complicated.

I also wanted to allow text after the INPUT element.  We don't need this in English, but it's possible that we might in one of the other languages.  This is what searchTheWebAfter is before.  Basically it's structured like:

{searchTheWebBefore} INPUT {searchTheWebAfter}

The web search span needs to go before the INPUT statement, which is why it's not in {searchTheWebAfter}.

I'd really like to be able to plug everything into a single parameterized string, like:

  "Search {searchEngine}: {inputElement}"

or (using my current system):

  "Search <span macro="searchEngine">: <span macro="inputElement">"

or even

  "Search %S: %I"

Because of the flex-based layout requirements, I can't really do it this way, though.

In this case, I suppose it might make sense to just skip parameterization altogether and do:

<!-- LOCALIZATION NOTE (httpFileNotFound.searchTheWeb1, httpFileNotFound.searchTheWeb2, httpFileNotFound.searchTheWeb3):
Example:
         SSSSSS  IIIIIIIIIIIIIIIIIIIII
  Search Google: #####################
  1111111      22                     3333333 

S's denote the current search engine, the I's denote an HTML Input element,
and the 1's, 2's, and 3's denote searchTheWeb1, searchTheWeb2, and
searchTheWeb3, respectively.
-->

<!ENTITY httpFileNotFound.searchTheWeb1 "Search ">
<!ENTITY httpFileNotFound.searchTheWeb2 ": ">
<!ENTITY httpFileNotFound.searchTheWeb3 ""

I don't know, maybe the comment is overkill.

Comment 94

[Axel Hecht [:Pike]]

I wonder if some languages would sound more natural if the UI was like

Search for <input> with Google.

I don't have a concrete example, just wondering.

Regarding the spacing and/or &nbsp;, watch out for RTL.


Regarding the "webby", in browser it is
httpFileNotFound=Firefox can't find the file at %S.
and in dom it is
httpFileNotFound=The file %S cannot be found. Please check the location and try again.

The latter sounds much more like a web browser than the first. Not sure if either should say "file" or "page", too.

I admit that I don't even know where that string actually shows in the UI, though.

Comment 95

[Vlado Valastiak [:wladow]]

(In reply to comment #94)
> I wonder if some languages would sound more natural if the UI was like
> 
> Search for <input> with Google.
> 
> I don't have a concrete example, just wondering.
> 
Looking at http://mxr.mozilla.org/l10n-central/search?string=contextMenuSearchText&find=browser%2Fbrowser.properties&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-central
there are several locales that already used this order in the "Search <engine> for <text>" context menu item, I can see at least sk, cs, ca, it...

Comment 96

[Curtis Bartley [:cbartley]]

(In reply to comment #94)

> Regarding the "webby", in browser it is
> httpFileNotFound=Firefox can't find the file at %S.
> and in dom it is
> httpFileNotFound=The file %S cannot be found. Please check the location and try
> again.
> 
> The latter sounds much more like a web browser than the first. Not sure if
> either should say "file" or "page", too.
> 
> I admit that I don't even know where that string actually shows in the UI,
> though.

After reviewing the code, I don't think either of these two strings is shown in the UI.  They have to exist for some of the generic code in nsDocShell.cpp to work correctly, since it will try to look them up before it realizes it doesn't need them.  The text "This page can't be found" is displayed where they'd expect to be used in the regular network error page.

Comment 97

[Curtis Bartley [:cbartley]]

(In reply to comment #94)
> I wonder if some languages would sound more natural if the UI was like
> 
> Search for <input> with Google.
> 
> I don't have a concrete example, just wondering.


The current approach (as opposed to the alternative I proposed in comment #93), supports this possibility already.

For example, you can do

  <!ENTITY httpFileNotFound.searchTheWebBefore "Search for">
  <!ENTITY httpFileNotFound.searchTheWebAfter "with <span local-id='searchEngine'/>">

just as easily as

  <!ENTITY httpFileNotFound.searchTheWebBefore "Search <span local-id='searchEngine'/>:">
  <!ENTITY httpFileNotFound.searchTheWebAfter "">


> Regarding the spacing and/or &nbsp;, watch out for RTL.


Is there some way to display English RTL for testing purposes?

Comment 98

[Curtis Bartley [:cbartley]]

I've been trying to think of a better way to put parameters into the ENTITY definitions.  One alternative is to factor the parameter SPAN elements out into their own ENTITY definitions.  For example, instead of

  <!ENTITY httpFileNotFound.didYouMeanLink "Did you mean: <span macro='link'/>">

you might have:

  <!ENTITY httpFileNotFound.didYouMeanLink "Did you mean: &httpFNF.linkParam;">

The httpFNF.linkParam ENTITY would need to be defined separately with a LOCALIZATION NOTE that it shouldn't be localized.  (It would just contain the SPAN element from the first example.)

I don't know if this is a real improvement, but I personally find having the HTML embedded in the ENTITY definitions to be kind of noisy.

Comment 99

[Axel Hecht [:Pike]]

Sounds interesting.

As far as l10n is concerned, I'm somewhere between an r- and r+ with comments, I think the next step is to get actual code reviews from the rest of the r? gang on that patch.

Comment 100

[(no longer active)]

(In reply to comment #97)
> Is there some way to display English RTL for testing purposes?

As mentioned on IRC, the Force RTL extension can be used for this task: <https://addons.mozilla.org/en-US/firefox/addon/7438>

Comment 101

[Curtis Bartley [:cbartley]]

Attachment: 404 error page screenshot in LTR

Comment 102

[Curtis Bartley [:cbartley]]

Attachment: 404 error page screenshot in RTL

There's one obvious problem in the line 

  Go to http://www.almostinfinite.com/

where the trailing slash is misplaced.  It looks like this must be a rendering bug.  I can't see any way that my code could be causing this particular problem.

Comment 103

[(no longer active)]

Comment on attachment 427225 [details]
404 error page screenshot in RTL

(In reply to comment #102)
> Created an attachment (id=427225) [details]
> 404 error page screenshot in RTL
> 
> There's one obvious problem in the line 
> 
>   Go to http://www.almostinfinite.com/
> 
> where the trailing slash is misplaced.  It looks like this must be a rendering
> bug.  I can't see any way that my code could be causing this particular
> problem.

This is an actual RTL bug in the page.  The case of this problem is that the slash character is a weak bidi character, which means its placement is affected by the nature of characters before and after it in bidi text.  However, URLs are inherently LTR, so they must be displayed as LTR runs in the middle of RTL text in RTL mode.

The easiest solution here is to wrap URLs in something like this:

<span dir="ltr">url</span>

That code is a no-op for LTR builds, and makes things appear correctly in RTL builds.

Comment 104

[(no longer active)]

As an aside, is it intentional that there is no question mark at the end of the first list item?

Comment 105

[Curtis Bartley [:cbartley]]

(In reply to comment #103)
> (From update of attachment 427225 [details])
> (In reply to comment #102)
> > Created an attachment (id=427225) [details] [details]
> > 404 error page screenshot in RTL
> > 
> > There's one obvious problem in the line 
> > 
> >   Go to http://www.almostinfinite.com/
> > 
> > where the trailing slash is misplaced.  It looks like this must be a rendering
> > bug.  I can't see any way that my code could be causing this particular
> > problem.
> 
> This is an actual RTL bug in the page.  The case of this problem is that the
> slash character is a weak bidi character, which means its placement is affected
> by the nature of characters before and after it in bidi text.  However, URLs
> are inherently LTR, so they must be displayed as LTR runs in the middle of RTL
> text in RTL mode.
> 
> The easiest solution here is to wrap URLs in something like this:
> 
> <span dir="ltr">url</span>
> 
> That code is a no-op for LTR builds, and makes things appear correctly in RTL
> builds.

OK, that will be easy enough to fix.  I assume that "/" is a weak bi-di character because it is used in both LTR and RTL languages?

Comment 106

[Curtis Bartley [:cbartley]]

(In reply to comment #104)
> As an aside, is it intentional that there is no question mark at the end of the
> first list item?

That's an excellent question.  It is intentional, because jboriss's design mocks did it that way.  Except that I went back and looked, and she's got some mocks with question marks and some without.  I'll check with her and see what she wants.

Comment 107

[(no longer active)]

(In reply to comment #105)
> OK, that will be easy enough to fix.  I assume that "/" is a weak bi-di
> character because it is used in both LTR and RTL languages?

Well, not exactly.  It's basically because it's not part of an LTR alphabet, and it has a usage similar to that of punctuation characters in RTL and LTR text.  Actually there are more complex things which can happen in bidi handling (such as character mirroring for things like parenthesis), but it's very complicated (and I don't claim to understand all of it.) See <http://unicode.org/reports/tr9/> for the entire gory details if you're interested.

(In reply to comment #106)
> (In reply to comment #104)
> > As an aside, is it intentional that there is no question mark at the end of the
> > first list item?
> 
> That's an excellent question.  It is intentional, because jboriss's design
> mocks did it that way.  Except that I went back and looked, and she's got some
> mocks with question marks and some without.  I'll check with her and see what
> she wants.

I might not be the best person to comment about English issues, but it seems wrong to me without the question mark.

Comment 108

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #106)
> That's an excellent question.  It is intentional, because jboriss's design
> mocks did it that way.  Except that I went back and looked, and she's got some
> mocks with question marks and some without.  I'll check with her and see what
> she wants.

My recommendation would be no question mark, for a few reasons:

1. The URL could be so long that it would be significantly separated from the body of the question, making it look more a part of the URL than a phrase
2. The question mark would not allow for selecting the last segment of text and pasting it.
3. While Google doing it does not make it right, it's significant that Google always does it this way.  Most users encountering this page will be familiar with Google Link Doctor's method, and the functionality we'd be providing would be exactly the same.  By using the same treatment, we're encouraging users to rely on the behavior they're used to rather than learning a new one.  Also, the fact that Google does this may (untested) mean users are in the habit of highlighting the last portion, as in #2

Comment 109

[(no longer active)]

(In reply to comment #108)
> 2. The question mark would not allow for selecting the last segment of text and
> pasting it.

That is only valid for localizations which don't need to add text after the URL to make a meaningful phrase.  For example, Persian, which is the only other language that I know, will need a trailing text anyway, so I'm not sure if this is a valid assumption.  And we always have the Copy Link Location context menu entry, which should provide the same data in the clipboard.

Comment 110

[Shawn Wilsher :sdwilsh]

Comment on attachment 426919 [details] [diff] [review]
404 override and new 404 error page v8

For a review with more context, see http://reviews.visophyte.org/r/426919/.

on file: docshell/base/nsIAlternateURISuggester.idl line 19
>  * Mozilla Corporation.

nit: Mozilla Foundation


on file: docshell/base/nsIAlternateURISuggester.idl line None

you should be able to just do interface nsIURI; and not include it.


on file: docshell/base/nsIAlternateURISuggester.idl line None

You are going to do a follow-up bug to make this asynchronous, right?


on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line 19
>  * Mozilla Corporation.

nit: Mozilla Foundation


on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None

nit: used named functions please!


on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None

Our style is usually dot at end of first line, and then line up second line
with the Cc.


on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None

NetUtil.newURI(stmt.row.url)


on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None

Just calling finalize here is sufficient.


on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None

NewUtil.newURI(searchEngine.searchForm);


on file: toolkit/components/places/tests/unit/test_suggester.js line 54
>   let newURI = ioService.newURI("http://www.example.com/x", null, null);

NetUtil.newURI


r=sdwilsh with comments addressed

Comment 111

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v9

--- bz and johnath ---

See comment #88.

--- Pike ---

Forgot to replace the   spacer with a CSS solution.  I'll pick that up on the next rev.  (I'm not sure it's an L10N problem since its embedded in the entity definition and can be changed per-locale, but CSS is probably cleaner anyway.)

--- Ehsan ---

(In reply to comment #103)
> (From update of attachment 427225 [details])
> (In reply to comment #102)
> > Created an attachment (id=427225) [details] [details]
> > 404 error page screenshot in RTL
> > 
> > There's one obvious problem in the line 
> > 
> >   Go to http://www.almostinfinite.com/
> > 
> > where the trailing slash is misplaced.  It looks like this must be a rendering
> > bug.  I can't see any way that my code could be causing this particular
> > problem.
> 
> This is an actual RTL bug in the page.  The case of this problem is that the
> slash character is a weak bidi character, which means its placement is affected
> by the nature of characters before and after it in bidi text.  However, URLs
> are inherently LTR, so they must be displayed as LTR runs in the middle of RTL
> text in RTL mode.
> 
> The easiest solution here is to wrap URLs in something like this:
> 
> <span dir="ltr">url</span>
> 
> That code is a no-op for LTR builds, and makes things appear correctly in RTL
> builds.

I've hardwired two anchor elements and the INPUT element to be 'dir="ltr"'.  As long as we're preloading the INPUT element with a URL, I think we pretty much need to force it to LTR.  The other alternative is to tear the URL into pieces, e.g. "http://www.almostinfinite.com/track" might get reduced to "www.almostinfinite.com track", which is how the search engine is likely to treat it anyway.  Then the hardwiring won't be necessary, which will work better if the user wants to type other words into the search box.  (Which we're pretty much encouraging them to do anyway.)  Hmm, is "." a weak bi-di character as well?

I've added comments to notFoundError.xhtml explaining the reason for forcing LTR on some elements.  You should probably take a look at the comments and see if they make sense.

I'll attach a new RTL screenshot as well.

--- Sdwilsh ---


(In reply to comment #110)
> (From update of attachment 426919 [details] [diff] [review])
> For a review with more context, see http://reviews.visophyte.org/r/426919/.
> 
> on file: docshell/base/nsIAlternateURISuggester.idl line 19
> >  * Mozilla Corporation.
> 
> nit: Mozilla Foundation


Done.


> on file: docshell/base/nsIAlternateURISuggester.idl line None
> 
> you should be able to just do interface nsIURI; and not include it.


Done.


> on file: docshell/base/nsIAlternateURISuggester.idl line None
> 
> You are going to do a follow-up bug to make this asynchronous, right?


An async solution would be quite difficult to do with the current error page architecture.  The error page is unprivileged, and all the interesting data, including the URL suggestions, have to be passed in using the query string.  I do have a plan for making the query itself substantially faster... 


> on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line 19
> >  * Mozilla Corporation.
> 
> nit: Mozilla Foundation


Done.


> on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None
> 
> nit: used named functions please!


Done.


> on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None
> 
> Our style is usually dot at end of first line, and then line up second line
> with the Cc.


Done.


> on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None
> 
> NetUtil.newURI(stmt.row.url)


Done.


> on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None
> 
> Just calling finalize here is sufficient.


Done.


> on file: toolkit/components/places/src/nsAlternateUrlSuggester.js line None
> 
> NewUtil.newURI(searchEngine.searchForm);


Done.


> on file: toolkit/components/places/tests/unit/test_suggester.js line 54
> >   let newURI = ioService.newURI("http://www.example.com/x", null, null);
> 
> NetUtil.newURI


Done.


> r=sdwilsh with comments addressed

Comment 112

[Curtis Bartley [:cbartley]]

Attachment: interdiff: v8 to v9

Comment 113

[Curtis Bartley [:cbartley]]

Attachment: 404 error page LTR/RTL screenshot v9

Comment 114

[(no longer active)]

Comment on attachment 427686 [details] [diff] [review]
404 override and new 404 error page v9

>+      .nfe-web-search-input {
>+         width:100%;

Please add |text-align: start;| here so that the alignment is set to right in RTL mode.

>+      }

rtl-r=me with that fixed.

Comment 115

[AKIHIRO Misaki (a.k.a Kuden)]

(In reply to comment #111)
> Created an attachment (id=427686) [details]
> 404 override and new 404 error page v9

+    <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all" />

Please make chrome://global/skin/notFoundError.css. And, CSS style rules in notFoundError.xhtml are moved from notFoundError.xhtml to notFoundError.css. 

Because, netError.css style rules are different in each third party themes. 
Theming by third party themes is not easy under the present situation.

Comment 116

[Curtis Bartley [:cbartley]]

(In reply to comment #114)
> (From update of attachment 427686 [details] [diff] [review])
> >+      .nfe-web-search-input {
> >+         width:100%;
> 
> Please add |text-align: start;| here so that the alignment is set to right in
> RTL mode.
> 
> >+      }
> 
> rtl-r=me with that fixed.

Right now I'm forcing the INPUT to LTR since I'm pre-loading it with a URL.  As a consequence, |text-align: start;| doesn't make any difference.

So do I want to a) not force it to LTR, or b) force it to LTR and figure out some way to right-align it when the rest of the page is in RTL?

Comment 117

[Steffen Wilberg]

Comment on attachment 427686 [details] [diff] [review]
404 override and new 404 error page v9

>--- a/docshell/base/nsAboutRedirector.cpp	Thu Feb 18 19:03:52 2010 -0500
>     { "neterror", "chrome://global/content/netError.xhtml",
>       nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
>       nsIAboutModule::ALLOW_SCRIPT |
>       nsIAboutModule::HIDE_FROM_ABOUTABOUT },
>     { "memory", "chrome://global/content/aboutMemory.xhtml",
>-      nsIAboutModule::ALLOW_SCRIPT }
>+      nsIAboutModule::ALLOW_SCRIPT },
>+    { "notfounderror", "chrome://global/content/notFoundError.xhtml",
>+      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
>+      nsIAboutModule::ALLOW_SCRIPT },
You might want to add the nsIAboutModule::HIDE_FROM_ABOUTABOUT flag to hide about:notfounderror from about:about, since it's probably not useful when called without a query string, just like about:neterror and about:certerror.

Comment 118

[(no longer active)]

(In reply to comment #116)
> (In reply to comment #114)
> > (From update of attachment 427686 [details] [diff] [review] [details])
> > >+      .nfe-web-search-input {
> > >+         width:100%;
> > 
> > Please add |text-align: start;| here so that the alignment is set to right in
> > RTL mode.
> > 
> > >+      }
> > 
> > rtl-r=me with that fixed.
> 
> Right now I'm forcing the INPUT to LTR since I'm pre-loading it with a URL.  As
> a consequence, |text-align: start;| doesn't make any difference.
> 
> So do I want to a) not force it to LTR, or b) force it to LTR and figure out
> some way to right-align it when the rest of the page is in RTL?

Oh, no, sorry, I mistyped.  I meant to say:

Please add |text-align: left;| here so that the text alignment is set to left in RTL mode.

I didn't do a very good job there, did I?  :-)

Example of the effect I want to achieve:
data:text/html,<input style="direction:rtl;text-align:left;" value="I'm right to left!">

Comment 119

[Mike Beltzner [:beltzner, not reading bugmail]]

Curtis: are you still waiting for further reviews or do these comments make your existing set of patches obsolete? Not sure that's clear to the people who are marked as review? on the patches.

Comment 120

[Curtis Bartley [:cbartley]]

(In reply to comment #119)
> Curtis: are you still waiting for further reviews or do these comments make
> your existing set of patches obsolete? Not sure that's clear to the people who
> are marked as review? on the patches.

I still need a review of docshell/resources/content/notFoundError.xhtml.

The review request flag is still set to Johnath, but I know from talking to him in IRC that he doesn't think he's the right person to review the changes here.  On the other hand, if Johnath is not the right person, then, frankly I'm not certain who is.

There are two parts to the notFoundError.xhtml review.  The first is simply "is the code OK".  The second is "is the approach OK", especially with regards to how the search box is handled.  I think Pike is waiting for review feedback on how the search is handled (e.g. what engine do we use, etc.) before final L10N approval.

I have a few changes to nsDocShell.cpp that bz (most likely) will need to review.  I don't regard these as serious changes, so I've been waiting to bug him about that until the XHTML changes are reviewed.

I have one outstanding RTL change I need for Ehsan's R+ (which technically he has already given me).  I haven't done that yet just because I didn't want to roll another patch for a one line change.  (I expect I'll need at least one more patch for XHTML issues anyway.)

Comment 121

[Curtis Bartley [:cbartley]]

Comment on attachment 427686 [details] [diff] [review]
404 override and new 404 error page v9

jst: Can you review docshell/resources/content/notFoundError.xhtml or can you recommend someone who can?

Comment 122

[Johnny Stenback (:jst)]

Comment on attachment 427686 [details] [diff] [review]
404 override and new 404 error page v9

r=jst for notFoundError.xhtml

Comment 123

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v10

This patch contains Ehsan's last requested RTL change.  Otherwise it's the same as the v9 patch.

bz: I added a fair amount of code to nsDocShell.cpp since your r+ last year.  All it does is plumb some more data through to the error page, but since it's C++ code and it's in docshell, it probably needs a further review.

Pike: I think the outstanding L10N question is whether the search box in the 404 error page has been adequately parameterized.  Is that right?

Comment 124

[(no longer active)]

The patch looks good RTL-wise, FWIW.

Comment 125

[Axel Hecht [:Pike]]

Comment on attachment 439721 [details] [diff] [review]
404 override and new 404 error page v10

r=me with nits.

I recall that yandex does support site search, but with a different key. I guess I'd still prefer to hook this up with bug 482229, poked for an update there. That might be good enough for a follow up bug, though. 

An example url for yandex would be http://yandex.ru/yandsearch?date=&text=firefox&site=mozilla.ru&rstr=-&zone=all&wordforms=all&lang=all&within=0&from_day=&from_month=&from_year=&to_day=&to_month=&to_year=&mime=all&numdoc=10&lr=99, "Firefox site:mozilla.ru" doesn't work.

Either (using site search feature or fixing yandex) would be fine for a follow-up bug, though.

The other one is a macro rename that I'd like to see:

+<!-- LOCALIZATION NOTE (httpFileNotFound.searchSiteFor) - The span elements are
+parameters: e.g. "Search example.com for spam" (where "spam" is hyper-linked). -->
+<!ENTITY httpFileNotFound.searchSiteFor
+    "Search <span macro='site'/> for <span macro='searchUrl'/>">

Can we make searchURL be searchTerm? URL is a bit misleading on what text is visually inserted there.

Comment 126

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v11

bz:

Can you take a final look at the docshell changes since the Mk VII patch?  These changes are easy to see if you compare the Mk VII patch and the v11 patch with FileMerge (I assume any visual diff tool will work).

I tried to build an interdiff but the interdiff tool chokes when comparing these patches for some reason that I haven't been able to figure out.

Pike:

(In reply to comment #125)
> (From update of attachment 439721 [details] [diff] [review])
> r=me with nits.
> 
> I recall that yandex does support site search, but with a different key. I
> guess I'd still prefer to hook this up with bug 482229, poked for an update
> there. That might be good enough for a follow up bug, though. 
> 
> An example url for yandex would be
> http://yandex.ru/yandsearch?date=&text=firefox&site=mozilla.ru&rstr=-&zone=all&wordforms=all&lang=all&within=0&from_day=&from_month=&from_year=&to_day=&to_month=&to_year=&mime=all&numdoc=10&lr=99,
> "Firefox site:mozilla.ru" doesn't work.
> 
> Either (using site search feature or fixing yandex) would be fine for a
> follow-up bug, though.


I think a follow-up bug is the way to go.  If Yandex supports site search, it should be pretty easy to add support, but I'll need to find a Russian speaker to help.


> The other one is a macro rename that I'd like to see:
> 
> +<!-- LOCALIZATION NOTE (httpFileNotFound.searchSiteFor) - The span elements
> are
> +parameters: e.g. "Search example.com for spam" (where "spam" is hyper-linked).
> -->
> +<!ENTITY httpFileNotFound.searchSiteFor
> +    "Search <span macro='site'/> for <span macro='searchUrl'/>">
> 
> Can we make searchURL be searchTerm? URL is a bit misleading on what text is
> visually inserted there.

Good catch.  Fixed.

I also changed the definition of httpFileNotFound.searchTheWebBefore to use "macro=" instead of "local-id=".  It now matches the way the other definitions are parameterized.

Comment 127

[Benjamin Stover (:stechz)]

Has there been any progress lately on this bug?  We need a mobile-friendly solution for Fennec 2.0 that also leverages places.

Comment 128

[Benjamin Stover (:stechz)]

One of the unique problems we face with mobile is that remote pages that show this page will not have any privileges (Firefox will run into this as well later).  We may need a special API to get the places match.

Thoughts?

Comment 129

[Curtis Bartley [:cbartley]]

(In reply to comment #127)
> Has there been any progress lately on this bug?  We need a mobile-friendly
> solution for Fennec 2.0 that also leverages places.

I'm still waiting on final review on the last of the C++ changes.

Comment 130

[Curtis Bartley [:cbartley]]

(In reply to comment #128)
> One of the unique problems we face with mobile is that remote pages that show
> this page will not have any privileges (Firefox will run into this as well
> later).  We may need a special API to get the places match.
> 
> Thoughts?

The 404 page in this patch already runs at minimum privilege -- all the data it needs is passed in using the query string.

It's not a very flexible approach and it requires way more C++ in nsDocShell.cpp that I'd like, but it works and it keeps the 404 page unprivileged.

I sounds like Fennec is wanting to do something more (or just different) than what this patch does.  Can you elaborate on what you guys are thinking? (This might deserve a new Fennec-specific bug.)

Anyway, I have been thinking about an alternative approach to overriding 404s which I think would be more flexible than what I'm doing here.  (It would be very similar to the "I'll-call-you" approach in bug #474698, for those of you that can see it.)

Comment 131

[Benjamin Stover (:stechz)]

> The 404 page in this patch already runs at minimum privilege -- all the data it
> needs is passed in using the query string.

Ah, I see that now.  Great!  Your changes in nsDocShell.cpp will need to be e10sified for mobile, but it should be a small change.

> I sounds like Fennec is wanting to do something more (or just different) than
> what this patch does.  Can you elaborate on what you guys are thinking? (This
> might deserve a new Fennec-specific bug.)

I think we want the same thing, but with something handle the high DPI screens for Fennec.  Adding a couple of tags in the <head> section will probably be a good first try.

I will make a new bug and file this as a dependency.

Comment 132

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 440551 [details] [diff] [review]
404 override and new 404 error page v11

r=me on the docshell bits.  I'm really sorry for the terrible lag here.  :(

Comment 133

[Curtis Bartley [:cbartley]]

(In reply to comment #132)
> Comment on attachment 440551 [details] [diff] [review]
> 404 override and new 404 error page v11
> 
> r=me on the docshell bits.  I'm really sorry for the terrible lag here.  :(

Thanks bz.  I think the patch has suffered from bitrot, so I'll need to build a new patch.  It will be a few days before I can get to that, since I'm traveling and don't have access to a fast connection.

Comment 134

[Steffen Wilberg]

You didn't address comment 117 yet.

Comment 135

[Curtis Bartley [:cbartley]]

(In reply to comment #134)
> You didn't address comment 117 yet.

Noted.

Comment 136

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v12

Beyond changes in context to address simple bitrot, this patch also has a few small but substantive changes from the v11 patch:

* docshell/base/nsAboutRedirector.cpp
    added nsIAboutModule::HIDE_FROM_ABOUTABOUT for notfounderror
* docshell/build/nsDocShellModule.cpp
    the way modules are specified changed substantially
* toolkit/components/places/src/Makefile.in
    Small change in how nsAlternateUrlSuggester.js was added to EXTRA_COMPONENTS and also added the reference to the new nsAlternateUrlSuggester.manifest
* toolkit/components/places/src/nsAlternateUrlSuggester.js
    replaced NSGetModule function definition with call to XPCOMUtils.generateNSGetFactory().
* toolkit/components/places/src/nsAlternateUrlSuggester.manifest
    new file

Comment 137

[Curtis Bartley [:cbartley]]

(In reply to comment #136)
> Created attachment 466729 [details] [diff] [review]
> 404 override and new 404 error page v12

I was hoping that I finally had a landable patch, but no.  When I push the patch to the try server, I get an error on the unit test ("toolkit/components/places/tests/unit/test_suggester.js") to the effect of 'Cc["@mozilla.org/browser/alternate-url-suggester;1"] is undefined'.  This same test works fine locally.  When I test the built executable, the 404 override page does not come up, so that's not working either.

The error in the test implies that the suggester service is not getting initialized.  The fact that the override page doesn't come up at all in the executable suggests a more serious problem.

I know at least some of the files are getting pushed, since test_suggester.js doesn't exist except in this patch.  I presume all of them are getting pushed since I don't know how a subset would get pushed.  Now it's possible I screwed something up when pushing to the try server, but I just can't see how.

Right now I'm stumped about how to debug this.

Comment 138

[:Gavin Sharp [email: [e-mail]]]

Some tests are run on packaged builds. Do you need to add your new component to package-manifest.in?

Comment 139

[[not reading bugmail]]

or does this need to tie in services?

Comment 140

[Curtis Bartley [:cbartley]]

(In reply to comment #138)
> Some tests are run on packaged builds. Do you need to add your new component to
> package-manifest.in?

I don't think that it.  The test is getting run, it's just failing in a way that I find surprising.  I should mention that the test is an xpcshell-test for whatever that's worth.

I have had successful try builds in the past, but the most recent one was probably 6 months ago.  Has anything changed in that time that could affect me?

Comment 141

[Shawn Wilsher :sdwilsh]

(In reply to comment #138)
> Some tests are run on packaged builds. Do you need to add your new component to
> package-manifest.in?
Pretty sure this is it.  Other places stuff had to get added to this, and your patch doesn't do it:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#346

Comment 142

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #140)
> I don't think that it.  The test is getting run, it's just failing in a way
> that I find surprising.

The test is failing because the component doesn't exist. Your component doesn't exist because it's not in the manifest :) The manifest doesn't affect whether the test is run.

Comment 143

[Curtis Bartley [:cbartley]]

(In reply to comment #142)
> (In reply to comment #140)
> > I don't think that it.  The test is getting run, it's just failing in a way
> > that I find surprising.
> 
> The test is failing because the component doesn't exist. Your component doesn't
> exist because it's not in the manifest :) The manifest doesn't affect whether
> the test is run.

Reading comprehension fail on my part.  I've updated the manifest and spun up a new try server build, so I should have results shortly (by which I mean, sometime today).

Comment 144

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v13

Fixed the package-manifest.in problem.  The test now passes and the 404 override page comes up when testing the build manually.  Unfortunately I'm still seeing some problems in tests run by the try server.

For example, I see:

36062 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug209275.xhtml | Test timed out.
37756 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug353415-1.html | Test timed out.
37759 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug353415-2.html | Test timed out.
45151 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug481335.xhtml | Test timed out.

and 

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_522545.js | Timed out


consistently across many individual try builds.  I haven't had a chance to investigate further, yet.  This patch should have minimal intrusion into the non-404 code path, but these errors are way too consistent to be something spurious.

Dang it.

Comment 145

[Curtis Bartley [:cbartley]]

(In reply to comment #144)

> Unfortunately I'm
> still seeing some problems in tests run by the try server.

OK, the primary problem here is that some tests use URLs that result in 404 errors.  When the 404 page comes from the server, the regular page load events are fired.  However, the 404 override page is an error page, and as such loads in the background (i.e. the LOAD_BACKGROUND flag is specified on the channel).  So now the 404 pages encountered by these tests never fire the normal onload event and the tests hang.

There are two obvious solutions:

(a) Load the 404 override page in the foreground, unlike the other error pages.
(b) Continue loading the 404 override page in the background and change the tests that depend on the old behavior.

I think (a) is probably the right way to go and will have the lowest risk.  However, it looks like it will require a change to nsDocShell.cpp to special-case the 404 override page.  Actually, it's more like error pages are already special-cased to load in the background, and we need to un-special-case the case where the error page is actually the 404 override page.  Needless to say, this is exactly the kind of code that nsDocShell.cpp doesn't need more of.  But I digress.

I'll investigate approach (a) more tomorrow and see if I can get a revised patch up.

As an added bonus, at least one test failed when it tried to reach into the 404 page DOM and triggered a security exception.  I'll save that problem for later.

Comment 146

[Boris Zbarsky [:bzbarsky]]

Approach (a) sounds right.

The security exception thing is unfortunate... that might bite web content in weird cases.  :(

Comment 147

[Curtis Bartley [:cbartley]]

(In reply to comment #146)

> The security exception thing is unfortunate... that might bite web content in
> weird cases.  :(

No doubt.  I don't think we have a choice here, though, since the 404 override page may now contain user history in the form of the suggestions.

Comment 148

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v14

This change causes the 404 override page to load normally, rather than in the background.  It fixes the test timeout problems I was seeing earlier.  The only change in this patch from the v13 patch is at the end of nsDocShell::LoadErrorPage():

------------------- v13 code -------------------

     nsCOMPtr<nsIURI> errorPageURI;
     nsresult rv = NS_NewURI(getter_AddRefs(errorPageURI), errorPageUrl);
     NS_ENSURE_SUCCESS(rv, rv);
 
     return InternalLoad(errorPageURI, nsnull, nsnull,
                         INTERNAL_LOAD_FLAGS_INHERIT_OWNER, nsnull, nsnull,
                         nsnull, nsnull, LOAD_ERROR_PAGE,
                         nsnull, PR_TRUE, nsnull, nsnull);
 }


------------------- v14 code -------------------

     nsCOMPtr<nsIURI> errorPageURI;
     nsresult rv = NS_NewURI(getter_AddRefs(errorPageURI), errorPageUrl);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    PRUint32 loadType = LOAD_ERROR_PAGE;
+
+    // Error pages normally load in the background.  The 404 override page is
+    // a special case, however.  We want it to load normally.  See bug 482874.
+    if (nsString(aErrorType).Equals(NS_LITERAL_STRING("httpFileNotFound"))) {
+      loadType = LOAD_NORMAL;
+    }
+
     return InternalLoad(errorPageURI, nsnull, nsnull,
                         INTERNAL_LOAD_FLAGS_INHERIT_OWNER, nsnull, nsnull,
+                        nsnull, nsnull, loadType,
                         nsnull, PR_TRUE, nsnull, nsnull);
 }

------------------- notes -------------------

* | nsString(aErrorType).Equals(NS_LITERAL_STRING("httpFileNotFound")) | 
  seems awfully ugly, and now I'm doing it twice in LoadErrorPage().
  An alternative might be to pass |aError| in from DisplayLoadError().

* There's 19 or 20 different LOAD_XXX enums.  Do we always want to use
  LOAD_NORMAL here?
  
* I think there really needs to be a comment here, but how detailed does
  it need to be?  Is it reasonable to just reference the bug?  I really
  want to expound on *why* the 404 override page is different, but I may
  exhibit a tendency to overkill when writing comments of this type.

Comment 149

[Curtis Bartley [:cbartley]]

There's still a couple of problem tests, for example:

content/html/content/test/test_bug353415-1.html and
content/html/content/test/test_bug353415-2.html

These tests submit a form to a bogus URL on the mochi.test server, then try to read location.href from the 404 page that comes back.  When it's the 404 override page, this read fails with a security error.

Here's the code from the first test:

<script>
	document.forms[0].submit();

	SimpleTest.waitForExplicitFinish();
        function doCheck(){
		is(frames['submit_frame'].location.href, "http://mochi.test:8888/blah?field1=teststring&field2=0&field4=1&field6=3&field7=2&field8=8&field9=9&field12=", "Submit string was correct.");
                SimpleTest.finish();
	}
</script>

Now that I think about it, it might be enough to pad out the mochi.test server's 404 page to 512+ bytes so it won't be overridden.  I'll give that a try next.

Comment 150

[Boris Zbarsky [:bzbarsky]]

> * | nsString(aErrorType).Equals(NS_LITERAL_STRING("httpFileNotFound")) | 

  nsString(aErrorType).EqualsLiteral("httpFileNotFound");

Though yes, it would be nice if aErrorType were nsAString.  That's a bunch more work, though.

> Do we always want to use LOAD_NORMAL here?

I think so, yes.

I think a longish comment here is fine as needed.

Comment 151

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v15

Changes in this patch from the v14 patch:

* Use EqualsLiteral in LoadErrorPage().
* More detailed comment for the LOAD_NORMAL case at the end of LoadErrorPage().
* Modified httpd.js (the MochiTest server) to guarantee a 404 page size of at
  least 512 bytes.

This appears to address all the test failures in content/html/content/test, if
I ignore the failures that I also see when the patch isn't applied.  I'm still
waiting on the try server to get more definitive results.

Comment 152

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #150)
> > * | nsString(aErrorType).Equals(NS_LITERAL_STRING("httpFileNotFound")) | 
> 
>   nsString(aErrorType).EqualsLiteral("httpFileNotFound");

Or nsDependentString(aErrorType) to avoid a copy, right?

Comment 153

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v16

Updated the change in browser.js to match a change in the underlying code.

Comment 154

[Curtis Bartley [:cbartley]]

OK, it looks like my Try build from yesterday afternoon was clean.  It seems like I probably need somebody to at least informally review my change to httpd.js, however.  Who would be a good choice for that?

Comment 155

[Joe Drew (not getting mail)]

Jeff Walden!

Comment 156

[Curtis Bartley [:cbartley]]

The last significant change that I made, was this one to httpd.js:

> diff -r fe4057f8386d netwerk/test/httpserver/httpd.js
> --- a/netwerk/test/httpserver/httpd.js	Thu Aug 26 14:46:47 2010 -0400
> +++ b/netwerk/test/httpserver/httpd.js	Thu Aug 26 13:59:10 2010 -0700
> @@ -3129,16 +3129,21 @@ ServerHandler.prototype =
>                        <h1>404 Not Found</h1>\
>                        <p>\
>                          <span style='font-family: monospace;'>" +
>                            htmlEscape(metadata.path) +
>                         "</span> was not found.\
>                        </p>\
>                      </body>\
>                    </html>";
> +
> +      // Ensure at least 512 characters in the page to prevent the browser
> +      // from overriding the 404 page.
> +      body += new Array(513).join(" ");
> +
>        response.bodyOutputStream.write(body, body.length);
>      },

All this does is pad the MochiTest server's 404 page out to more than 512
characters, which is the threshold for the 404 override page.  This guarantees
that 404 pages served by the MochiTest server are not overridden.  I did this
since we have some tests which expect a 404 page which they can inspect
programmatically, which they can't do if the override page comes up, since it
has much tighter security settings.  I mentioned this change earlier, but I
wanted to consolidate the explanation into a single comment.

I think this change is really low risk and is sufficiently trivial that it 
shouldn't require a formal review.  I do, however, want to spell it out for
informal review.

Comment 157

[Shawn Wilsher :sdwilsh]

Comment on attachment 469604 [details] [diff] [review]
404 override and new 404 error page v16

a=sdwilsh, but I think we should land this right after we freeze for beta 5 (so it'll make beta 6).

Comment 158

[Curtis Bartley [:cbartley]]

In the latest patch, the 404 override page shows the about:notfounderror URL in the location bar.  It's supposed to show the original URL that triggered the error instead, e.g. "http://www.example.com/this-page-does-not-exist.html".  This is a consequence of loading the page normally (LOAD_NORMAL) instead of  using LOAD_ERROR_PAGE.  The problem with the latter is that pages loaded with LOAD_ERROR_PAGE are loaded in the background and don't fire normal onload events -- see comment #145.

I'm not sure what the proper fix for this should be.

Comment 159

[Curtis Bartley [:cbartley]]

Attachment: 404 override and new 404 error page v17

This patch reverts to treating the 404 override page as an error page (mLoadType == LOAD_ERROR_PAGE).  However, the flag to load in the background is *not* set when the the error page is the 404 override page.  This way the 404 override page still generates the expected onload events, and we also retain the original path in the location bar, rather than the "about:notfounderror?..." path to the override page.

Comment 160

[Curtis Bartley [:cbartley]]

(In reply to comment #159)
> Created attachment 474780 [details] [diff] [review]
> 404 override and new 404 error page v17

I've got a clean try server build for this patch.  If anyone wants to take a look at the changes between the previously approved v16 patch and this one, the easiest way is to simply diff the two patches with a visual diff tool (e.g. FileMerge on the Mac).  I've found this often works better than using "interdiff".

Comment 161

[Oleg Romashin (:romaxa)]

Attachment: Patch from maemo6 updated to latest m-c

Comment 162

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 469604 [details] [diff] [review]
404 override and new 404 error page v16

(bookkeeping)

Comment 163

[(no longer active)]

I wanted to land this on cedar but I was not sure which patch should be landed.

Comment 164

[Shawn Wilsher :sdwilsh]

I think we need an updated patch, and at this point, probably an updated UX spec.

Comment 165

[Curtis Bartley [:cbartley]]

(In reply to comment #163)
> I wanted to land this on cedar but I was not sure which patch should be landed.

I don't think the v17 patch will apply cleanly any more.  It looks like romaxa's patch was just a straight update to deal with some changes in nsDocShell.cpp (look for the SAFE_ESCAPE macro).  There were a couple of other minor differences related to include files, etc.

Comment 166

[Wesley Johnston (:wesj)]

I was looking at this today since we are updating error pages on mobile. Reducing the need for the keyboard in Fennec is a primary UX goal. Is anyone pushing this forward anymore, or would it be better for me start to unbit-rotting the patches + annoy ux people?

Comment 167

[Curtis Bartley [:cbartley]]

(In reply to comment #166)
> I was looking at this today since we are updating error pages on mobile.
> Reducing the need for the keyboard in Fennec is a primary UX goal. Is anyone
> pushing this forward anymore, or would it be better for me start to
> unbit-rotting the patches + annoy ux people?

I'm not pushing it forward anymore -- due to the rigors of a full-time job, among other things.  If you want to take it and push it, then by all means, go for it.  I can answer questions, but that's about all I have time for right now.

Comment 168

[Wesley Johnston (:wesj)]

Attachment: Updated patch v17

This is just unbitrotted, and works as well as it ever did. I'm still digging through these comments and trying to figure out where this is in the review process...

I did talk to Ben about the e10s problems (since he sits next to me). Since we can't access places from the child process, we'll have to insert some remoting code into nsAlternateUrlSuggester.js... it'd be nice if that could be async.

Comment 169

[Curtis Bartley [:cbartley]]

(In reply to comment #168)
> Created attachment 546250 [details] [diff] [review] [review]
> Updated patch v17

> I did talk to Ben about the e10s problems (since he sits next to me). Since
> we can't access places from the child process, we'll have to insert some
> remoting code into nsAlternateUrlSuggester.js... it'd be nice if that could
> be async.

If I had this to do over again, I'd probably have a JavaScript service that hangs around waiting for 404 pages to load, and then have it insert the suggestions after the page loads.  There's code that does something pretty similar in the patch on bug #474698 that might be worth checking out.  That code listens for DOMContentLoaded, but I think there's a better event to listen to these days.  Let me know if you'd like more details.

Comment 170

[Wesley Johnston (:wesj)]

Hmm... 474698 is marked a need-to-know, but I think that sounds like a cleaner solution as well.

Comment 171

[Curtis Bartley [:cbartley]]

(In reply to comment #170)
> Hmm... 474698 is marked a need-to-know, but I think that sounds like a
> cleaner solution as well.

I cc'd you on it so you can see it.  Look for "DOMContentLoaded" in the patch.  Also note that the "content-document-global-created" event might be a better choice than DOMContentLoaded.

Comment 172

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In the screenshot, there is no button for the search field. I think there should be, so that you can do the search with a single click.

Also, do our contracts with search engines have to change to account for this new way of searching?

Comment 173

[Axel Hecht [:Pike]]

(In reply to comment #172)
> Also, do our contracts with search engines have to change to account for
> this new way of searching?

CCing Kev for that.

Comment 174

[Wesley Johnston (:wesj)]

Attachment: Updated patch v17 Part 1/3

Splitting this up to (hopefully) make things a little easier. This just contains the DocShell stuff to detect short 404 pages and redirect them to a default about:neterror page.

Comment 175

[Wesley Johnston (:wesj)]

Attachment: Updated patch v17 Part 2/3 -> nsAlternateUrlSuggester

nsAlternateUrlSuggester service

Comment 176

[Wesley Johnston (:wesj)]

Attachment: Updated patch v17 Part 3/3 -> Special error page

Use the modified error page + suggestions.

I'm not a big fan of this in its current form. The more I think about it, maybe a global listener is the way to go here. We can do the suggestions query async, while still getting the error page up quickly.

We'd add some global listener on the main window (parent process?) that watched for page loads of about:neterror pages. If we saw one swim by that we thought could benefit from suggestions, we could request helpful urls from the URLSuggester service (async) and then push them (into the child process and finally) into the page.

I'm curious if anyone has any feedback before I start running in that direction.

Comment 177

[Curtis Bartley [:cbartley]]

(In reply to comment #176)
> Created attachment 546632 [details] [diff] [review] [review]
> Updated patch v17 Part 3/3 -> Special error page
> 
> Use the modified error page + suggestions.
> 
> I'm not a big fan of this in its current form. The more I think about it,
> maybe a global listener is the way to go here. We can do the suggestions
> query async, while still getting the error page up quickly.
> 
> We'd add some global listener on the main window (parent process?) that
> watched for page loads of about:neterror pages. If we saw one swim by that
> we thought could benefit from suggestions, we could request helpful urls
> from the URLSuggester service (async) and then push them (into the child
> process and finally) into the page.
> 
> I'm curious if anyone has any feedback before I start running in that
> direction.

FWIW, I think it should be pretty easy to do this.  One advantage is that the entire suggester service and page injection can be done in JavaScript.  The biggest downside, is that someone would probably need to remove a bunch of the new nsDocShell C++ code from the 404 interception part of the patch.

Comment 178

[Wesley Johnston (:wesj)]

Attachment: Updated patch v17.1 Part 2/3 -> nsAlternateUrlSuggester

Just a little change to send back results broken up by path segments (i.e. http://www.example.com/foo/bar will offer http://www.example.com/foo)?.

Will need to add async methods here next.

Comment 179

[Wesley Johnston (:wesj)]

Attachment: Updated patch v18 Part 3/3 -> Error page listener

This implements the listener method I proposed above in Firefox. It just uses the standard netError.xhtml page.

Still haven't handled e10s or anything here, that's next I hope.

Comment 180

[Wesley Johnston (:wesj)]

Attachment: Patch v19 1/4 -> DocShell changes

This hasn't changed, but I hate having different numbers here. This is just DocShell parts. Hasn't changed too much from the original, beyond removing all of the code sending queryParams to the neterror page.

Comment 181

[Wesley Johnston (:wesj)]

Attachment: Patch v19 2/4 -> NetErrorHelper.jsm

This implements a NetErrorHelper service which queries places (async) for urls that are similar to the one in question (within three characters). e10s remoted for mobile.

Comment 182

[Wesley Johnston (:wesj)]

Attachment: Patch v19 3/4 -> Firefox frontend pieces

Hooks this up to the Firefox frontend. Will probably need some UX review once the backend parts looks ok to everyone.

Comment 183

[Wesley Johnston (:wesj)]

Attachment: Patch v19 4/4 -> Mobile pieces

Mobile frontend pieces. Pretty much the same as the Firefox ones.

Comment 184

[Dão Gottwald [:dao]]

Comment on attachment 549514 [details] [diff] [review]
Patch v19 2/4 -> NetErrorHelper.jsm

This file should use 2-space indentation as well.

Comment 185

[Dão Gottwald [:dao]]

Comment on attachment 549515 [details] [diff] [review]
Patch v19 3/4 -> Firefox frontend pieces

You're also using 4-space indentation randomly in netError.xhtml.

Comment 186

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 549511 [details] [diff] [review]
Patch v19 1/4 -> DocShell changes

This is fine, but file a followup bug to convert this to the new prefs API?

Comment 187

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 549514 [details] [diff] [review]
Patch v19 2/4 -> NetErrorHelper.jsm

Review of attachment 549514 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/resources/content/NetErrorHelper.jsm
@@ +41,5 @@
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +const ASYNC_SUGGEST_MESSAGE = "AUS:asyncsuggest"
> +const ASYNC_SUGGEST_MESSAGE_RETURN = "AUS:asyncsuggest:return"

missing semicolons

@@ +43,5 @@
> +
> +const ASYNC_SUGGEST_MESSAGE = "AUS:asyncsuggest"
> +const ASYNC_SUGGEST_MESSAGE_RETURN = "AUS:asyncsuggest:return"
> +
> +let NetErrorHelper = {

I see the file is using a mixture of 2 vs 4 spaces indentation, this should be fixed.  Usually we use 2 spaces, but I know docshell is exotic by using 4 spaces.  Whatever you decide to go it should be consistent in the file.

@@ +45,5 @@
> +const ASYNC_SUGGEST_MESSAGE_RETURN = "AUS:asyncsuggest:return"
> +
> +let NetErrorHelper = {
> +    init: function neh_init() {
> +        let appInfo = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);

Services.appinfo

@@ +49,5 @@
> +        let appInfo = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
> +        if (appInfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> +          let messageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]
> +                                        .getService(Ci.nsIFrameMessageManager);
> +          messageManager.addMessageListener(ASYNC_SUGGEST_MESSAGE, this);

bad indentation, align dots with [ and avoid the local
Cc["@mozilla.org/parentprocessmessagemanager;1"]
  .getService(Ci.nsIFrameMessageManager)
  .addMessageListener(ASYNC_SUGGEST_MESSAGE, this);

@@ +59,5 @@
> +        if (appInfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> +          let messageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]
> +                                        .getService(Ci.nsIFrameMessageManager);
> +          messageManager.removeMessageListener(ASYNC_SUGGEST_MESSAGE, this);
> +        }

ditto for appinfo and indentation

@@ +65,5 @@
> +
> +    isErrorPage: function neh_iserrorpage(aWindow) {
> +        if (aWindow.location.href == aWindow.document.documentURI)
> +            return false;
> +        let url = aWindow.document.documentURI;

you use it twice, so you can cache it earlier

@@ +67,5 @@
> +        if (aWindow.location.href == aWindow.document.documentURI)
> +            return false;
> +        let url = aWindow.document.documentURI;
> +        let uri = Services.io.newURI(url, null, null);
> +        return uri.scheme == "about" &&  /^neterror/.test(uri.path); // ? \?e=httpFileNotFound

double space after &&
what does the comment mean?

@@ +72,5 @@
> +    },
> +
> +    getSuggestionsAsync: function neh_getsuggestasync(aUri, aCallback) {
> +
> +        let appInfo = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);

useless newline, and Services.appinfo

@@ +75,5 @@
> +
> +        let appInfo = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
> +        if (appInfo.processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> +            let messageManager = Cc["@mozilla.org/childprocessmessagemanager;1"].
> +                                 getService(Ci.nsISyncMessageSender);

nit: the indentation style is not consistent with the previous...
btw since you use these so often, you may rather add lazyServiceGetters to your module

@@ +85,5 @@
> +            );
> +            let json = { url: aUri.spec };
> +            messageManager.sendAsyncMessage(ASYNC_SUGGEST_MESSAGE, json);
> +            return;
> +        } else {

we usually don't "if return else return", the else is useless here

@@ +93,5 @@
> +            let self = this;
> +            stmt.executeAsync({
> +                _results: [],
> +                handleCompletion: function(aReason) {
> +                    self.handleResults(aUri, this._results, aCallback);

rather than defining a self, you may just use NetErrorHelper.handleResults

@@ +95,5 @@
> +                _results: [],
> +                handleCompletion: function(aReason) {
> +                    self.handleResults(aUri, this._results, aCallback);
> +                },
> +                handleError: function(aError) { },

Cu.reportError here please

@@ +101,5 @@
> +                    let row = aResultSet.getNextRow();
> +                    while(row) {
> +                        this._results.push(row.getResultByName("url"));
> +                        row = aResultSet.getNextRow();
> +                    }

you may use a for like
for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow())

@@ +103,5 @@
> +                        this._results.push(row.getResultByName("url"));
> +                        row = aResultSet.getNextRow();
> +                    }
> +                }
> +            });

add a QueryInterface through XPCOMUtils.generateQI

@@ +107,5 @@
> +            });
> +        }
> +    },
> +
> +    handleResults: function(aUri, aResults, aCallback) {

name the method

@@ +119,5 @@
> +        this.suggestSearchOptions(aUri, ret);
> +        aCallback(ret);
> +    },
> +
> +    receiveMessage: function AUS_receiveMessage(aMessage) {

what is AUS_?

@@ +135,5 @@
> +            }
> +        }
> +    },
> +
> +    _getSiteFromUrl: function neh_getSiteFromUrl(url) {

what is "site" here? hostname? part of it? It probably needs a better name and some documentation

@@ +143,5 @@
> +        else
> +            return url;
> +    },
> +
> +    _getStatement: function AUS_getStatement(aUri) {

what is AUS_?

@@ +149,5 @@
> +        // longer strings.  We use a max-length cut-off to limit query times.
> +        const maxUrlLength = 80;
> +
> +        // We ignore strings that differ too much from the reference.
> +        const maxEditDistance = 5;

nit: some consts are all uppercase, while these are camelcase. I'd suggest going the common SOME_CONST style and move these consts to the top of the module

@@ +158,5 @@
> +        }
> +
> +        let historyService = Cc["@mozilla.org/browser/nav-history-service;1"].
> +                         getService(Ci.nsPIPlacesDatabase);
> +        let db = historyService.DBConnection;

add a PlacesDBConn lazyGetter

@@ +165,5 @@
> +        // avoid an overzealous warning about escaping patterns for LIKE.  Also
> +        // note that pages with NULL titles will be excluded; this is a feature
> +        // since it means we exclude pages that we've shown the 404 override page
> +        // for.
> +        let stmt = db.createStatement(

Use createAsyncStatement, its cheaper in this case.

@@ +171,5 @@
> +          " WHERE ABS(LENGTH(url) - LENGTH(:spec)) <= :maxEditDistance " +
> +          " AND levenshteinDistance(url, :spec) <= :maxEditDistance " +
> +          " AND url <> :spec " +
> +          " AND NOT title LIKE :like404 " +
> +          " ORDER BY levenshteinDistance(url, :spec) LIMIT 3 ");

This query is crazily expensive, we don't really want to do this since it's causing a full table scan of some tens thousands rows.

What we should do is to limit the search by something with an index, a really simple idea is to limit it to the recently visited pages (what I do here). A more expensive alternative may be to limit to the most frecent pages, we may evaluate this later based on feedback.

This is my suggestion, that is at least 10 times faster:

/* do not warn (bug 482874) */
SELECT url, title
FROM moz_places
WHERE last_visit_date > (strftime('%s','now','-30 days') * 1000000)
AND ABS(LENGTH(url) - LENGTH(:spec)) <= :maxDistance
AND levenshteinDistance(url, :spec) BETWEEN 1 AND >maxDistance
ORDER BY levenshteinDistance(url, :spec) ASC

A couple notes:
- the "do not warn" is needed due to the order by, it can't be optimized
- I removed the LIKE, since it's slow and implementation was hackish, I think we should filter results here in the module
- I removed the limit, it's not free and since we have to filter results, we should rather .cancel() the pendingStatement when we have enough results

@@ +174,5 @@
> +          " AND NOT title LIKE :like404 " +
> +          " ORDER BY levenshteinDistance(url, :spec) LIMIT 3 ");
> +        stmt.params.spec = aUri.spec;
> +        stmt.params.like404 = "%404%";
> +        stmt.params.maxEditDistance = maxEditDistance;

you don't want to recreate the statement each time, cache it in a ._stmt, and just bind params here.  You should listen for "places-shutdown" and .finalize() it there. Indeed I think right now this would badly assert on shutdown and leak Places connection since you don't finalize it.

@@ +178,5 @@
> +        stmt.params.maxEditDistance = maxEditDistance;
> +        return stmt;
> +    },
> +
> +    suggestSearchOptions: function AUS_suggestSearchOptions(aUri, aReturn) {

AUS_?

@@ +180,5 @@
> +    },
> +
> +    suggestSearchOptions: function AUS_suggestSearchOptions(aUri, aReturn) {
> +        let searchService = Cc["@mozilla.org/browser/search-service;1"].
> +                             getService(Ci.nsIBrowserSearchService);

Services.search

@@ +189,5 @@
> +            let submission = searchEngine.getSubmission(aUri.spec, null);
> +            aReturn.parameterizedSearchSpec = this.createParameterizedSearchUrl(
> +                                                                 searchEngine);
> +            [aReturn.siteSearchQuery, aReturn.siteSearchSpec] =
> +                           this.createSiteSearchComponents(searchEngine, aUri);

nit: fancy indentation

@@ +200,5 @@
> +    },
> +
> +    //////////////////////////////////////////////////////////////////////////////
> +    //// utility methods
> +    supportsSiteSearch: function (searchEngine) {

name the method

@@ +212,5 @@
> +        let sld = hostParts.pop();
> +        return (sld == "google" || sld == "yahoo" || sld == "bing");
> +    },
> +
> +    createParameterizedSearchUrl: function (searchEngine) {

name the method

@@ +217,5 @@
> +        let submission = searchEngine.getSubmission("QUERY", null);
> +        return submission.uri.spec;
> +    },
> +
> +    createSiteSearchComponents: function (searchEngine, uri) {

name the method

::: docshell/resources/content/Makefile.in
@@ +41,4 @@
>  srcdir		= @srcdir@
>  VPATH		= @srcdir@
>  
> +EXTRA_PP_JS_MODULES = \

why do you need preprocessing here, I don't see any directive at first glance

Comment 188

[Wesley Johnston (:wesj)]

Attachment: Patch v20 2/3 -> NetErrorHelper.jsm

Thanks for the comments mak. Updated to address them all I think. Lots of lazy loaders... 

> Services.appinfo
Strange, this throws an error in my builds. I made it a lazy getter here, but I'll have to look and see whats up...

> what does the comment mean?
Whoops. I was just trying to make life simple for a sec and forgot about this. This check is more exact now, and looks for error pages with e=httpFileNotFound. I'm also actually using this in the next patches now!

Comment 189

[Wesley Johnston (:wesj)]

Attachment: Patch v20 3/4 -> Firefox frontend

Updated Firefox frontend

Comment 190

[Wesley Johnston (:wesj)]

Attachment: Patch v20 4/4 -> Mobile frontend

Comment 191

[Axel Hecht [:Pike]]

Comment on attachment 550845 [details] [diff] [review]
Patch v20 3/4 -> Firefox frontend

The templating stuff here is not going to work for l10n. You'll need something that 

- explains what's what
- allows to reorder the filled in strings

Best would be to re-use something that's used elsewhere in our code. If that doesn't work, I'd use a syntax that is somewhat forwards compatible with l20n, that is, '{{ variableName }}' as placeholder.

Either way, all of the strings with logic need extensive localization notes explaining what kind of input will get there.

Also, a comment on why httpFileNotFound is empty would be helpful for localizers to keep it empty. If there's even a reason to have those, if they're empty.

Comment 195

[Wesley Johnston (:wesj)]

Not actively working on this right now.

Comment 196

[Bur]

Maybe before trying to improve on 404 error messages where at least there is one, any message at all should be implemented if the server doesn't display one. Firefox still simply shows a blank page when a server returns a header with no html body.

Bug 442255 was filed in 2008 and there was even a patch around though it somehow didn't make it into the builds.

Comment 199

[BelFox]

(In reply to Bur from comment #196)
> Maybe before trying to improve on 404 error messages where at least there is
> one, any message at all should be implemented if the server doesn't display
> one. Firefox still simply shows a blank page when a server returns a header
> with no html body.
> 
> Bug 442255 was filed in 2008 and there was even a patch around though it
> somehow didn't make it into the builds.

+1!

I was testing a REST API endpoint via Firefox today.
For a particular URL, the server returned a 404 error code without body (Content-Length=0).

Expected result:
- In absence of a server-generated 404 page, Firefox passes the 404 message along to the user via its own, simple error page (like other browsers do).

Actual result:
- Firefox shows a blank page. Only by checking through the dev tools, I found out a 404 had been returned.


Potential quick win for near-future release? Candidate Test Pilot experiment?