Last Comment Bug 734688 - OpenSearch broke session history for content tabs
: OpenSearch broke session history for content tabs
: regression
Product: Thunderbird
Classification: Client Software
Component: Search (show other bugs)
: 10 Branch
: All All
-- normal (vote)
: Thunderbird 14.0
Assigned To: Jim Porter (:squib)
Depends on:
Blocks: 677421
  Show dependency treegraph
Reported: 2012-03-10 15:51 PST by Dave Townsend [:mossop]
Modified: 2016-09-21 06:16 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

FIx this(?) (887 bytes, patch)
2012-03-14 18:42 PDT, Jim Porter (:squib)
no flags Details | Diff | Splinter Review
Change IDs, remove disablehistory, and move the find bar (1.47 KB, patch)
2012-03-14 22:05 PDT, Jim Porter (:squib)
standard8: review-
Details | Diff | Splinter Review
More fixes... (4.02 KB, patch)
2012-04-15 16:35 PDT, Jim Porter (:squib)
no flags Details | Diff | Splinter Review
Alternate patch (5.35 KB, patch)
2012-04-15 16:43 PDT, Jim Porter (:squib)
standard8: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description User image Dave Townsend [:mossop] 2012-03-10 15:51:31 PST
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.

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
(, 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.

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 User image Jim Porter (:squib) 2012-03-14 18:42:53 PDT
Created attachment 606053 [details] [diff] [review]
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 User image Dave Townsend [:mossop] 2012-03-14 20:24:54 PDT
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 User image Jim Porter (:squib) 2012-03-14 22:05:10 PDT
Created attachment 606116 [details] [diff] [review]
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.

Comment 4 User image Mark Banner (:standard8) 2012-03-20 06:46:58 PDT
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 User image Mark Banner (:standard8) 2012-04-13 07:56:22 PDT
Jim, any chance you could fix this up before next week?
Comment 6 User image Jim Porter (:squib) 2012-04-15 16:35:27 PDT
Created attachment 615204 [details] [diff] [review]
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 User image Jim Porter (:squib) 2012-04-15 16:43:51 PDT
Created attachment 615205 [details] [diff] [review]
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 User image Mark Banner (:standard8) 2012-04-17 13:32:55 PDT
Comment on attachment 615204 [details] [diff] [review]
More fixes...

Yep, I prefer the other one.
Comment 9 User image Mark Banner (:standard8) 2012-04-17 13:33:11 PDT
Comment on attachment 615205 [details] [diff] [review]
Alternate patch

[Triage Comment]
Comment 10 User image Mark Banner (:standard8) 2012-04-17 14:09:09 PDT
Checked in:
Comment 11 User image Mark Banner (:standard8) 2012-04-17 14:10:09 PDT
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 13 User image Ludovic Hirlimann [:Usul] 2012-04-18 03:07:08 PDT
Jim could you investigate the testing part further please ?

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