Closed Bug 1096804 Opened 5 years ago Closed 4 years ago

[e10s] Support windows taskbar previews and thumbnails

Categories

(Core :: Widget: Win32, defect)

36 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s m8+ ---
firefox46 --- fixed

People

(Reporter: drunkenf00l, Assigned: jimm)

References

(Blocks 1 open bug)

Details

(Keywords: productwanted)

Attachments

(8 files, 17 obsolete files)

50.56 KB, image/png
Details
103.60 KB, image/png
Details
25.04 KB, patch
dao
: review+
Details | Diff | Splinter Review
12.11 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.95 KB, patch
dao
: review+
Details | Diff | Splinter Review
12.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.65 KB, patch
dao
: review+
Details | Diff | Splinter Review
8.53 KB, patch
dao
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141110030204

Steps to reproduce:

Enable e10s


Actual results:

There is only one tab per open window in the Windows taskbar


Expected results:

There should have been one tab per tab open in Firefox. The two attached screenshots demonstrate what I mean. The same tabs are opened in both screenshot. One screenshot shows what happens when e10s is enabled and the other with it disabled. The screenshot showing multiple tabs is what I get with e10s disabled and is the expected result.
The expected result
tracking-e10s: --- → ?
Felipe, Bill says you told him this feature was turned off. True?
Flags: needinfo?(felipc)
To me, it looks the same both with e10s ON/OFF.
36.0a1 (2014-11-17) Win 7 x64
Component: Untriaged → Shell Integration
Maybe it's one of my settings?

Important Modified Preferences
------------------------------

