1096804 - [e10s] Support windows taskbar previews and thumbnails

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Tony Paloma]

Attachment: Only one tab while e10s enabled.png

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.

Comment 1

[Tony Paloma]

Attachment: Typical tabs - expected result.png

The expected result

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Felipe, Bill says you told him this feature was turned off. True?

Comment 3

[Paul Silaghi, QA [:pauly]]

To me, it looks the same both with e10s ON/OFF.
36.0a1 (2014-11-17) Win 7 x64

Comment 4

[Tony Paloma]

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

Comment 5

[Tony Paloma]

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

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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

Comment 7

[juan becerra [:juanb]]

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.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

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

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

product-wanted to ask if we should block on this non-default behavior, or just remove the option.

Comment 11

[Jim Mathies [:jimm]]

Attachment: wip

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.

Comment 12

[Jim Mathies [:jimm]]

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

Comment 13

[Jim Mathies [:jimm]]

Attachment: wip

transparency and refresh issues worked out.

Comment 14

[Jim Mathies [:jimm]]

Attachment: rollup

Comment 15

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1096804 - Update WindowsPreviewPerTab to support e10s. r?dao

Bug 1096804 - Update WindowsPreviewPerTab to support e10s. r?dao

Comment 16

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1096804 - Add support to PageThumbs for requesting non-downlampled window screenshots. r?dao

Bug 1096804 - Add support to PageThumbs for requesting non-downlampled window screenshots. r?dao

Comment 17

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1096804 - Add a new asynchronous callback interface for thumbnail and preview image requests.

Bug 1096804 - Add a new asynchronous callback interface for thumbnail and preview image requests.

Comment 18

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1096804 - Update taskbar preview image manipulation code so that it can work with a passed in canvas vs. a locally stored canvas.

Bug 1096804 - Update taskbar preview image manipulation code so that it can work with a passed in canvas vs. a locally stored canvas.

Comment 19

[Jim Mathies [:jimm]]

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/

Comment 20

[Jim Mathies [:jimm]]

hmm, rats. sorry for the spam, I was hoping that would update just one of the requests.

Comment 21

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1096804 - Add support to PageThumbs for requesting non-downlampled window screenshots. r?dao

Bug 1096804 - Add support to PageThumbs for requesting non-downlampled window screenshots. r?dao

Comment 22

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1096804 - Add a new asynchronous callback interface for thumbnail and preview image requests.

Bug 1096804 - Add a new asynchronous callback interface for thumbnail and preview image requests.

Comment 23

[Jim Mathies [:jimm]]

Attachment: MozReview Request: Bug 1096804 - Update taskbar preview image manipulation code so that it can work with a passed in canvas vs. a locally stored canvas.

Bug 1096804 - Update taskbar preview image manipulation code so that it can work with a passed in canvas vs. a locally stored canvas.

Comment 24

[Dão Gottwald [:dao]]

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).

Comment 25

[Jim Mathies [:jimm]]

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

Comment 26

[Jim Mathies [:jimm]]

Attachment: part 1 - taskbar front end code

Comment 27

[Dão Gottwald [: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?

Comment 28

[Jim Mathies [:jimm]]

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

Comment 29

[Dão Gottwald [:dao]]

(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?

Comment 30

[Jim Mathies [:jimm]]

> > > >+  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.

Comment 31

[Dão Gottwald [:dao]]

(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?

Comment 32

[Jim Mathies [:jimm]]

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

Comment 33

[Jim Mathies [:jimm]]

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.

Comment 34

[Dão Gottwald [:dao]]

Ok, let's stick with onStateChange for now.

Comment 35

[Jim Mathies [:jimm]]

Attachment: part 1 - taskbar front end code

Comment 36

[Jim Mathies [:jimm]]

Dao, ping?

Comment 37

[Jim Mathies [:jimm]]

Attachment: part 2 - pagethumbs changes

Comment 38

[Jim Mathies [:jimm]]

Attachment: part 3 - xpcom taskbar preview interface changes

deprecates the sync interface and adds a new async interface for Winodws taskbar thumbnailing.

Comment 39

[Jim Mathies [:jimm]]

Attachment: part 4 - cpp widget changes

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.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 41

[Dão Gottwald [:dao]]

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

Comment 42

[Jim Mathies [:jimm]]

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

Comment 43

[Jim Mathies [:jimm]]

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

Comment 44

[Jim Mathies [:jimm]]

Attachment: part 2 - pagethumbs changes

Comment 45

[Jim Mathies [:jimm]]

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.

Comment 46

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 47

[Jim Mathies [:jimm]]

Attachment: part 4 - cpp changes

Comment 48

[Jim Mathies [:jimm]]

Attachment: part 4 - cpp changes

added the iswindow check plus better result checking for the gfx code.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 50

[Jim Mathies [:jimm]]

Attachment: part 4 - cpp changes

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.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 52

[Dão Gottwald [:dao]]

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

Comment 53

[Jim Mathies [:jimm]]

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

nope.

Comment 54

[Dão Gottwald [:dao]]

Can you generate one?

Comment 55

[Dão Gottwald [:dao]]

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

Comment 56

[Dão Gottwald [:dao]]

*sigh* Looks like http://hg.mozilla.org/mozilla-central/rev/fd998a174ee5 bitrotted you for no good reason.

Comment 57

[Dão Gottwald [:dao]]

Attachment: interdiff for latest part 1 and 2 changes

Comment 58

[Jim Mathies [:jimm]]

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

Comment 59

[Jim Mathies [:jimm]]

(In reply to Dão Gottwald [:dao] from comment #54)
> Can you generate one?

yeah I can try.

Comment 60

[Jim Mathies [:jimm]]

Attachment: part 1.5 - taskbar front end code

Comment 61

[Jim Mathies [:jimm]]

Attachment: part 2.5 - pagethumbs changes

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 63

[Jim Mathies [:jimm]]

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.

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f077e048a451
https://hg.mozilla.org/integration/mozilla-inbound/rev/49de4e1f40e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8afe4f072efd
https://hg.mozilla.org/integration/mozilla-inbound/rev/d71559a65774
https://hg.mozilla.org/integration/mozilla-inbound/rev/deaf7c6ca55b

Comment 65

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a018997bc53a

Comment 66

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26a221678726

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f077e048a451
https://hg.mozilla.org/mozilla-central/rev/49de4e1f40e6
https://hg.mozilla.org/mozilla-central/rev/8afe4f072efd
https://hg.mozilla.org/mozilla-central/rev/d71559a65774
https://hg.mozilla.org/mozilla-central/rev/deaf7c6ca55b
https://hg.mozilla.org/mozilla-central/rev/a018997bc53a
https://hg.mozilla.org/mozilla-central/rev/26a221678726