Last Comment Bug 746837 - Fix sessionstore to handle an exception thrown when attempting to focus a window that has been navigated
: Fix sessionstore to handle an exception thrown when attempting to focus a win...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks: hueyfix 749106
  Show dependency treegraph
 
Reported: 2012-04-18 17:58 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2012-05-04 09:05 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.04 KB, patch)
2012-04-18 17:58 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
better patch? (1.37 KB, patch)
2012-04-19 00:57 PDT, Dão Gottwald [:dao]
paul: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-18 17:58:18 PDT
Created attachment 616385 [details] [diff] [review]
Patch

This will throw after Bug 695480 lands.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-18 18:18:39 PDT
This should probably just be calling browser.focus()?
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-18 18:19:17 PDT
Maybe!  I don't have any idea what I'm really doing here ...
Comment 3 Dão Gottwald [:dao] 2012-04-19 00:29:18 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> This should probably just be calling browser.focus()?

'browser' is the tabbrowser here. It should be renamed accordingly, 'let browser = tabbrowser.getBrowserForTab(tab);' should be added and then we can focus the browser in the callback function.
Comment 4 Dão Gottwald [:dao] 2012-04-19 00:57:29 PDT
Created attachment 616488 [details] [diff] [review]
better patch?

It seems that we don't even need the setTimeout anymore. This doesn't regress bug 342432 comment 6.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-20 11:52:51 PDT
Comment on attachment 616488 [details] [diff] [review]
better patch?

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

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1475,3 @@
>  
>      // focus the tab's content area
> +    tabbrowser.getBrowserForTab(tab).focus();

Make this `tab.linkedBrowser` and we have a deal.
Comment 6 Dão Gottwald [:dao] 2012-04-21 05:47:10 PDT
http://hg.mozilla.org/mozilla-central/rev/d1ac8e24872c
Comment 7 neil@parkwaycc.co.uk 2012-05-04 09:05:24 PDT
(In reply to Gavin Sharp from comment #1)
> This should probably just be calling browser.focus()?

Indeed, as of the great focus rewrite (somewhere between 3.5 and 4, I forget when) focusing the browser is the way to go (except in certain unrelated cases where you specifically want to raise the window at the same time).

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