accessibility.typeaheadfind.flashBar: 0
browser.cache.disk.capacity: 358400
browser.cache.disk.smart_size_cached_value: 358400
browser.cache.disk.smart_size.first_run: false
browser.cache.disk.smart_size.use_old_max: false
browser.cache.frecency_experiment: 1
browser.fixup.domainwhitelist.192.168.1.88: true
browser.link.open_newwindow.override.external: 2
browser.places.importBookmarksHTML: false
browser.places.importDefaults: false
browser.places.leftPaneFolderId: -1
browser.places.migratePostDataAnnotations: false
browser.places.smartBookmarksVersion: 7
browser.places.updateRecentTagsUri: false
browser.privatebrowsing.dont_prompt_on_enter: true
browser.sessionstore.upgradeBackup.latestBuildID: 20141111030203
browser.startup.homepage_override.buildID: 20141116030212
browser.startup.homepage_override.mstone: 36.0a1
browser.tabs.remote.autostart: true
browser.tabs.remote.autostart.1: false
dom.ipc.plugins.processLaunchTimeoutSecs: 5
dom.ipc.plugins.timeoutSecs: 5
dom.ipc.processCount: 5
dom.mozApps.used: true
dom.w3c_touch_events.expose: false
extensions.lastAppVersion: 36.0a1
font.internaluseonly.changed: false
general.useragent.extra.microsoftdotnet: ( .NET CLR 3.5.30729; .NET4.0E)
gfx.color_management.mode: 1
gfx.direct3d.last_used_feature_level_idx: 0
gfx.direct3d.prefer_10_1: true
media.gmp-gmpopenh264.lastUpdate: 1412201450
media.gmp-gmpopenh264.version: 1.1
media.gmp-manager.lastCheck: 1416363471
network.cookie.prefsMigrated: true
network.image.imageBehavior: 0
network.protocol-handler.warn-external.dnupdate: false
places.database.lastMaintenance: 1416304644
places.history.expiration.transient_current_max_pages: 104858
places.history.expiration.transient_optimal_database_size: 167772160
places.last_vacuum: 1307849125
plugin.disable_full_page_plugin_for_types: application/pdf
plugin.importedState: true
plugin.state.flash: 1
plugin.state.npauthz: 0
plugin.state.npctrl: 1
plugin.state.npfbplugin: 0
plugin.state.npgeplugin: 0
plugin.state.npnv3dv: 1
plugin.state.npnv3dvstreaming: 1
plugin.state.npolgdet: 0
plugin.state.nppicasa: 0
plugin.state.npqtplugin: 1
plugin.state.npspwrap: 0
plugin.state.npunity3d: 1
plugin.state.npuplaypc: 0
plugin.state.npuplaypchub: 1
plugin.state.npwlpg: 0
print.print_printer: HPE8CAC5 (HP Photosmart Premium C309g-m)
print.printer_CutePDF_Writer.print_bgcolor: false
print.printer_CutePDF_Writer.print_bgimages: false
print.printer_CutePDF_Writer.print_colorspace:
print.printer_CutePDF_Writer.print_command:
print.printer_CutePDF_Writer.print_downloadfonts: false
print.printer_CutePDF_Writer.print_duplex: -2130280380
print.printer_CutePDF_Writer.print_edge_bottom: 0
print.printer_CutePDF_Writer.print_edge_left: 0
print.printer_CutePDF_Writer.print_edge_right: 0
print.printer_CutePDF_Writer.print_edge_top: 0
print.printer_CutePDF_Writer.print_evenpages: true
print.printer_CutePDF_Writer.print_footercenter:
print.printer_CutePDF_Writer.print_footerleft: &PT
print.printer_CutePDF_Writer.print_footerright: &D
print.printer_CutePDF_Writer.print_headercenter:
print.printer_CutePDF_Writer.print_headerleft: &T
print.printer_CutePDF_Writer.print_headerright: &U
print.printer_CutePDF_Writer.print_in_color: true
print.printer_CutePDF_Writer.print_margin_bottom: 0.5
print.printer_CutePDF_Writer.print_margin_left: 0.5
print.printer_CutePDF_Writer.print_margin_right: 0.5
print.printer_CutePDF_Writer.print_margin_top: 0.5
print.printer_CutePDF_Writer.print_oddpages: true
print.printer_CutePDF_Writer.print_orientation: 0
print.printer_CutePDF_Writer.print_page_delay: 50
print.printer_CutePDF_Writer.print_paper_data: 1
print.printer_CutePDF_Writer.print_paper_height: 11.00
print.printer_CutePDF_Writer.print_paper_name:
print.printer_CutePDF_Writer.print_paper_size_type: 0
print.printer_CutePDF_Writer.print_paper_size_unit: 0
print.printer_CutePDF_Writer.print_paper_width: 8.50
print.printer_CutePDF_Writer.print_plex_name:
print.printer_CutePDF_Writer.print_resolution: -2146270936
print.printer_CutePDF_Writer.print_resolution_name:
print.printer_CutePDF_Writer.print_reversed: false
print.printer_CutePDF_Writer.print_scaling: 1.00
print.printer_CutePDF_Writer.print_shrink_to_fit: true
print.printer_CutePDF_Writer.print_to_file: false
print.printer_CutePDF_Writer.print_unwriteable_margin_bottom: 0
print.printer_CutePDF_Writer.print_unwriteable_margin_left: 0
print.printer_CutePDF_Writer.print_unwriteable_margin_right: 0
print.printer_CutePDF_Writer.print_unwriteable_margin_top: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_bgcolor: false
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_bgimages: false
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_colorspace:
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_command:
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_downloadfonts: false
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_duplex: 1515870810
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_edge_bottom: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_edge_left: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_edge_right: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_edge_top: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_evenpages: true
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_footercenter:
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_footerleft: &PT
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_footerright: &D
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_headercenter:
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_headerleft: &T
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_headerright: &U
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_in_color: true
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_margin_bottom: 0.5
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_margin_left: 0.5
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_margin_right: 0.5
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_margin_top: 0.5
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_oddpages: true
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_orientation: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_page_delay: 50
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_pagedelay: 500
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_paper_data: 1
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_paper_height: 11.00
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_paper_name:
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_paper_size_type: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_paper_size_unit: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_paper_width: 8.50
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_plex_name:
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_resolution: 1515870810
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_resolution_name:
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_reversed: false
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_scaling: 1.00
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_shrink_to_fit: true
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_to_file: false
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_unwriteable_margin_bottom: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_unwriteable_margin_left: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_unwriteable_margin_right: 0
print.printer_HPE8CAC5_(HP_Photosmart_Premium_C309g-m).print_unwriteable_margin_top: 0
print.printer_L019_on_PRINT_(redirected_1).print_bgcolor: false
print.printer_L019_on_PRINT_(redirected_1).print_bgimages: false
print.printer_L019_on_PRINT_(redirected_1).print_colorspace:
print.printer_L019_on_PRINT_(redirected_1).print_command:
print.printer_L019_on_PRINT_(redirected_1).print_downloadfonts: false
print.printer_L019_on_PRINT_(redirected_1).print_duplex: 0
print.printer_L019_on_PRINT_(redirected_1).print_edge_bottom: 0
print.printer_L019_on_PRINT_(redirected_1).print_edge_left: 0
print.printer_L019_on_PRINT_(redirected_1).print_edge_right: 0
print.printer_L019_on_PRINT_(redirected_1).print_edge_top: 0
print.printer_L019_on_PRINT_(redirected_1).print_evenpages: true
print.printer_L019_on_PRINT_(redirected_1).print_footercenter:
print.printer_L019_on_PRINT_(redirected_1).print_footerleft: &PT
print.printer_L019_on_PRINT_(redirected_1).print_footerright: &D
print.printer_L019_on_PRINT_(redirected_1).print_headercenter:
print.printer_L019_on_PRINT_(redirected_1).print_headerleft: &T
print.printer_L019_on_PRINT_(redirected_1).print_headerright: &U
print.printer_L019_on_PRINT_(redirected_1).print_in_color: true
print.printer_L019_on_PRINT_(redirected_1).print_margin_bottom: 0.5
print.printer_L019_on_PRINT_(redirected_1).print_margin_left: 0.5
print.printer_L019_on_PRINT_(redirected_1).print_margin_right: 0.5
print.printer_L019_on_PRINT_(redirected_1).print_margin_top: 0.5
print.printer_L019_on_PRINT_(redirected_1).print_oddpages: true
print.printer_L019_on_PRINT_(redirected_1).print_orientation: 0
print.printer_L019_on_PRINT_(redirected_1).print_page_delay: 50
print.printer_L019_on_PRINT_(redirected_1).print_paper_data: 1
print.printer_L019_on_PRINT_(redirected_1).print_paper_height: 11.00
print.printer_L019_on_PRINT_(redirected_1).print_paper_name:
print.printer_L019_on_PRINT_(redirected_1).print_paper_size_type: 0
print.printer_L019_on_PRINT_(redirected_1).print_paper_size_unit: 0
print.printer_L019_on_PRINT_(redirected_1).print_paper_width: 8.50
print.printer_L019_on_PRINT_(redirected_1).print_plex_name:
print.printer_L019_on_PRINT_(redirected_1).print_resolution: 0
print.printer_L019_on_PRINT_(redirected_1).print_resolution_name:
print.printer_L019_on_PRINT_(redirected_1).print_reversed: false
print.printer_L019_on_PRINT_(redirected_1).print_scaling: 1.00
print.printer_L019_on_PRINT_(redirected_1).print_shrink_to_fit: true
print.printer_L019_on_PRINT_(redirected_1).print_to_file: false
print.printer_L019_on_PRINT_(redirected_1).print_unwriteable_margin_bottom: 0
print.printer_L019_on_PRINT_(redirected_1).print_unwriteable_margin_left: 0
print.printer_L019_on_PRINT_(redirected_1).print_unwriteable_margin_right: 0
print.printer_L019_on_PRINT_(redirected_1).print_unwriteable_margin_top: 0
print.printer_Microsoft_XPS_Document_Writer.print_bgcolor: false
print.printer_Microsoft_XPS_Document_Writer.print_bgimages: false
print.printer_Microsoft_XPS_Document_Writer.print_colorspace:
print.printer_Microsoft_XPS_Document_Writer.print_command:
print.printer_Microsoft_XPS_Document_Writer.print_downloadfonts: false
print.printer_Microsoft_XPS_Document_Writer.print_duplex: 1515870810
print.printer_Microsoft_XPS_Document_Writer.print_edge_bottom: 0
print.printer_Microsoft_XPS_Document_Writer.print_edge_left: 0
print.printer_Microsoft_XPS_Document_Writer.print_edge_right: 0
print.printer_Microsoft_XPS_Document_Writer.print_edge_top: 0
print.printer_Microsoft_XPS_Document_Writer.print_evenpages: true
print.printer_Microsoft_XPS_Document_Writer.print_footercenter:
print.printer_Microsoft_XPS_Document_Writer.print_footerleft: &PT
print.printer_Microsoft_XPS_Document_Writer.print_footerright: &D
print.printer_Microsoft_XPS_Document_Writer.print_headercenter:
print.printer_Microsoft_XPS_Document_Writer.print_headerleft: &T
print.printer_Microsoft_XPS_Document_Writer.print_headerright: &U
print.printer_Microsoft_XPS_Document_Writer.print_in_color: true
print.printer_Microsoft_XPS_Document_Writer.print_margin_bottom: 0.5
print.printer_Microsoft_XPS_Document_Writer.print_margin_left: 0.5
print.printer_Microsoft_XPS_Document_Writer.print_margin_right: 0.5
print.printer_Microsoft_XPS_Document_Writer.print_margin_top: 0.5
print.printer_Microsoft_XPS_Document_Writer.print_oddpages: true
print.printer_Microsoft_XPS_Document_Writer.print_orientation: 0
print.printer_Microsoft_XPS_Document_Writer.print_page_delay: 50
print.printer_Microsoft_XPS_Document_Writer.print_paper_data: 1
print.printer_Microsoft_XPS_Document_Writer.print_paper_height: 11.00
print.printer_Microsoft_XPS_Document_Writer.print_paper_name:
print.printer_Microsoft_XPS_Document_Writer.print_paper_size_type: 0
print.printer_Microsoft_XPS_Document_Writer.print_paper_size_unit: 0
print.printer_Microsoft_XPS_Document_Writer.print_paper_width: 8.50
print.printer_Microsoft_XPS_Document_Writer.print_plex_name:
print.printer_Microsoft_XPS_Document_Writer.print_resolution: 1515870810
print.printer_Microsoft_XPS_Document_Writer.print_resolution_name:
print.printer_Microsoft_XPS_Document_Writer.print_reversed: false
print.printer_Microsoft_XPS_Document_Writer.print_scaling: 1.00
print.printer_Microsoft_XPS_Document_Writer.print_shrink_to_fit: true
print.printer_Microsoft_XPS_Document_Writer.print_to_file: false
print.printer_Microsoft_XPS_Document_Writer.print_unwriteable_margin_bottom: 0
print.printer_Microsoft_XPS_Document_Writer.print_unwriteable_margin_left: 0
print.printer_Microsoft_XPS_Document_Writer.print_unwriteable_margin_right: 0
print.printer_Microsoft_XPS_Document_Writer.print_unwriteable_margin_top: 0
privacy.cpd.cookies: false
privacy.cpd.downloads: false
privacy.cpd.formdata: false
privacy.cpd.history: false
privacy.cpd.sessions: false
privacy.sanitize.migrateFx3Prefs: true
privacy.sanitize.timeSpan: 0
security.warn_viewing_mixed: false
storage.vacuum.last.index: 1
storage.vacuum.last.places.sqlite: 1413934972
webgl.prefer-native-gl: true
The only one I thought might have an effect was dom.ipc.processCount. I tried reverting it, but it made no difference. Same bugged behavior with all extensions disabled as well.

