Issue metadata
Sign in to add a comment
|
Webextensions' isWindowClosing from chrome.tabs.onRemoved info is always true
Reported by
seb.bl...@gmail.com,
Aug 7
|
||||||||||||||||||||||
Issue description
UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/68.0.3440.75 Chrome/68.0.3440.75 Safari/537.36
Steps to reproduce the problem:
1. Create a webextension with following code: chrome.tabs.onRemoved.addListener((id, info) => { console.log(id, info); });
2. close the last tab
3. reopen the browser and close the window
What is the expected behavior?
When closing the last tab, info.isWindowClosing should be false, because the tab is not closed due to the window being closed (this is the behaviour pre Chromium 68). It should only be true if the window is closed explicitly.
What went wrong?
Since Chromium 68, info.isWindowClosing is true when I close the last tab and when I close the window which makes it useless.
Did this work before? Yes 67
Chrome version: 68.0.3440.75 Channel: n/a
OS Version:
Flash Version:
For the code where this matters look at https://chrome.google.com/webstore/detail/do-not-close-browser-with/iiolkehjeklhkdphaakkceadenbcdahj
,
Aug 8
Able to reproduce the issue on reported version 68.0.3440.75, 68.0.3440.84 and on latest chrome# 70.0.3515.0 using Ubuntu 14.04 and Windows-10, hence providing Bisect Info Note: Issue is not applicable to Mac Bisect Info: ================ Good build: 68.0.3430.0 Bad build: 68.0.3431.0 On running per-revision bisect, got error: "Exception: Error running the gsutil command: ServiceException: 401 Anonymous caller does not have storage.objects.get access to chrome-test-builds/official-by-commit/Win x64 Builder" and below is the change-log from chromium bisect You are probably looking for a change made after 558524 (known good), but no later than 558530 (first known bad). https://chromium.googlesource.com/chromium/src/+log/dd09cdf9373c2d180da6a82d2eda2d49dafb9508..f64c33d60de88b6b0d1a3d65860a710ab37dafac Change-Id: I2fc54749e495ab9c925bdba89ef857c4ea3537d9 Reviewed-on: https://chromium-review.googlesource.com/1055732 @erikchen: Please confirm the issue and help in re-assigning if it is not related to your change. Adding ReleaseBlock-Stable as it is seems a recent break, feel free to remove it if not applicable. Thanks!
,
Aug 8
,
Aug 8
,
Aug 9
This feature was added here: https://bugs.chromium.org/p/chromium/issues/detail?id=56592 """ Because closing a window first triggers onTabClosed events for each tab, there is no way to tell whether a tab is closing because the user explicitly closed that tab, or because the user closed the whole window. An example of how this would be useful is my extension SimpleWindowSaver. When a user closes a tab in a saved window, I want to remove that tab from the saved state. However, when the user closes the window, I want to save all the state so they can restore it later. """ This leaves open the question -- what is the appropriate behavior when a user closes the last tab in a window, which will also close the Window. This isn't particularly well defined. Right now, we set closing_all_ = true in two places: 1) CloseAllTabs() -- this gets called when closing the Window 2) DetachWebContentsImpl, when detaching the last tab. (2) violates the public comment on closing_all(): """ 145 // Returns true if the tabstrip is currently closing all open tabs (via a 146 // call to CloseAllTabs) """ Looking at all callsites for closing_all(), they appear to want the semantics described in the comment. As such, removing (2) should fix this bug, and potential fix some other similar issues.
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/25919fcea95287f1be0ac0f686cbfc118faa1aca commit 25919fcea95287f1be0ac0f686cbfc118faa1aca Author: erikchen <erikchen@chromium.org> Date: Fri Aug 10 13:17:41 2018 Change implementation of TabStripModel::closing_all(). The semantics of TabStripModel::closing_all() [both in the header comments, and in usage by callsites] is that it should only be set to |true| when CloseAllTabs() has been called. However, it was also being set to |true| in TabStripModel::DetachWebContentsImpl. This was incorrect. It appears to have been present in the initial commit of chrome/ to the Chromium repository: 09911bf300f1a419907a9412154760efd0b7abc3. Bug: 871977 Change-Id: Ied0dcb79611f1c2b144657dec897551d3734a38a Reviewed-on: https://chromium-review.googlesource.com/1169622 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#582130} [modify] https://crrev.com/25919fcea95287f1be0ac0f686cbfc118faa1aca/chrome/browser/ui/tabs/tab_strip_model.cc
,
Aug 13
Able to reproduce the issue on chrome reported version 68.0.3440.75 Verified the fix on Windows-10 & Ubuntu 14.04 on Chrome version #70.0.3521.0 as per the comment#0 Attaching screen cast for reference. Observed "Unable to close the window through tab close button when only one tab is present" Hence, the fix is working as expected. Adding the verified label. Thanks!
,
Aug 13
,
Aug 13
[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.
,
Aug 13
This is regressed in M68, so lets target fix for M70. Pls let me know ASAP if there is any concern here.
,
Aug 13
That sounds good.
,
Aug 13
Thank you erikchen@. Removing M68/M69 milestones.
,
Aug 13
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by vamshi.kommuri@chromium.org
, Aug 8