Last Comment Bug 1306294 - Restarting browser while Simplify Page mode on, restores 2 new empty tabs
: Restarting browser while Simplify Page mode on, restores 2 new empty tabs
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Printing (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla53
Assigned To: Matheus Longaray (:mlongaray)
: Bogdan Maris, QA [:bogdan_maris]
: Mike Conley (:mconley)
Mentors:
Depends on: 1323987
Blocks: 962433
  Show dependency treegraph
 
Reported: 2016-09-29 07:09 PDT by Bogdan Maris, QA [:bogdan_maris]
Modified: 2017-01-19 10:29 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
wontfix
wontfix
affected
fixed


Attachments
WIP1: Remove not worth-saving tabs when restoring a crashed session (2.17 KB, patch)
2016-10-13 11:25 PDT, Matheus Longaray (:mlongaray)
mconley: feedback-
Details | Diff | Splinter Review
WIP2: Move filtering to SessionSaver._saveState() (2.16 KB, patch)
2016-10-24 06:41 PDT, Matheus Longaray (:mlongaray)
mdeboer: review-
mconley: feedback+
Details | Diff | Splinter Review
WIP3: Move filtering to _collectWindowData() (894 bytes, patch)
2016-10-28 06:28 PDT, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP4: Move filtering to SessionSaver._saveState() (2.16 KB, patch)
2016-11-03 11:42 PDT, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP5: Make selectedWindow attribute concise and adjust session restore tests (8.87 KB, patch)
2016-11-08 04:46 PST, Matheus Longaray (:mlongaray)
mdeboer: review+
Details | Diff | Splinter Review
WIP6: Address nit (9.89 KB, patch)
2016-11-09 05:14 PST, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP7: Fix browser_819510_perwindowpb.js (10.11 KB, patch)
2016-11-11 12:39 PST, Matheus Longaray (:mlongaray)
mdeboer: review-
Details | Diff | Splinter Review
WIP8: Ensure only primary window is opened after browser_625016.js is done (10.69 KB, patch)
2016-11-16 09:22 PST, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP9: Revert to WIP6 fixing only browser_819510_perwindowpb.js (11.25 KB, patch)
2016-11-16 13:20 PST, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP10: Fix browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test (12.71 KB, patch)
2016-11-18 13:38 PST, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP11: Fix browser_newtab_userTypedValue.js (13.76 KB, patch)
2016-11-21 10:20 PST, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review
WIP12: Change browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test (13.54 KB, patch)
2016-11-22 06:30 PST, Matheus Longaray (:mlongaray)
mdeboer: review+
Details | Diff | Splinter Review
WIP13: Fix browser_newtab_userTypedValue.js and browser_urlbarAboutHomeLoading.js (15.73 KB, patch)
2016-12-06 03:30 PST, Matheus Longaray (:mlongaray)
mdeboer: review+
Details | Diff | Splinter Review
WIP14: Add about:printpreview redirector (15.05 KB, patch)
2016-12-22 12:50 PST, Matheus Longaray (:mlongaray)
no flags Details | Diff | Splinter Review

Description User image Bogdan Maris, QA [:bogdan_maris] 2016-09-29 07:09:43 PDT
[Affected versions]:
- Latest Nightly 52.0a1 (2016-09-28)
- Latest Aurora 51.0a2 (2016-09-29)
- Firefox 50.0b2

[Affected platforms]:
- Windows 10 Pro x64
- Windows 7 x64
- Ubuntu 16.04 LTS x64 / x32

[Steps to reproduce]:
1. Start Firefox
2. Visit a page with Reader View (ex: wikipedia.org)
3. Go to File -> Print Preview
4. Click on "Simplify Page" checkbox
5. Restart browser (Shift + F2)

[Expected result]:
- Previous session is restored properly.

[Actual result]:
- “Simplify Page” mode is off and two new empty tabs are restored (one of them has the title of the tab where the “Simplify Page” mode was on).

[Regression range]:
- This is not a regression since the behavior reproduces on Nightly build (2016-06-06), when it first landed.

[Additional notes]:
- When clicking the new empty tab which has the title of the tab where the “Simplify Page” mode was on, that title is no more displayed.
- Only one new empty tab will be displayed when restarting from Print Preview.
Comment 1 User image Bogdan Maris, QA [:bogdan_maris] 2016-09-29 07:28:45 PDT
And here is a screencast with the issue: https://goo.gl/0y7XGw (was to large to upload it directly to bugzilla).
Comment 2 User image Mike Conley (:mconley) 2016-10-07 07:00:33 PDT
We shouldn't be persisting these tabs.

We should be returning false here: http://searchfox.org/mozilla-central/rev/fd672b97f13aa83af5f04caa7b61bd443fb623e9/browser/components/sessionstore/SessionStore.jsm#3892

Which should make sure that the tab is not persisted to the session store. mlongaray, if you have a second, do you think you could see what's going on here? Are we returning true for this SessionStore method when we should be returning false? If so, why?
Comment 3 User image Matheus Longaray (:mlongaray) 2016-10-11 06:32:06 PDT
Hey Mike, I managed to debug SessionStore.jsm and could confirm the following.

This method (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3892) does return false indeed when print-preview-related-tabs are closed. However, this method is only called when there is a 'close' event. If the browser closes abruptly, this method does not get called.

When the browser starts up again, it will end up on this piece of code (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#1192) in which inits a Session with, what I presumed to be, all windows/tabs that were open prior crashing.

initSession() checks if the previous session crashed, so it can show the about:sessionrestore page (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#548).

What do you recommend for us to do? Should we remove unwanted tabs from the initial state? (http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#523)
Comment 4 User image Ritu Kothari (:ritu) 2016-10-11 15:57:58 PDT
I was able to repro this and I think we should consider fixing this in 51 and perhaps uplifting to 50 if the fix is simple enough. Not sure if P3 is appropriate here, seems more like a P2 to me i.e. fix in the next cycle like 51.
Comment 5 User image Mike Conley (:mconley) 2016-10-13 08:07:45 PDT
Hey mlongaray - you pinged me in IRC saying that you had a patch. Can I see it?
Comment 6 User image Matheus Longaray (:mlongaray) 2016-10-13 11:25:24 PDT
Created attachment 8800792 [details] [diff] [review]
WIP1: Remove not worth-saving tabs when restoring a crashed session

When recovering from a crash, we restore all windows and tabs from last session. This patch filters out not worth-saving tabs, i.e., tabs that only have a transient about: entry, no other session history, and no userTypedValue.
Comment 7 User image Ritu Kothari (:ritu) 2016-10-13 12:51:52 PDT
We've made a decision to turn off "Simplify Page" feature on Fx50. This is now a wontfix for that version.
Comment 8 User image Mike Conley (:mconley) 2016-10-17 13:19:06 PDT
Comment on attachment 8800792 [details] [diff] [review]
WIP1: Remove not worth-saving tabs when restoring a crashed session

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

Hm. This kinda seems like wallpapering over a problem. Why are we even storing the state if the state would be uninteresting if the tab or window closed?

Hey mikedeboer, do you know of a good reason why we only compute _shouldSaveTabState on window or tab close, and not when we're otherwise getting tab or window state?

Like, in _collectWindowData, we go ahead and collect each tab's state, regardless of _shouldSaveTabState... should we perhaps do a _shouldSaveTabState there as well? I think that'd be preferable over filtering out things that we don't want to persist after having already reloaded it off the disk.
Comment 9 User image Mike Conley (:mconley) 2016-10-17 13:20:01 PDT
ni? to mikedeboer for comment 8. It's cool if you're not sure - I know _shouldSaveTabState is from before you were module owner. We might be able to redirect to ttaubert, but thought I'd check with you first.
Comment 10 User image Mike de Boer [:mikedeboer] 2016-10-18 07:08:26 PDT
Yes, I'd say that `_shouldSaveTabState` can/ should be used in `_collectWindowData` too, but I'd like to learn from Tim why that's not the case currently.
Comment 11 User image Tim Taubert [:ttaubert] 2016-10-22 03:15:15 PDT
I think this is mainly due to the semantic difference between collecting window data and saving it. The function is called `_shouldSaveTabState()` because it tells you whether we deem that tab's data worth saving, although of course we also use it to determine whether it will show up in the list of closed tabs for a window, or a window will go into the list of closed windows.

`SessionStore.getBrowserState()` or `.getWindowState()` returns the in-memory state, and doesn't filter tabs. Whether that's important I don't know, but there of course might be add-ons that depend on this. There won't be WebExtensions depending on this though, but maybe there are other consumers inside Firefox.

So either you can change `_collectWindowData()`, or you can filter with `_shouldSaveTabState()` again in `SessionSaver._saveState()`.
Comment 12 User image Matheus Longaray (:mlongaray) 2016-10-24 06:41:22 PDT
Created attachment 8803865 [details] [diff] [review]
WIP2: Move filtering to SessionSaver._saveState()

This patch moves filtering of not worth-saving tabs to SessionSaver._saveState()
Comment 13 User image Mike Conley (:mconley) 2016-10-27 08:29:47 PDT
Comment on attachment 8803865 [details] [diff] [review]
WIP2: Move filtering to SessionSaver._saveState()

The general idea looks okay to me. I think you'll want mikedeboer to review it though.

Thanks!
Comment 14 User image Matheus Longaray (:mlongaray) 2016-10-27 08:53:34 PDT
Comment on attachment 8803865 [details] [diff] [review]
WIP2: Move filtering to SessionSaver._saveState()

Asking for review from mikedeboer after mconley granted feedback.
Comment 15 User image Mike de Boer [:mikedeboer] 2016-10-28 04:31:39 PDT
Comment on attachment 8803865 [details] [diff] [review]
WIP2: Move filtering to SessionSaver._saveState()

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

I'm sorry, but if it's at all possible, I'd like the change to happen in `_collectWindowData()`.
Essentially, from this:
```js
// update the internal state data for this window
for (let tab of tabs) {
  let tabData = TabState.collect(tab);
  tabMap.set(tab, tabData);
  tabsData.push(tabData);
}
```

to:

```js
// Update the internal state data for this window.
for (let tab of tabs) {
  let tabData = TabState.collect(tab);
  if (!this._shouldSaveTabState(tabData))
    continue;
  tabMap.set(tab, tabData);
  tabsData.push(tabData);
}
```

This might seem scary, but hey, those are the changes most fun to work on!
When you push this change to try - after you've confirmed that it fixes this bug too - you'll find out quickly enough whether works across the board.
Comment 16 User image Mike de Boer [:mikedeboer] 2016-10-28 04:44:46 PDT
BTW, I'm here to help - you can find me on IRC in #developers or #fx-team to discuss things!
Comment 17 User image Matheus Longaray (:mlongaray) 2016-10-28 06:28:33 PDT
Created attachment 8805535 [details] [diff] [review]
WIP3: Move filtering to _collectWindowData()

This patch moves filtering of not worth-saving tabs to SessionStoreInternal._collectWindowData()
Comment 18 User image Mike de Boer [:mikedeboer] 2016-10-28 07:22:40 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4027c9c66168
Comment 19 User image Mike de Boer [:mikedeboer] 2016-10-28 15:18:48 PDT
According to that try run we have work to do! So what I'll do asap is analyze the results and see if we can fix the tests that are failing right now. 'browser_urlbarAboutHomeLoading.js' is making me a bit worried, because that's a behavior regression test.
I think that's something you'll want me to do, am I right?
There are two scenarios:
1) I can find simple fixes for all the tests
2) I can't and we can use your previous patch that'll filter the state dictionary.
Comment 20 User image Mike de Boer [:mikedeboer] 2016-11-01 06:14:36 PDT
Comment on attachment 8805535 [details] [diff] [review]
WIP3: Move filtering to _collectWindowData()

Please re-request review when you've got a new patch ready for me to look at. Thanks!
Comment 21 User image Matheus Longaray (:mlongaray) 2016-11-03 11:42:49 PDT
Created attachment 8807243 [details] [diff] [review]
WIP4: Move filtering to SessionSaver._saveState()

As discussed on IRC with mikedeboer, we have decided to go back to this approach given its a lot less invasive. This patch moves filtering of not worth-saving tabs to SessionSaver._saveState().
Comment 22 User image Mike de Boer [:mikedeboer] 2016-11-04 07:13:32 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64bcddecbbf5
Comment 23 User image Matheus Longaray (:mlongaray) 2016-11-08 04:46:55 PST
Created attachment 8808591 [details] [diff] [review]
WIP5: Make selectedWindow attribute concise and adjust session restore tests

This patch addresses concerns raised about selectedWindow and selected attributes if we happen to remove a not-worthy tab that is currently selected. Since we will get rid of not-worthy tabs from our session state, this patch also adjusts a few tests as needed.
Comment 24 User image Mike de Boer [:mikedeboer] 2016-11-08 05:20:36 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f029d359094f
Comment 25 User image Mike de Boer [:mikedeboer] 2016-11-08 05:38:55 PST
Comment on attachment 8808591 [details] [diff] [review]
WIP5: Make selectedWindow attribute concise and adjust session restore tests

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

Pending a green try run, r=me!

::: browser/components/sessionstore/SessionSaver.jsm
@@ +185,5 @@
>      stopWatchStart("COLLECT_DATA_MS", "COLLECT_DATA_LONGEST_OP_MS");
>      let state = SessionStore.getCurrentState(forceUpdateAllWindows);
>      PrivacyFilter.filterPrivateWindowsAndTabs(state);
>  
> +    // Make sure we only write worth saving tabs to disk

nit: missing dot.
Comment 26 User image Matheus Longaray (:mlongaray) 2016-11-09 05:14:48 PST
Created attachment 8808983 [details] [diff] [review]
WIP6: Address nit

This patch addresses missing dot.
Comment 27 User image Matheus Longaray (:mlongaray) 2016-11-09 05:32:57 PST
Status:

Last try run is showing that browser/components/sessionstore/test/browser_819510_perwindowpb.js is failing on Linux. However, this test is passing when I run it locally and it does makes sense to pass. This same test fails on the other platforms.

browser/components/sessionstore/test/browser_newtab_userTypedValue.js is failing on OS X, and when I run it locally on Linux, sometimes it works as expected, sometimes it does not (after different builds without any code change on the test). Maybe a timing issue?
Comment 28 User image Matheus Longaray (:mlongaray) 2016-11-11 12:39:51 PST
Created attachment 8809930 [details] [diff] [review]
WIP7: Fix browser_819510_perwindowpb.js

This patch fixes an issue with browser_819510_perwindowpb.js test.
Comment 29 User image Mike de Boer [:mikedeboer] 2016-11-11 12:44:07 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a53fd096f0f
Comment 30 User image Mike de Boer [:mikedeboer] 2016-11-14 04:20:11 PST
Comment on attachment 8809930 [details] [diff] [review]
WIP7: Fix browser_819510_perwindowpb.js

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

Matheus, doesn't seem like the changes you made are quite enough yet! :(
On OSX, we've got a failing browser/components/sessionstore/test/browser_scrollPositions.js (non-e10s) and a failing browser/components/sessionstore/test/browser_newtab_userTypedValue.js (e10s).
On Windows we've got a failing browser/components/sessionstore/test/browser_scrollPositionsReaderMode.js and browser/components/sessionstore/test/browser_scrollPositions.js (non-e10s) and a failing browser/components/sessionstore/test/browser_newtab_userTypedValue.js (e10s).

I'm r- the patch just because of this, so once you've got everything green, this'll automatically turn into an r+.
Comment 31 User image Matheus Longaray (:mlongaray) 2016-11-16 09:22:45 PST
Created attachment 8811352 [details] [diff] [review]
WIP8: Ensure only primary window is opened after browser_625016.js is done
Comment 32 User image Mike de Boer [:mikedeboer] 2016-11-16 10:13:34 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=384ad175f5c8
Comment 33 User image Matheus Longaray (:mlongaray) 2016-11-16 13:20:08 PST
Created attachment 8811467 [details] [diff] [review]
WIP9: Revert to WIP6 fixing only browser_819510_perwindowpb.js
Comment 34 User image Mike de Boer [:mikedeboer] 2016-11-16 13:24:48 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f41f79a51ca3
Comment 35 User image Matheus Longaray (:mlongaray) 2016-11-18 13:38:56 PST
Created attachment 8812340 [details] [diff] [review]
WIP10: Fix browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test

Following is the progress for WIP10 build on Treeherder.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57cdf373a4939647b997749f8c9549665bf9c9cc&selectedJob=31469395

If you have the time, could you take a look at these results - Mike? I believe we would only have browser_newtab_userTypedValue.js test pending to fix.
Comment 36 User image Mike de Boer [:mikedeboer] 2016-11-21 08:24:01 PST
These results are looking good to, but I don't understand why browser_newtab_userTypedValue.js is failing, because it's passing just fine for me locally (OSX 10.12 debug build), so I'm not sure what's up there...
With that try push I'm also not seeing the commit(s) you pushed to try, so I'm curious what you were running there.
Comment 37 User image Matheus Longaray (:mlongaray) 2016-11-21 09:18:15 PST
(In reply to Mike de Boer [:mikedeboer] from comment #36)
> These results are looking good to, but I don't understand why
> browser_newtab_userTypedValue.js is failing, because it's passing just fine
> for me locally (OSX 10.12 debug build), so I'm not sure what's up there...

Yes, same for me. browser_newtab_userTypedValue.js is failing locally for me but passing on try.

> With that try push I'm also not seeing the commit(s) you pushed to try, so
> I'm curious what you were running there.

Here's what I pushed: https://hg.mozilla.org/try/rev/be22a9cfa9af
Comment 38 User image Matheus Longaray (:mlongaray) 2016-11-21 10:20:52 PST
Created attachment 8812845 [details] [diff] [review]
WIP11: Fix browser_newtab_userTypedValue.js

Following is the progress for WIP11 build on Treeherder.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dbf1b23473e63e696eb0dfe20599cdda665c45b
Comment 39 User image Matheus Longaray (:mlongaray) 2016-11-21 13:11:39 PST
Comment on attachment 8812845 [details] [diff] [review]
WIP11: Fix browser_newtab_userTypedValue.js

Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer
Comment 40 User image Mike de Boer [:mikedeboer] 2016-11-22 03:30:49 PST
Comment on attachment 8812845 [details] [diff] [review]
WIP11: Fix browser_newtab_userTypedValue.js

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

::: browser/components/sessionstore/test/browser_scrollPositions.js
@@ +112,5 @@
>  add_task(function test_scroll_background_tabs() {
>    pushPrefs(["browser.sessionstore.restore_on_demand", true]);
>  
>    let newWin = yield BrowserTestUtils.openNewBrowserWindow();
> +  BrowserTestUtils.loadURI(newWin.gBrowser, "http://www.example.com/");

Why did you put this change here and why did it help?
(This is not correct, because you're passing in `gBrowser` and not a xul:browser instance that's attached to a tab.)

::: browser/components/sessionstore/test/browser_scrollPositionsReaderMode.js
@@ +20,5 @@
>  add_task(function test_scroll_background_about_reader_tabs() {
>    pushPrefs(["browser.sessionstore.restore_on_demand", true]);
>  
>    let newWin = yield BrowserTestUtils.openNewBrowserWindow();
> +  BrowserTestUtils.loadURI(newWin.gBrowser, "http://www.example.com/");

Same here.
Comment 41 User image Matheus Longaray (:mlongaray) 2016-11-22 06:30:05 PST
Created attachment 8813186 [details] [diff] [review]
WIP12: Change browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test

Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer

Following is the progress for WIP12 build on Treeherder.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dbf1b23473e63e696eb0dfe20599cdda665c45b
Comment 43 User image Mike de Boer [:mikedeboer] 2016-11-22 06:50:13 PST
Comment on attachment 8813186 [details] [diff] [review]
WIP12: Change browser_scrollPositions.js and browser_scrollPositionsReaderMode.js test

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

r=me! \o/ Great job, Matheus, persevering and pulling this through the end!
I hope it'll stick when it lands. Fingers crossed.

I'm looking forward to seeing more of your work!
Comment 44 User image Pulsebot 2016-11-23 00:22:29 PST
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b75d4672954
"Restarting browser while Simplify Page mode on, restores 2 new empty tabs". r=mdeboer
Comment 45 User image Carsten Book [:Tomcat] 2016-11-23 03:44:33 PST
sorry had to back this out for new test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39706918&repo=mozilla-inbound
Comment 46 User image Pulsebot 2016-11-23 03:54:18 PST
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4772a0f4bed2
Backed out changeset 4b75d4672954 for bc5 test failures
Comment 47 User image Matheus Longaray (:mlongaray) 2016-11-23 05:36:24 PST
(In reply to Carsten Book [:Tomcat] from comment #45)
> sorry had to back this out for new test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=39706918&repo=mozilla-
> inbound

No worries! Let's see if we can find out what's going on there. Thanks for letting us know.
Comment 48 User image Matheus Longaray (:mlongaray) 2016-12-06 03:30:17 PST
Created attachment 8816973 [details] [diff] [review]
WIP13: Fix browser_newtab_userTypedValue.js and browser_urlbarAboutHomeLoading.js

Commit message: Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer

Our last patch set had to be backed out due some bc test failures (browser_newtab_userTypedValue.js & browser_urlbarAboutHomeLoading.js)

Following is the progress for WIP13 build on Treeherder. I re-triggered the respective bc test suites 10 times more for all platforms. No failures this time.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b35b53b5eee412065071a401c54ccdc8b7d335a&group_state=expanded
Comment 49 User image Matheus Longaray (:mlongaray) 2016-12-14 05:07:12 PST
Comment on attachment 8816973 [details] [diff] [review]
WIP13: Fix browser_newtab_userTypedValue.js and browser_urlbarAboutHomeLoading.js

Commit message: Bug 1306294 - Filters out not worth-saving tabs from session state. r=mikedeboer
Comment 50 User image Pulsebot 2016-12-14 18:53:33 PST
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe1b40baf57
Filters out not worth-saving tabs from session state. r=mikedeboer
Comment 51 User image Carsten Book [:Tomcat] 2016-12-15 05:23:56 PST
https://hg.mozilla.org/mozilla-central/rev/cbe1b40baf57
Comment 52 User image Matheus Longaray (:mlongaray) 2016-12-22 12:50:12 PST
Created attachment 8821283 [details] [diff] [review]
WIP14: Add about:printpreview redirector

As discussed on bug 1323987, we are not allowed to remove "about:blank" and "about:newtab" tabs from our session state when saving it to disk - these tabs should be persisted.

Since preview opens up "about:blank" tab in background to put the document into print preview mode, we are adding a new entry in our about: redirector called "about:printpreview". In this way, when we are print previewing and session store kicks in to save our opened tabs to disk, session store will discard this kind of tab - not saving it to disk.

This patch updates our browser code to use about:printpreview and makes expected changes to SessionStore. It also updates the internal notion of nsDocShell to manually set our document's URI when creating a blank document for print preview content viewer.
Comment 53 User image Matheus Longaray (:mlongaray) 2016-12-22 12:59:59 PST
Comment on attachment 8821283 [details] [diff] [review]
WIP14: Add about:printpreview redirector

On -175,17 +180,19 @@ AboutRedirector::NewChannel(nsIURI* aURI:

I tested URI_NORELATIVE flag (protocol doesn't support relative URIs, e.g., about and javascript) instead and it happens to work in our case. What I'm not sure is that if we are going to be causing repercussions using this flag - other than about:printpreview. See more at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProtocolHandler.
Comment 54 User image Mike de Boer [:mikedeboer] 2017-01-02 08:45:03 PST
Comment on attachment 8821283 [details] [diff] [review]
WIP14: Add about:printpreview redirector

Matheus,

Overall this patch looks like it's going in the right direction, but it doesn't belong to this bug.
Can you attach it to bug 1323987 instead?
Can you also put a treeherder link to the try job you pushed or are about to push?

Thanks!
Comment 55 User image Matheus Longaray (:mlongaray) 2017-01-04 03:42:36 PST
Sorry, Mike - my mistake. I'll move this patch to bug 1323987 as well as post the treeherder link.
Comment 56 User image Gerry Chang [:gchang] 2017-01-17 23:21:03 PST
We've built 51 RC and it's too late for 51. Mark 51 won't fix.

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