Application Basics
------------------

Name: Firefox
Version: 36.0a1
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Multiprocess Windows: 2/2

Graphics
--------

Adapter Description: NVIDIA GeForce GTX 770
Adapter Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Adapter RAM: 4095
Device ID: 0x1184
Direct2D Enabled: true
DirectWrite Enabled: true (6.3.9600.17111)
Driver Date: 10-29-2014
Driver Version: 9.18.13.4460
GPU #2 Active: false
GPU Accelerated Windows: 2/2 Direct3D 11 (OMTC)
Subsys ID: 37783842
Vendor ID: 0x10de
WebGL Renderer: Google Inc. -- ANGLE (NVIDIA GeForce GTX 770 Direct3D9Ex vs_3_0 ps_3_0)
windowLayerManagerRemote: true
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0
Component: Shell Integration → Widget: Win32
Product: Firefox → Core
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #2)
> Felipe, Bill says you told him this feature was turned off. True?

Blassey, that's right, the feature is disabled by default. However, it has a visible checkbox in the Preferences dialog for Windows users.

I think the best would be to measure how many users are using it to make an informed decision. But I imagine it's low. There have always been non-fixed crashers with this feature, and IE also used this in one of their versions but then went back and turned it off as it's a really confusing experience.
Flags: needinfo?(felipc)
In non-e10s mode the "show tab previews in the Windows taskbar" works. If it is checked, you get tab previews.

