734688 - OpenSearch broke session history for content tabs

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Dave Townsend [:mossop]]

Since Firefox 10 I've been seeing session history broken for content tabs created by my extension WebApp Tabs. I finally figured out the problem today, the browser elements created by specialTabs.js end up with the disablehistory attribute set on them. I have a workaround but it looks to me like this is a mistake caused by the landing of bug 677421.

That patch is too big for me to want to spend anytime trying to figure out exactly what the expectation is of these things, but here are two anomalies I can see that are likely bugs:

The overlay attempts to modify the dummycontentbrowser that all content browsers are cloned from to enable history by setting the disablehistory attribute to false. The problem is that that won't work, if the disablehistory attribute exists at all then history is disabled no matter its value.
http://hg.mozilla.org/comm-central/annotate/b438e21e3c6b/mail/base/content/webSearchTab.xul#l53

As far as I can tell this should be unnecessary anyway as the specialTabs code removes the disablehistory attribute on startup before any new content tabs are created
(http://mxr.mozilla.org/comm-central/source/mail/base/content/specialTabs.js#500), this used to work fine but unfortunately bug 677421 also introduced a second element with the id "dummycontentbrowser" and it is this element that is now returned by getElementById("dummycontentbrowser") so it is that that gets its dummycontentbrowser attribute removed instead.
http://hg.mozilla.org/comm-central/annotate/b438e21e3c6b/mail/base/content/webSearchTab.xul#l71

My best guess without really knowing what the rest of the OpenSearch code is up to is that renaming that new browser element to have a unique ID and removing the attempt to modify disablehistory from webSearchTab.xul should just make everything work.

Comment 1

[Jim Porter (:squib)]

Attachment: FIx this(?)

It seems that somewhere along the lines, something went awry, and now the back and forward buttons don't work in opensearch. I think this is the same underlying issue as in the bug here, so let's fix it.

I'm not really sure this is the right way, but it fixes the back and forward buttons, at least.

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 606053 [details] [diff] [review]
FIx this(?)

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

::: mail/base/content/webSearchTab.xul
@@ +67,3 @@
>              <button class="defaultButton" type="checkbox"/>
>            </vbox>
>            <browser id="dummycontentbrowser" type="content-targetable" flex="1"

Without renaming this ID you won't actually fix this bug as you'll still have two elements in the document with the same ID and the code to make session history work in content pages won't work because of it.

Comment 3

[Jim Porter (:squib)]

Attachment: Change IDs, remove disablehistory, and move the find bar

At the risk of turning this into an ever-expanding patch, here are fixes for the three issues I know of in OpenSearch:

1) Web history should work now (should we do things the specialTabs.js way[1]?)
2) The IDs are unique
3) The find bar shows up in the right spot (I'm surprised no one noticed this until now! Go ahead, try hitting Ctrl+F on 10.0+...)

Tests for #1 would be good, but I'm not sure how to override the engines to use a local page.

[1] http://mxr.mozilla.org/comm-central/source/mail/base/content/specialTabs.js#500

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 606116 [details] [diff] [review]
Change IDs, remove disablehistory, and move the find bar

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

This looks reasonable, but we need to enable history the same way as content tabs - I don't want to impose loading global history too early in startup.

Comment 5

[Mark Banner (:standard8)]

Jim, any chance you could fix this up before next week?

Comment 6

[Jim Porter (:squib)]

Attachment: More fixes...

Wow, some of these mistakes were really silly... everything should be working now, though. We probably ought to think about ways to test this so it doesn't regress again.

Comment 7

[Jim Porter (:squib)]

Attachment: Alternate patch

Here's an alternate method for implementing the command controller. I think this is better, since it would have saved us from the switch statement fallthrough bug that the previous patch addresses.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 615204 [details] [diff] [review]
More fixes...

Yep, I prefer the other one.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 615205 [details] [diff] [review]
Alternate patch

[Triage Comment]

Comment 10

[Mark Banner (:standard8)]

Checked in:

http://hg.mozilla.org/comm-central/rev/da4a29c67276

Comment 11

[Mark Banner (:standard8)]

btw for the tests, I think the search engine allows you to add in search engines via calls, investigation of the tests in toolkit/ might help with ideas.

Comment 12

[Mark Banner (:standard8)]

Checked into branches:

http://hg.mozilla.org/releases/comm-aurora/rev/937f5096f67f
http://hg.mozilla.org/releases/comm-beta/rev/3936c046a857

Comment 13

[Ludovic Hirlimann [:Usul]]

Jim could you investigate the testing part further please ?