Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577273002:
Clean up IconLoader. (Closed)
|
Created:
4 years ago by Avi (use Gerrit) Modified:
4 years ago Reviewers:
sky CC:
chromium-reviews, asanka, extensions-reviews_chromium.org, tfarina, mac-reviews_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-downloads_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up IconLoader.
There are lifetime issues due to IconLoader being refcounted for no good reason, so stop doing that.
BUG=674743
Committed: https://crrev.com/f0a7b5b81faa4d2abf594b6e3ac135ed60dad504
Cr-Commit-Position: refs/heads/master@{#439348}
Patch Set 1 #Patch Set 2 : smaller scope #
Total comments: 7
Patch Set 3 : lifetime fixed #
Total comments: 8
Patch Set 4 : bind #
Messages
Total messages: 45 (26 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Clean up IconLoader and IconManager. There are lifetime issues due to IconLoader being refcounted for no good reason, plus IconManager's loading is cancelable despite no one actually using it. BUG=674743 ========== to ========== Clean up IconLoader. There are lifetime issues due to IconLoader being refcounted for no good reason, so stop doing that. BUG=674743 ==========
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avi@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... File chrome/browser/icon_loader.cc (right): https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, base::Unretained(this)), Don't you need a weakptr here for the case of IconManager deleting IconLoader while passing to another thread? https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... File chrome/browser/icon_loader.h (right): https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... chrome/browser/icon_loader.h:46: using IconLoadedCallback = base::Callback< Would it make sense to make this a OnceCallback?
I'm getting dizzy thinking about the possibility now of IconManager being deleted while IconLoader is bouncing around on different threads... https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... File chrome/browser/icon_loader.cc (right): https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, base::Unretained(this)), On 2016/12/16 18:21:10, sky wrote: > Don't you need a weakptr here for the case of IconManager deleting IconLoader > while passing to another thread? Sorry, I'm not following you. IconManager creates IconLoader, and doesn't delete it until it calls back on the load. Or are you worried about the deletion when IconManager is destroyed and kills all the IconLoaders early? In that case, wouldn't all the Binds have issues? https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... File chrome/browser/icon_loader.h (right): https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... chrome/browser/icon_loader.h:46: using IconLoadedCallback = base::Callback< On 2016/12/16 18:21:10, sky wrote: > Would it make sense to make this a OnceCallback? Suppose so. I saw something about that fly by though I didn't think about it too hard.
https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... File chrome/browser/icon_loader.h (right): https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... chrome/browser/icon_loader.h:46: using IconLoadedCallback = base::Callback< On 2016/12/16 19:48:51, Avi wrote: > On 2016/12/16 18:21:10, sky wrote: > > Would it make sense to make this a OnceCallback? > > Suppose so. I saw something about that fly by though I didn't think about it too > hard. OK, this is nuts. I switch in a OnceCallback. Sprinkle std::move() everywhere it's copied around. OK. Trying to actually call Run on it requires std::move. OK. And I still get: ../../chrome/browser/icon_manager.cc:24:23: error: no matching member function for call to 'Run' std::move(callback).Run(image); ~~~~~~~~~~~~~~~~~~~~^~~ ../../base/callback.h:44:5: note: candidate function not viable: no known conversion from 'typename remove_reference<const Callback<void (Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> &>::type' (aka 'const base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>') to 'base::internal::RunMixin<base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' for object argument R Run(Args... args) & { ^ ../../base/callback.h:60:5: note: candidate function not viable: no known conversion from 'typename remove_reference<const Callback<void (Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> &>::type' (aka 'const base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>') to 'base::internal::RunMixin<base::Callback<void (gfx::Image *), base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' for object argument R Run(Args... args) && { ^ I am simply not smart enough to work OnceCallback.
https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... File chrome/browser/icon_loader.h (right): https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... chrome/browser/icon_loader.h:46: using IconLoadedCallback = base::Callback< OK, I think I have that problem figured out.
You can't PostTask a OnceCallback so this is a problem.
On 2016/12/16 20:39:35, Avi wrote:
> You can't PostTask a OnceCallback so this is a problem.
And I'm still getting absurd errors:
In file included from ../../chrome/browser/icon_manager.cc:5:
In file included from ../../chrome/browser/icon_manager.h:53:
In file included from ../../base/task/cancelable_task_tracker.h:42:
In file included from ../../base/bind.h:8:
In file included from ../../base/bind_internal.h:13:
../../base/bind_helpers.h:281:13: error: 'mutable' and 'const' cannot be mixed
mutable T scoper_;
^
../../chrome/browser/icon_manager.cc:70:47: note: in instantiation of template
class 'base::internal::PassedWrapper<const base::Callback<void (gfx::Image *),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >'
requested here
&RunCallbackIfNotCanceled, is_canceled, base::Passed(&callback));
^
../../chrome/browser/icon_manager.cc:91:40: error: no matching member function
for call to 'Run'
std::move(client_request.callback).Run(result.get());
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
../../base/callback.h:44:5: note: candidate function not viable: no known
conversion from 'typename remove_reference<const Callback<void (Image *),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> &>::type'
(aka 'const base::Callback<void (gfx::Image *),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>') to
'base::internal::RunMixin<base::Callback<void (gfx::Image *),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' for
object argument
R Run(Args... args) & {
^
../../base/callback.h:60:5: note: candidate function not viable: no known
conversion from 'typename remove_reference<const Callback<void (Image *),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> &>::type'
(aka 'const base::Callback<void (gfx::Image *),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>') to
'base::internal::RunMixin<base::Callback<void (gfx::Image *),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once> >' for
object argument
R Run(Args... args) && {
^
https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... File chrome/browser/icon_loader.cc (right): https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, base::Unretained(this)), On 2016/12/16 19:48:51, Avi wrote: > On 2016/12/16 18:21:10, sky wrote: > > Don't you need a weakptr here for the case of IconManager deleting IconLoader > > while passing to another thread? > > Sorry, I'm not following you. > > IconManager creates IconLoader, and doesn't delete it until it calls back on the > load. Or are you worried about the deletion when IconManager is destroyed and > kills all the IconLoaders early? Yes. > In that case, wouldn't all the Binds have > issues? Indeed. That's why I'm saying it seems like you need a WeakPtr for this class so that the binds end up doing nothing. That said, this class is used from multiple threads, and I'm not sure WeakPtrFactory works in that scenario.
On 2016/12/16 20:46:08, sky wrote: > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... > File chrome/browser/icon_loader.cc (right): > > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... > chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, > base::Unretained(this)), > On 2016/12/16 19:48:51, Avi wrote: > > On 2016/12/16 18:21:10, sky wrote: > > > Don't you need a weakptr here for the case of IconManager deleting > IconLoader > > > while passing to another thread? > > > > Sorry, I'm not following you. > > > > IconManager creates IconLoader, and doesn't delete it until it calls back on > the > > load. Or are you worried about the deletion when IconManager is destroyed and > > kills all the IconLoaders early? > > Yes. > > > In that case, wouldn't all the Binds have > > issues? > > Indeed. That's why I'm saying it seems like you need a WeakPtr for this class so > that the binds end up doing nothing. That said, this class is used from multiple > threads, and I'm not sure WeakPtrFactory works in that scenario. Yeah. If the IconManager kills the IconLoader on another thread while the IconLoader is actively doing something we segfault, weak_ptr or not. OK, suppose IconManager gives the IconLoader a weak pointer, and the IconLoader owns itself. IconLoader goes and loads, and maybe delivers a result to the IconManager if it's still around. Sound good?
On 2016/12/16 21:00:11, Avi wrote: > On 2016/12/16 20:46:08, sky wrote: > > > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... > > File chrome/browser/icon_loader.cc (right): > > > > > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... > > chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, > > base::Unretained(this)), > > On 2016/12/16 19:48:51, Avi wrote: > > > On 2016/12/16 18:21:10, sky wrote: > > > > Don't you need a weakptr here for the case of IconManager deleting > > IconLoader > > > > while passing to another thread? > > > > > > Sorry, I'm not following you. > > > > > > IconManager creates IconLoader, and doesn't delete it until it calls back on > > the > > > load. Or are you worried about the deletion when IconManager is destroyed > and > > > kills all the IconLoaders early? > > > > Yes. > > > > > In that case, wouldn't all the Binds have > > > issues? > > > > Indeed. That's why I'm saying it seems like you need a WeakPtr for this class > so > > that the binds end up doing nothing. That said, this class is used from > multiple > > threads, and I'm not sure WeakPtrFactory works in that scenario. > > Yeah. If the IconManager kills the IconLoader on another thread while the > IconLoader is actively doing something we segfault, weak_ptr or not. > > OK, suppose IconManager gives the IconLoader a weak pointer, and the IconLoader > owns itself. IconLoader goes and loads, and maybe delivers a result to the > IconManager if it's still around. Sound good? (BTW, I refuse to use OnceCallback. I am literally not smart enough to make it work. See https://groups.google.com/a/chromium.org/d/msg/cxx/WOODFJjIOi0/tno3iK0iAAAJ .)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
SGTM On Fri, Dec 16, 2016 at 1:00 PM, <avi@chromium.org> wrote: > On 2016/12/16 20:46:08, sky wrote: >> > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... >> File chrome/browser/icon_loader.cc (right): >> >> > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... >> chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, >> base::Unretained(this)), >> On 2016/12/16 19:48:51, Avi wrote: >> > On 2016/12/16 18:21:10, sky wrote: >> > > Don't you need a weakptr here for the case of IconManager deleting >> IconLoader >> > > while passing to another thread? >> > >> > Sorry, I'm not following you. >> > >> > IconManager creates IconLoader, and doesn't delete it until it calls >> > back on >> the >> > load. Or are you worried about the deletion when IconManager is >> > destroyed > and >> > kills all the IconLoaders early? >> >> Yes. >> >> > In that case, wouldn't all the Binds have >> > issues? >> >> Indeed. That's why I'm saying it seems like you need a WeakPtr for this >> class > so >> that the binds end up doing nothing. That said, this class is used from > multiple >> threads, and I'm not sure WeakPtrFactory works in that scenario. > > Yeah. If the IconManager kills the IconLoader on another thread while the > IconLoader is actively doing something we segfault, weak_ptr or not. > > OK, suppose IconManager gives the IconLoader a weak pointer, and the > IconLoader > owns itself. IconLoader goes and loads, and maybe delivers a result to the > IconManager if it's still around. Sound good? > > https://codereview.chromium.org/2577273002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for trying! I honestly haven't tried using it, but given we're trying to convert to using Once/Repeat it's good to try, and more importantly to provide feedback if it's painful to use. Which you did. Thanks! -Scott On Fri, Dec 16, 2016 at 1:15 PM, <avi@chromium.org> wrote: > On 2016/12/16 21:00:11, Avi wrote: >> On 2016/12/16 20:46:08, sky wrote: >> > >> > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... >> > File chrome/browser/icon_loader.cc (right): >> > >> > >> > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... >> > chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, >> > base::Unretained(this)), >> > On 2016/12/16 19:48:51, Avi wrote: >> > > On 2016/12/16 18:21:10, sky wrote: >> > > > Don't you need a weakptr here for the case of IconManager deleting >> > IconLoader >> > > > while passing to another thread? >> > > >> > > Sorry, I'm not following you. >> > > >> > > IconManager creates IconLoader, and doesn't delete it until it calls >> > > back > on >> > the >> > > load. Or are you worried about the deletion when IconManager is >> > > destroyed >> and >> > > kills all the IconLoaders early? >> > >> > Yes. >> > >> > > In that case, wouldn't all the Binds have >> > > issues? >> > >> > Indeed. That's why I'm saying it seems like you need a WeakPtr for this > class >> so >> > that the binds end up doing nothing. That said, this class is used from >> multiple >> > threads, and I'm not sure WeakPtrFactory works in that scenario. >> >> Yeah. If the IconManager kills the IconLoader on another thread while the >> IconLoader is actively doing something we segfault, weak_ptr or not. >> >> OK, suppose IconManager gives the IconLoader a weak pointer, and the > IconLoader >> owns itself. IconLoader goes and loads, and maybe delivers a result to the >> IconManager if it's still around. Sound good? > > (BTW, I refuse to use OnceCallback. I am literally not smart enough to make > it > work. See > https://groups.google.com/a/chromium.org/d/msg/cxx/WOODFJjIOi0/tno3iK0iAAAJ > .) > > https://codereview.chromium.org/2577273002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/16 21:57:12, sky wrote: > Thanks for trying! > > I honestly haven't tried using it, but given we're trying to convert > to using Once/Repeat it's good to try, and more importantly to provide > feedback if it's painful to use. Which you did. Thanks! > > -Scott > > On Fri, Dec 16, 2016 at 1:15 PM, <mailto:avi@chromium.org> wrote: > > On 2016/12/16 21:00:11, Avi wrote: > >> On 2016/12/16 20:46:08, sky wrote: > >> > > >> > > > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... > >> > File chrome/browser/icon_loader.cc (right): > >> > > >> > > >> > > > https://codereview.chromium.org/2577273002/diff/20001/chrome/browser/icon_loa... > >> > chrome/browser/icon_loader.cc:26: base::Bind(&IconLoader::ReadGroup, > >> > base::Unretained(this)), > >> > On 2016/12/16 19:48:51, Avi wrote: > >> > > On 2016/12/16 18:21:10, sky wrote: > >> > > > Don't you need a weakptr here for the case of IconManager deleting > >> > IconLoader > >> > > > while passing to another thread? > >> > > > >> > > Sorry, I'm not following you. > >> > > > >> > > IconManager creates IconLoader, and doesn't delete it until it calls > >> > > back > > on > >> > the > >> > > load. Or are you worried about the deletion when IconManager is > >> > > destroyed > >> and > >> > > kills all the IconLoaders early? > >> > > >> > Yes. > >> > > >> > > In that case, wouldn't all the Binds have > >> > > issues? > >> > > >> > Indeed. That's why I'm saying it seems like you need a WeakPtr for this > > class > >> so > >> > that the binds end up doing nothing. That said, this class is used from > >> multiple > >> > threads, and I'm not sure WeakPtrFactory works in that scenario. > >> > >> Yeah. If the IconManager kills the IconLoader on another thread while the > >> IconLoader is actively doing something we segfault, weak_ptr or not. > >> > >> OK, suppose IconManager gives the IconLoader a weak pointer, and the > > IconLoader > >> owns itself. IconLoader goes and loads, and maybe delivers a result to the > >> IconManager if it's still around. Sound good? > > > > (BTW, I refuse to use OnceCallback. I am literally not smart enough to make > > it > > work. See > > https://groups.google.com/a/chromium.org/d/msg/cxx/WOODFJjIOi0/tno3iK0iAAAJ > > .) > > > > https://codereview.chromium.org/2577273002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > NP. The killer for now is the fact that you can't PostTask a OnceCallback, so that kinda kills it right there. Anyway, PTAL at patch set 3. I'm pretty sure this will end up green.
https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... File chrome/browser/icon_loader.cc (right): https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... chrome/browser/icon_loader.cc:13: /*static*/ style for these is // static https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... chrome/browser/icon_loader.cc:28: void IconLoader::Start() { move above constructor (to match declaration order). https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... File chrome/browser/icon_loader.h (right): https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... chrome/browser/icon_loader.h:47: void(IconLoader*, std::unique_ptr<gfx::Image>, const IconGroup&)>; Given that the IconLoader may be deleted by the time this is called could this take a void* to reinforce the pointer shouldn't be used? Alternatively change IconManager to supply and integer id that it comes up with, which I think is all IconManager really needs. https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_man... File chrome/browser/icon_manager.cc (right): https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_man... chrome/browser/icon_manager.cc:29: IconLoader* loader; Do you need this anymore?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... File chrome/browser/icon_loader.cc (right): https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... chrome/browser/icon_loader.cc:13: /*static*/ On 2016/12/16 23:27:27, sky wrote: > style for these is > // static Done. https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... chrome/browser/icon_loader.cc:28: void IconLoader::Start() { On 2016/12/16 23:27:27, sky wrote: > move above constructor (to match declaration order). Done. https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... File chrome/browser/icon_loader.h (right): https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_loa... chrome/browser/icon_loader.h:47: void(IconLoader*, std::unique_ptr<gfx::Image>, const IconGroup&)>; On 2016/12/16 23:27:27, sky wrote: > Given that the IconLoader may be deleted by the time this is called could this > take a void* to reinforce the pointer shouldn't be used? Alternatively change > IconManager to supply and integer id that it comes up with, which I think is all > IconManager really needs. Or drop it, and have callers pre-bind the info they need. See the next patch. https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_man... File chrome/browser/icon_manager.cc (right): https://codereview.chromium.org/2577273002/diff/40001/chrome/browser/icon_man... chrome/browser/icon_manager.cc:29: IconLoader* loader; On 2016/12/16 23:27:27, sky wrote: > Do you need this anymore? I don't need any of this any more :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! LGTM
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482001014401100,
"parent_rev": "6dc264c71caa0508c0a9cc9eba854dd082bca2be", "commit_rev":
"e497724a0de5c1c4309878269bfdffdd3a15753e"}
Message was sent while issue was closed.
Description was changed from ========== Clean up IconLoader. There are lifetime issues due to IconLoader being refcounted for no good reason, so stop doing that. BUG=674743 ========== to ========== Clean up IconLoader. There are lifetime issues due to IconLoader being refcounted for no good reason, so stop doing that. BUG=674743 Review-Url: https://codereview.chromium.org/2577273002 Committed: https://chromium.googlesource.com/chromium/src/+/e497724a0de5c1c4309878269bfd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e497724a0de5c1c4309878269bfd...
Message was sent while issue was closed.
Description was changed from ========== Clean up IconLoader. There are lifetime issues due to IconLoader being refcounted for no good reason, so stop doing that. BUG=674743 Review-Url: https://codereview.chromium.org/2577273002 Committed: https://chromium.googlesource.com/chromium/src/+/e497724a0de5c1c4309878269bfd... ========== to ========== Clean up IconLoader. There are lifetime issues due to IconLoader being refcounted for no good reason, so stop doing that. BUG=674743 Committed: https://crrev.com/f0a7b5b81faa4d2abf594b6e3ac135ed60dad504 Cr-Commit-Position: refs/heads/master@{#439348} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f0a7b5b81faa4d2abf594b6e3ac135ed60dad504 Cr-Commit-Position: refs/heads/master@{#439348} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