In e10s mode, toggling doesn't make a difference. Tab previews are not shown.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → jmathies
Duplicate of this bug: 1149698
Summary: Only one tab per window appears when hovering the icon in the Windows taskbar while e10s is enabled → [e10s] Support windows taskbar previews and thumbnails
FWIW there are two tests that cover this functionality:
 - browser/modules/test/browser_taskbar_preview.js
 - widget/tests/taskbar_previews.xul

The latter is disabled and the former disabled on e10s, because they need some fixes, but it might help you on testing or save from writing tests..

Some details on when taskbar_previews.xul was disabled in https://bugzilla.mozilla.org/show_bug.cgi?id=605813#c13
product-wanted to ask if we should block on this non-default behavior, or just remove the option.
Keywords: productwanted
Attached patch wip (obsolete) — Splinter Review
A couple issues to sort out here still, we lost MozAfterPaint on browser elements, so refresh doesn't work currently. I'm thinking of using a simple timer to refresh occasionally for the tab in the foreground. Also my new graphics code isn't handling aero glass properly yet, need to dig into that.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> product-wanted to ask if we should block on this non-default behavior, or
> just remove the option.

If we consider killing this feature, I think we should accumulate telemetry on beta or release to find out how many users actually turn this on. This is a neat Windows feature chrome does not support by default.
Attached patch wip (obsolete) — Splinter Review
transparency and refresh issues worked out.
Attachment #8689213 - Attachment is obsolete: true
Attached patch rollup (obsolete) — Splinter Review
Attachment #8689510 - Attachment is obsolete: true
Bug 1096804 - Update WindowsPreviewPerTab to support e10s. r?dao
Attachment #8690225 - Flags: review?(dao)
Bug 1096804 - Add support to PageThumbs for requesting non-downlampled window screenshots. r?dao
Attachment #8690226 - Flags: review?(dao)
Bug 1096804 - Add a new asynchronous callback interface for thumbnail and preview image requests.
Bug 1096804 - Update taskbar preview image manipulation code so that it can work with a passed in canvas vs. a locally stored canvas.
Comment on attachment 8690225 [details]
MozReview Request: Bug 1096804 - Update WindowsPreviewPerTab to support e10s. r?dao

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25799/diff/1-2/
Attachment #8690226 - Attachment is obsolete: true
Attachment #8690226 - Flags: review?(dao)
Attachment #8690227 - Attachment is obsolete: true
Attachment #8690228 - Attachment is obsolete: true
hmm, rats. sorry for the spam, I was hoping that would update just one of the requests.
Bug 1096804 - Add support to PageThumbs for requesting non-downlampled window screenshots. r?dao
Attachment #8690233 - Flags: review?(dao)
Bug 1096804 - Add a new asynchronous callback interface for thumbnail and preview image requests.
Bug 1096804 - Update taskbar preview image manipulation code so that it can work with a passed in canvas vs. a locally stored canvas.
Comment on attachment 8690225 [details]
MozReview Request: Bug 1096804 - Update WindowsPreviewPerTab to support e10s. r?dao

Please use

* this.tab.ownerGlobal instead of Services.wm.getMostRecentWindow("navigator:browser")

* this.tab.linkedBrowser.getBoundingClientRect() or this.tab.linkedBrowser.clientWidth/Height instead of this.tab.linkedBrowser.boxObject


