New issue
Advanced search Search tips

Issue 673578 link

Starred by 1 user
Status: Untriaged
Owner: ----
Cc:
a...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

GlobalErrorService should own all its errors, and destroying errors should close supporting ui

Project Member Reported by a...@chromium.org, Dec 13 2016 
In refactoring, the ownership story for GlobalErrorService is crazy; it sometimes owns the errors and sometimes not.

Some preliminary work has been done to clear this up, and to divide up the errors into "owned" and "unowned" but that's only slightly less crazy.

All errors in the GlobalErrorService should be owned by the GlobalErrorService.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 13 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2451b25ae9188cfe5e0177c9c306ced7c3a173ed

commit 2451b25ae9188cfe5e0177c9c306ced7c3a173ed
Author: avi <avi@chromium.org>
Date: Tue Dec 13 16:55:17 2016

Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained were owned, and sometimes they weren't. This separates the two cases into clear statements of ownership, and deprecates the case where the error objects are unowned.

BUG= 555865 ,673578

Review-Url: https://codereview.chromium.org/2409443002
Cr-Commit-Position: refs/heads/master@{#438192}

[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/extensions/extension_disabled_ui.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/extensions/external_install_error.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/extensions/warning_badge_service.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/recovery/recovery_install_global_error.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/safe_browsing/srt_fetcher_win.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/safe_browsing/srt_global_error_win.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/sync/sync_global_error.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/ui/global_error/global_error_service.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/ui/global_error/global_error_service.h
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/ui/global_error/global_error_service_browsertest.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/ui/global_error/global_error_service_unittest.cc
[modify] https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed/chrome/browser/ui/toolbar/app_menu_model_unittest.cc

Comment 2 by sky@chromium.org, Feb 4 2017

Summary: GlobalErrorService should own all its errors, and destroying errors should close supporting ui (was: GlobalErrorService should own all its errors)
I'm also adding to this that when an error is destroyed any ui we are showing should be torn down as well. The current approach is to use a weakptr, which still leaves the ui up. That is just wrong.

Comment 3 by lethalantidote@chromium.org, Feb 9 2017

Owner: lethalantidote@chromium.org
Status: Assigned (was: Untriaged)

Comment 4 by lethalantidote@chromium.org, Sep 13 2017

Owner: ----
Status: Available (was: Assigned)
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 13

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
►

Sign in to add a comment