The change from 'if (this.tab.hasAttribute("pending")) {' to 'if (!this.tab.hasAttribute("pending")) {' doesn't make sense (see the comment above that line).
Attachment #8690225 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #24)
> The change from 'if (this.tab.hasAttribute("pending")) {' to 'if
> (!this.tab.hasAttribute("pending")) {' doesn't make sense (see the comment
> above that line).

I do not understand this change. The current patch paints the content if the tab if not pending, and leaves the tab blank if it is pending. If we have a blurry text problem with pending tabs and content we should do this.

The old non-e10s code did something different, it drew the chrome window into the ctx (which would include foreground tab content) and then if the tab was pending paints the desired tab content into the window using the OLD updateCanvasPreview() logic. 

What we might want to do is remove this check and see how things look for pending tabs currently.
Attachment #8690225 - Attachment is obsolete: true
Attachment #8690233 - Attachment is obsolete: true
Attachment #8690233 - Flags: review?(dao)
Attachment #8690234 - Attachment is obsolete: true
Attachment #8690235 - Attachment is obsolete: true
Attached patch part 1 - taskbar front end code (obsolete) — Splinter Review
Attachment #8691395 - Flags: review?(dao)
Comment on attachment 8691395 [details] [diff] [review]
part 1 - taskbar front end code

>+    PageThumbs.captureToCanvas(this.linkedBrowser, this.canvasPreview,
>+                               aCallback, aWantScreenshot);

I don't like the extra boolean argument (the meaning of which I already forgot again). How about adding another method for this purpose?

>+  onLocationChange: function (aBrowser) {
>+    // I'm not sure we need this, onStateChange does a really good job
>+    // of picking up page changes.
>+    //this.invalidateTabPreview(aBrowser);
>+  },

Please figure this out, we don't want to land commented out code.

Could onStateChange notify more often than we want, or miss updates that don't involve network activity? I'm not sure this is a good proxy for what we want here. Can't we keep using MozAfterPaint by moving the event listener to the content process?
Attachment #8691395 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #27)
> Comment on attachment 8691395 [details] [diff] [review]
> part 1 - taskbar front end code
> 
> >+    PageThumbs.captureToCanvas(this.linkedBrowser, this.canvasPreview,
> >+                               aCallback, aWantScreenshot);
> 
> I don't like the extra boolean argument (the meaning of which I already
> forgot again). How about adding another method for this purpose?

aWantScreenshot was poorly named, I had changed it to aNoDownscale which I think was better. I can split it out anyway. I'm not sure what method name to use.

existing apis:

captureToCanvas
captureToBlob
captureAndStore
captureAndStoreIfStale

possible apis:

captureScreenshotToCanvas
captureScreenToCanvas
captureContentToCanvas

> >+  onLocationChange: function (aBrowser) {
> >+    // I'm not sure we need this, onStateChange does a really good job
> >+    // of picking up page changes.
> >+    //this.invalidateTabPreview(aBrowser);
> >+  },
> 
> Please figure this out, we don't want to land commented out code.
> 
> Could onStateChange notify more often than we want, or miss updates that
> don't involve network activity? I'm not sure this is a good proxy for what
> we want here. Can't we keep using MozAfterPaint by moving the event listener
> to the content process?

MozAfterPaint doesn't fire for browsers anymore, hence the updates. I think I left this in for your feedback, my comment about onStateChange working suitably was based on my testing.
(In reply to Jim Mathies [:jimm] from comment #28)
> (In reply to Dão Gottwald [:dao] from comment #27)
> > Comment on attachment 8691395 [details] [diff] [review]
> > part 1 - taskbar front end code
> > 
> > >+    PageThumbs.captureToCanvas(this.linkedBrowser, this.canvasPreview,
> > >+                               aCallback, aWantScreenshot);
> > 
> > I don't like the extra boolean argument (the meaning of which I already
> > forgot again). How about adding another method for this purpose?
> 
> aWantScreenshot was poorly named, I had changed it to aNoDownscale which I
> think was better. I can split it out anyway. I'm not sure what method name
> to use.
> 
> existing apis:
> 
> captureToCanvas
> captureToBlob
> captureAndStore
> captureAndStoreIfStale
> 
> possible apis:
> 
> captureScreenshotToCanvas
> captureScreenToCanvas
> captureContentToCanvas

Hmmm, or maybe make it a named parameter:

PageThumbs.captureToCanvas(this.linkedBrowser, this.canvasPreview, aCallback, { fullScale: true });

> > >+  onLocationChange: function (aBrowser) {
> > >+    // I'm not sure we need this, onStateChange does a really good job
> > >+    // of picking up page changes.
> > >+    //this.invalidateTabPreview(aBrowser);
> > >+  },
> > 
> > Please figure this out, we don't want to land commented out code.
> > 
> > Could onStateChange notify more often than we want, or miss updates that
> > don't involve network activity? I'm not sure this is a good proxy for what
> > we want here. Can't we keep using MozAfterPaint by moving the event listener
> > to the content process?
> 
> MozAfterPaint doesn't fire for browsers anymore,

... but it does fire on the content window, right?
> > > >+  onLocationChange: function (aBrowser) {
> > > >+    // I'm not sure we need this, onStateChange does a really good job
> > > >+    // of picking up page changes.
> > > >+    //this.invalidateTabPreview(aBrowser);
> > > >+  },
> > > 
> > > Please figure this out, we don't want to land commented out code.
> > > 
> > > Could onStateChange notify more often than we want, or miss updates that
> > > don't involve network activity? I'm not sure this is a good proxy for what
> > > we want here. Can't we keep using MozAfterPaint by moving the event listener
> > > to the content process?
> > 
> > MozAfterPaint doesn't fire for browsers anymore,
> 
> ... but it does fire on the content window, right?

It fires on the top level (chrome) window. This isn't of much use though since it isn't associated  with a tab. The content window (browser.contentWindow) is over in content so we don't have direct access to that. Generally I was trying to avoid additional content script here. Plus as I mentioned the on state change STATE_STOP|STATE_IS_NETWORK works well at getting things updated as the page loads. Thumbnails won't update if something in the page changes (like a css change), I'm not sure that's something we need to be concerned about here.
(In reply to Jim Mathies [:jimm] from comment #30)
> > > > >+  onLocationChange: function (aBrowser) {
> > > > >+    // I'm not sure we need this, onStateChange does a really good job
> > > > >+    // of picking up page changes.
> > > > >+    //this.invalidateTabPreview(aBrowser);
> > > > >+  },
> > > > 
> > > > Please figure this out, we don't want to land commented out code.
> > > > 
> > > > Could onStateChange notify more often than we want, or miss updates that
> > > > don't involve network activity? I'm not sure this is a good proxy for what
> > > > we want here. Can't we keep using MozAfterPaint by moving the event listener
> > > > to the content process?
> > > 
> > > MozAfterPaint doesn't fire for browsers anymore,
> > 
> > ... but it does fire on the content window, right?
> 
> It fires on the top level (chrome) window. This isn't of much use though
> since it isn't associated  with a tab. The content window
> (browser.contentWindow) is over in content so we don't have direct access to
> that.

That's why I said "by moving the event listener to the content process"...

> Generally I was trying to avoid additional content script here.

Why?
(In reply to Dão Gottwald [:dao] from comment #31)
> (In reply to Jim Mathies [:jimm] from comment #30)
> > > > > >+  onLocationChange: function (aBrowser) {
> > > > > >+    // I'm not sure we need this, onStateChange does a really good job
> > > > > >+    // of picking up page changes.
> > > > > >+    //this.invalidateTabPreview(aBrowser);
> > > > > >+  },
> > > > > 
> > > > > Please figure this out, we don't want to land commented out code.
> > > > > 
> > > > > Could onStateChange notify more often than we want, or miss updates that
> > > > > don't involve network activity? I'm not sure this is a good proxy for what
> > > > > we want here. Can't we keep using MozAfterPaint by moving the event listener
> > > > > to the content process?
> > > > 
> > > > MozAfterPaint doesn't fire for browsers anymore,
> > > 
> > > ... but it does fire on the content window, right?
> > 
> > It fires on the top level (chrome) window. This isn't of much use though
> > since it isn't associated  with a tab. The content window
> > (browser.contentWindow) is over in content so we don't have direct access to
> > that.
> 
> That's why I said "by moving the event listener to the content process"...
> 
> > Generally I was trying to avoid additional content script here.
> 
> Why?

Added complexity and ipc traffic vs. benefit.

Thinking about how this might work: we could register a MozAfterPaint handler down in browser-child.js and forward that over, then broadcast it in chrome. Events should be limited to just active browser(s), so two tabs during tab switching and one tab after a tab switch. This should get also get paint notifications for scrolling as well since apz sends dom scroll updates over to content.

One issue come to mind, the non-e10s version of this code only updated areas of the thumbnail that changed. In this e10s version I currently ask for content window thumbnails through PageThumbs. The additional overhead of taking and shipping a new thumbnail over for every paint might be pretty bad. We could put this on a delayed timer to try and coalesce paints, or create a new api partial thumbnails or something. Thoughts? (This is the complexity I wanted to avoid, but maybe there are ways we can keep this simple.)
Flags: needinfo?(dao)
Not really finding a simple way to track paints here. We have to register for the event in content on each content window (or flip the registration around as windows become active). Then forward that over and request a thumbnail update.

If I add this I think we should coalesce paint notifications and only update every once in a while vs. trying to update in real time based on individual paint notifications.
Ok, let's stick with onStateChange for now.
Flags: needinfo?(dao)
Attachment #8690138 - Attachment is obsolete: true
Attachment #8691395 - Attachment is obsolete: true
Attachment #8699590 - Flags: review?(dao)
Dao, ping?
Attachment #8699590 - Flags: review?(dao) → review+
Attached patch part 2 - pagethumbs changes (obsolete) — Splinter Review
Attachment #8702903 - Flags: review?(dao)
deprecates the sync interface and adds a new async interface for Winodws taskbar thumbnailing.
Attachment #8702904 - Flags: review?(roc)
Attached patch part 4 - cpp widget changes (obsolete) — Splinter Review
The way this works: Windows calls us through TaskbarPreview::DrawBitmap requesting a live preview or taskbar thumbnail. In the old code we made a sync call to the controller (implemented in front end js) which rendered the image and returned. With the new code we create a callback object and request the thumbnail or live preview through the js controller, then front end code calls us back at done() with the image. We hand that to Windows after we munge it through some graphics code. Thankfully this Windows interface was designed to handle async responses.
Attachment #8702906 - Flags: review?(roc)
Comment on attachment 8702906 [details] [diff] [review]
part 4 - cpp widget changes

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

::: widget/windows/TaskbarPreview.h
@@ +125,5 @@
> +protected:
> +  virtual ~TaskbarPreviewCallback() {}
> +
> +private:
> +  HWND mPreviewWindow;

How do we ensure that mPreviewWindow remains valid during the lifetime of this object? Can something close/destroy the window before this completes?

Maybe it would be better to use a weak or strong reference to nsIWidget here and get the window that way. Then we can test whether the HWND has been destroyed?
Attachment #8702906 - Flags: review?(roc)
Comment on attachment 8702903 [details] [diff] [review]
part 2 - pagethumbs changes

>diff --git a/toolkit/components/thumbnails/PageThumbUtils.jsm b/toolkit/components/thumbnails/PageThumbUtils.jsm
>--- a/toolkit/components/thumbnails/PageThumbUtils.jsm
>+++ b/toolkit/components/thumbnails/PageThumbUtils.jsm
>@@ -130,54 +130,72 @@ this.PageThumbUtils = {
>    * at the thumbnail size. It has to do this through a two step process:
>    *
>    * 1) Render the content at the window size to a canvas that is 2x the thumbnail size
>    * 2) Downscale the canvas from (1) down to the thumbnail size
>    *
>    * This is because the thumbnail size is too small to render at directly,
>    * causing pages to believe the browser is a small resolution. Also,
>    * at that resolution, graphical artifacts / text become very jagged.
>    * It's actually better to the eye to have small blurry text than sharp
>    * jagged pixels to represent text.
>    *
>    * @params aWindow - the window to create a snapshot of.
>-   * @params aDestCanvas (optional) a destination canvas to draw the final snapshot to.
>+   * @param aArgs (optional) Additional named parameters:
>+   *   fullScale - request that a non-downscaled image be returned.
>+   * @params aDestCanvas (optional) a destination canvas to draw the final
>+   *   snapshot to.
>    * @return Canvas with a scaled thumbnail of the window.
>    */
>-  createSnapshotThumbnail: function(aWindow, aDestCanvas = null) {
>+  createSnapshotThumbnail: function(aWindow, aArgs, aDestCanvas = null) {

That signature seems a bit weird. Can you move aDestCanvas into aArgs (as just "canvas", I guess)?

We don't expect third-party code to use PageThumbUtils, right? So maybe it's not that important...
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Comment on attachment 8702906 [details] [diff] [review]
> part 4 - cpp widget changes
> 
> Review of attachment 8702906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/TaskbarPreview.h
> @@ +125,5 @@
> > +protected:
> > +  virtual ~TaskbarPreviewCallback() {}
> > +
> > +private:
> > +  HWND mPreviewWindow;
> 
> How do we ensure that mPreviewWindow remains valid during the lifetime of
> this object? Can something close/destroy the window before this completes?
> 
> Maybe it would be better to use a weak or strong reference to nsIWidget here
> and get the window that way. Then we can test whether the HWND has been
> destroyed?

I don't think it's an issue. mPreviewWindow is an hwnd, so if that goes away after we fire off an async thumbnail request to content, it means Windows no longer needs the thumbnail. I'd rather add a IsWindow check to avoid the thumbnail api calls vs. keep a widget pointer around. Will update and check this over one more time to be sure.
> We don't expect third-party code to use PageThumbUtils, right? So maybe it's
> not that important...

There are three uses according to mxr addons search - 

https://mxr.mozilla.org/addons/search?string=PageThumbUtils

but I think the changes you suggest still look safe to make.
Attachment #8702903 - Attachment is obsolete: true
Attachment #8702906 - Attachment is obsolete: true
Attachment #8702903 - Flags: review?(dao)
Comment on attachment 8703806 [details] [diff] [review]
part 2 - pagethumbs changes

That change didn't work out as well as expected. I tried adding the canvas to the args param, but I ran into a problem an inadvertent passing of args with a canvas in it to an ipc call in PageThumbs, which throws. So I skipped trying to put everything in args for createSnapshotThumbnail and gt rid of the default value for aCanvas instead.
Attachment #8703806 - Flags: review?(dao)
Attachment #8703806 - Flags: review?(dao) → review+
Attached patch parts 1 and 2 - js code (obsolete) — Splinter Review
I was testing on a high dpi device and ran into a bug which had me looking at this code again. I ended up doing additional cleanup on createSnapshotThumbnail and the front end code. Dao, would you mind looking this over?

Note, createSnapshotThumbnail still does an extra copy between canvases in certain circumstances, which I'd like to get rid of. But I think I'll do that in a follow up after this has landed.

changes:
- taskbar code is now using the canvas returned in the callback vs. the canvas handed in
- better commenting and support for fullScale requests in createSnapshotThumbnail
- high dpi bug fixed in taskbar code, the scaling was broken for like preview.
Attachment #8699590 - Attachment is obsolete: true
Attachment #8703806 - Attachment is obsolete: true
Attachment #8704313 - Flags: review?(dao)
Attached patch part 4 - cpp changes (obsolete) — Splinter Review
Attached patch part 4 - cpp changes (obsolete) — Splinter Review
added the iswindow check plus better result checking for the gfx code.
Attachment #8704316 - Attachment is obsolete: true
Attachment #8704317 - Flags: review?(roc)
Comment on attachment 8704317 [details] [diff] [review]
part 4 - cpp changes

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

I'm skeptical of the IsWindow check. As far as I know it is impossible to use IsWindow reliably because a different window may have been created with the same HWND, so we could be referring to the wrong window.
Attachment #8704317 - Flags: review?(roc)
Ok, here's a rev that keeps a ref'd pointer to the TaskbarPreview, which has some utility methods on it for checking the state of the widget associated with the window.
Attachment #8704317 - Attachment is obsolete: true
Attachment #8705179 - Flags: review?(roc)
Comment on attachment 8705179 [details] [diff] [review]
part 4 - cpp changes

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

I'm not super comfortable with this ... AFAICT things could still go wrong, getting the wrong window.

Is there a reason why we can't hold a weak reference to the nsWindow, so that we can reliably detect if the window died?
Attachment #8705179 - Flags: review?(roc)
(In reply to Jim Mathies [:jimm] from comment #46)
> Created attachment 8704313 [details] [diff] [review]
> parts 1 and 2 - js code
> 
> I was testing on a high dpi device and ran into a bug which had me looking
> at this code again. I ended up doing additional cleanup on
> createSnapshotThumbnail and the front end code. Dao, would you mind looking
> this over?
> 
> Note, createSnapshotThumbnail still does an extra copy between canvases in
> certain circumstances, which I'd like to get rid of. But I think I'll do
> that in a follow up after this has landed.
> 
> changes:
> - taskbar code is now using the canvas returned in the callback vs. the
> canvas handed in
> - better commenting and support for fullScale requests in
> createSnapshotThumbnail
> - high dpi bug fixed in taskbar code, the scaling was broken for like
> preview.

Do you have an interdiff? That would speed up the review.
(In reply to Dão Gottwald [:dao] from comment #52)
> Do you have an interdiff? That would speed up the review.

nope.
Can you generate one?
Comment on attachment 8704313 [details] [diff] [review]
parts 1 and 2 - js code

tried to generate the interdiff myself but this patch doesn't seem to apply cleanly on fx-team tip
Attachment #8704313 - Flags: review?(dao)
*sigh* Looks like http://hg.mozilla.org/mozilla-central/rev/fd998a174ee5 bitrotted you for no good reason.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #51)
> Comment on attachment 8705179 [details] [diff] [review]
> part 4 - cpp changes
> 
> Review of attachment 8705179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not super comfortable with this ... AFAICT things could still go wrong,
> getting the wrong window.

I don't see how that's possible here. We create and destroy TaskbarTabPreviews in response to TabOpen and TabClose events in the js code. When a tab closes, we run TaskbarTabPreview::Disable() [1], which runs TaskbarPreview::Disable(). This destroys the proxy window TaskbarTabPreview creates registers with Windows, and clears the internal variable it stores the proxy HWND in. The proxy HWND is kept in mProxyWindow which is what TaskbarTabPreview returns from PreviewWindow(). So TaskbarTabPreview::PreviewWindow() will return null as soon as all the TabClose handler code finishes. In my new code here, I check the result of PreviewWindow() null in nsITaskbarPreviewCallbacks done() handler before making any dwm thumbnail apis calls.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/modules/WindowsPreviewPerTab.jsm#509
[2] https://dxr.mozilla.org/mozilla-central/source/widget/windows/TaskbarTabPreview.cpp#251

> Is there a reason why we can't hold a weak reference to the nsWindow, so
> that we can reliably detect if the window died?

As long as we have a ref'd pointer to TaskbarTabPreview, I don't think we need it. Besides, we want to track tab lifetime here, not window. When the underlying nsWindow closes, each tab fires TabClose, and all the tab close code I've described above runs.
Flags: needinfo?(roc)
(In reply to Dão Gottwald [:dao] from comment #54)
> Can you generate one?

yeah I can try.
Attachment #8699590 - Attachment is obsolete: false
Attachment #8705643 - Attachment is obsolete: true
Attachment #8704313 - Attachment is obsolete: true
Attachment #8703806 - Attachment is obsolete: false
Attachment #8705662 - Flags: review?(dao)
Attachment #8705663 - Flags: review?(dao)
Attachment #8705662 - Flags: review?(dao) → review+
(In reply to Jim Mathies [:jimm] from comment #58)
> I don't see how that's possible here. We create and destroy
> TaskbarTabPreviews in response to TabOpen and TabClose events in the js
> code. When a tab closes, we run TaskbarTabPreview::Disable() [1], which runs
> TaskbarPreview::Disable(). This destroys the proxy window TaskbarTabPreview
> creates registers with Windows, and clears the internal variable it stores
> the proxy HWND in. The proxy HWND is kept in mProxyWindow which is what
> TaskbarTabPreview returns from PreviewWindow(). So
> TaskbarTabPreview::PreviewWindow() will return null as soon as all the
> TabClose handler code finishes. In my new code here, I check the result of
> PreviewWindow() null in nsITaskbarPreviewCallbacks done() handler before
> making any dwm thumbnail apis calls.

OK. Please document this in the code somewhere.
Flags: needinfo?(roc)
Attachment #8705663 - Flags: review?(dao) → review+
try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd15629bbdb2

I've enabled some taskbar specific tests as well, and removed on part of those tests that uses the old sync api.
Blocks: 1256714
Depends on: 1387802
You need to log in before you can comment on or make changes to this bug.